-
Notifications
You must be signed in to change notification settings - Fork 131
Add test case verifiying capsule sync in presence of null facet #20471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test case verifiying capsule sync in presence of null facet #20471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The
timestampvariable andnailgun_capsuleassignment at the end of the test are unused beyond immediate calls; consider inliningmodule_capsule_configured.nailgun_capsule.content_sync(timeout='90m')and removing the extratimestampcreation to keep the test focused and avoid dead code. - If possible, consider adding an explicit assertion that the host’s subscription facet (or its UUID) is null before triggering the capsule sync, so the test more directly verifies the specific regression scenario it is intended to cover.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `timestamp` variable and `nailgun_capsule` assignment at the end of the test are unused beyond immediate calls; consider inlining `module_capsule_configured.nailgun_capsule.content_sync(timeout='90m')` and removing the extra `timestamp` creation to keep the test focused and avoid dead code.
- If possible, consider adding an explicit assertion that the host’s subscription facet (or its UUID) is null before triggering the capsule sync, so the test more directly verifies the specific regression scenario it is intended to cover.
## Individual Comments
### Comment 1
<location> `tests/foreman/api/test_capsulecontent.py:2239` </location>
<code_context>
+ 'Expected to see the Capsule as the content source'
+ )
+ # Unregister the contenthost, leaving a null content facet behind.
+ assert rhel_contenthost.unregister().status == 0
+ # Perform a full capsule sync
+ timestamp = datetime.now(UTC)
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that the subscription/content facet is actually null before performing the sync
The test currently assumes `unregister()` leaves a null/missing subscription/content facet without checking it. To make this a reliable regression test for SAT-39794, add an explicit assertion after `unregister()` (via the host, facet API, or DB helper) that the relevant subscription facet UUID or facet object is indeed null/missing before running the capsule sync. This ensures the test will fail if `unregister()` behavior changes and no longer reproduces the bug condition.
Suggested implementation:
```python
assert nc.id == host.content_facet_attributes['content_source_id'], (
'Expected to see the Capsule as the content source'
)
# Unregister the contenthost, leaving a null content facet behind.
# Assert that the subscription/content facet is actually null before performing the sync.
host_after_unreg = module_target_sat.api.Host(id=host.id).read()
subscription_facet = getattr(host_after_unreg, 'subscription_facet_attributes', None)
assert not subscription_facet, (
'Expected host subscription facet to be null after unregister, '
'to reproduce SAT-39794 conditions'
)
repos_collection.setup_content(function_org.id, function_lce.id, override=True)
```
If the host model in this test file uses a different attribute name for the subscription/content facet (for example, `subscription_facet` or `subscription_facet_attributes['uuid']`), adjust the `subscription_facet = ...` line and the assertion accordingly to match the existing API. If there is already a test helper for fetching/inspecting the subscription facet, you may want to use that helper instead of calling `Host(id=host.id).read()` directly to stay consistent with the rest of the test suite.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
Not a code review but the test flow looks correct and tests the error condition correctly. |
|
PRT Result |
vsedmik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I have just one question bellow
|
We did get an issue Katello/katello#11597 around N-1/N-2 capsule syncs. Not sure if we cover those in any way in robotello? |
synkd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK pending answer to most recent question
(cherry picked from commit fb45d39)
…cet (#20540) Add test case verifiying capsule sync in presence of null facet (#20471) (cherry picked from commit fb45d39) Co-authored-by: Samuel Bible <[email protected]>
Problem Statement
Customers have run into issues syncing capsules when hosts have left behind a null subscription facet.
Solution
This adds a test case covering this issue.
Related Issues
https://issues.redhat.com/browse/SAT-40496
and
https://issues.redhat.com/browse/SAT-39794
PRT test Cases example
trigger: test-robottelo
pytest: tests/foreman/api/test_capsulecontent.py -k 'test_capsule_sync_nulled_subscription_facet'