-
Notifications
You must be signed in to change notification settings - Fork 131
[6.18.z] Add test case verifiying capsule sync in presence of null facet #20540
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
[6.18.z] Add test case verifiying capsule sync in presence of null facet #20540
Conversation
(cherry picked from commit fb45d39)
|
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:
- In the comment
# Sync a yum repo, publish and promote it to a CVE, sync the Capsule and wait for it., it looks likeCVEshould beCV(content view), which will make the test intent clearer. - The second assignment to
timestamp = datetime.now(UTC)just beforenailgun_capsule = module_capsule_configured.nailgun_capsuleis unused and can be removed to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the comment `# Sync a yum repo, publish and promote it to a CVE, sync the Capsule and wait for it.`, it looks like `CVE` should be `CV` (content view), which will make the test intent clearer.
- The second assignment to `timestamp = datetime.now(UTC)` just before `nailgun_capsule = module_capsule_configured.nailgun_capsule` is unused and can be removed to avoid confusion.
## Individual Comments
### Comment 1
<location> `tests/foreman/api/test_capsulecontent.py:2236-2237` </location>
<code_context>
+ 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 rhel_contenthost.unregister().status == 0
+ # Perform a full capsule sync
+ timestamp = datetime.now(UTC)
</code_context>
<issue_to_address>
**issue (testing):** The test does not explicitly verify that the subscription/content facet is actually null before triggering the capsule sync.
The test currently assumes unregister leaves a null subscription/content facet but never verifies it. To better cover SAT-39794/SAT-40496, please re-fetch the host after `unregister`, and add an assertion that the subscription/content facet (or the specific UUID field involved in the bug) is null before calling `content_sync`. This will make the test a more reliable regression guard for unregister behavior.
</issue_to_address>
### Comment 2
<location> `tests/foreman/api/test_capsulecontent.py:2230-2233` </location>
<code_context>
+ activation_keys=[repos_collection.setup_content_data['activation_key']['name']],
+ )
+ assert result.status == 0, f'Failed to register host: {result.stderr}'
+ host = module_target_sat.api.Host().search(
+ query={'search': f'name="{rhel_contenthost.hostname}"'}
+ )[0]
+ assert nc.id == host.content_facet_attributes['content_source_id'], (
+ 'Expected to see the Capsule as the content source'
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** The test does not assert that the host remains present after unregistering, which is essential to the described scenario.
Since the scenario specifies the host is unregistered "without deleting it" and leaves a stale/null facet, the test should verify the host still exists after `unregister()`. For example, re-run `Host().search` and assert the host is present. Otherwise, a future change that deletes the host on unregister could still pass this test while no longer exercising the intended buggy path.
Suggested implementation:
```python
rhel_contenthost.unregister()
# Verify the host remains present after unregister; the scenario expects only a stale/null facet
hosts_after_unregister = module_target_sat.api.Host().search(
query={'search': f'name="{rhel_contenthost.hostname}"'}
)
assert hosts_after_unregister, 'Host is expected to remain in Foreman after unregister()'
```
If the exact `unregister` call differs (e.g. `rhel_contenthost.api_unregister(...)` or includes arguments), update the `SEARCH` fragment to match that line precisely.
If the test already stores the host object from a previous `Host().search` call and relies on it later, you can optionally reuse that `host` variable instead of performing a second search, but the important part is that the test explicitly checks the host still exists after unregister.
</issue_to_address>
### Comment 3
<location> `tests/foreman/api/test_capsulecontent.py:2238-2241` </location>
<code_context>
+ )
+ # Unregister the contenthost, leaving a null content facet behind.
+ assert rhel_contenthost.unregister().status == 0
+ # Perform a full capsule sync
+ timestamp = datetime.now(UTC)
+ nailgun_capsule = module_capsule_configured.nailgun_capsule
+ sync_status = nailgun_capsule.content_sync(timeout='90m')
+ assert sync_status['result'] == 'success'
+
</code_context>
<issue_to_address>
**nitpick:** The timestamp set before the final `content_sync` call is unused and can be removed for clarity.
You assign `timestamp = datetime.now(UTC)` here but never use it (unlike the earlier `wait_for_sync` case). Please either remove this variable or use it in a `wait_for_sync`-style check if that was the original intent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Unregister the contenthost, leaving a null content facet behind. | ||
| assert rhel_contenthost.unregister().status == 0 |
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.
issue (testing): The test does not explicitly verify that the subscription/content facet is actually null before triggering the capsule sync.
The test currently assumes unregister leaves a null subscription/content facet but never verifies it. To better cover SAT-39794/SAT-40496, please re-fetch the host after unregister, and add an assertion that the subscription/content facet (or the specific UUID field involved in the bug) is null before calling content_sync. This will make the test a more reliable regression guard for unregister behavior.
|
PRT Result |
|
@sambible can we merge? |
Cherrypick of PR: #20471
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'