Fix errata contenthost - #20825
Conversation
Reviewer's GuideThis PR updates UI errata tests to stop using the removed legacy Content Host UI in favor of the new Hosts/All Hosts and Host details pages, adjusts expectations to match the new UI’s data model, and removes an obsolete test that depended on the legacy page. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_positive_apply_for_all_hosts, changing the task assertion fromlen(applicability_tasks) == num_hosts + 1to> 0significantly weakens the guarantee; consider asserting against an expected minimum/exact number of tasks derived fromnum_hoststo keep the test meaningful while still accommodating the new UI behavior. - In the updated host errata/package checks (e.g., asserting
'5.21-1.noarch'and usingsettings.repos.yum_6.errata[...]), consider centralizing these expected versions/labels in shared constants/fixtures so that UI or content changes don’t require editing multiple hard-coded strings in tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_positive_apply_for_all_hosts`, changing the task assertion from `len(applicability_tasks) == num_hosts + 1` to `> 0` significantly weakens the guarantee; consider asserting against an expected minimum/exact number of tasks derived from `num_hosts` to keep the test meaningful while still accommodating the new UI behavior.
- In the updated host errata/package checks (e.g., asserting `'5.21-1.noarch'` and using `settings.repos.yum_6.errata[...]`), consider centralizing these expected versions/labels in shared constants/fixtures so that UI or content changes don’t require editing multiple hard-coded strings in tests.
## Individual Comments
### Comment 1
<location> `tests/foreman/ui/test_errata.py:947` </location>
<code_context>
max_tries=30,
)
- assert len(applicability_tasks) == num_hosts + 1
+ assert len(applicability_tasks) > 0
# found updated kangaroo package in each host
updated_version = '0.2-1.noarch'
</code_context>
<issue_to_address>
**issue (testing):** The new assertion on `applicability_tasks` is weaker and may no longer guarantee that all expected tasks ran
Changing from `len(applicability_tasks) == num_hosts + 1` to `> 0` means the test can pass even if far fewer tasks are created than intended, weakening its ability to catch regressions. If the exact count is no longer stable, consider asserting a meaningful lower bound tied to `num_hosts` (e.g. `>= num_hosts`) or otherwise encoding the new, specific expectation so the test still verifies the intended behavior rather than just the presence of any tasks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
PRT Result |
| # updated walrus package found for each host | ||
| assert packages_rows[0]['Installed Package'] == FAKE_2_CUSTOM_PACKAGE | ||
| assert len(packages_rows) == 1 | ||
| assert packages_rows[0]['Installed version'] == '5.21-1.noarch' |
There was a problem hiding this comment.
Instead of hardcoding this assertion here, can't we use FAKE_2_CUSTOM_PACKAGE_NAME to fetch this version from FAKE_2_CUSTOM_PACKAGE?
There was a problem hiding this comment.
We can do something like
assert packages_rows[0]['Installed version'] == FAKE_2_CUSTOM_PACKAGE.split('-', 1)[1]
which would be less readable, though.
| session.location.select(loc_name=DEFAULT_LOC) | ||
| # Search for hosts needing RHSA security errata | ||
| result = session.contenthost.search('errata_status = security_needed') | ||
| result = session.host.search('errata_status = security_needed') |
There was a problem hiding this comment.
Can we use session.host_new.* here and below, instead of legacy host UI?
There was a problem hiding this comment.
session.host runs via new UI already, so host_new should work too. Let's try it.
|
|
PRT Result |
* Fix test_content_host_errata_search_commands * Fix test_positive_apply_for_all_hosts * Remove test_positive_content_host_previous_env * Fix test_positive_show_count_on_host_pages * Address comments (cherry picked from commit 3653f9b)
Problem Statement
The legacy "Content host" page has been removed as part of SAT-37927 but some errata UI tests were left behind, trying to reach removed
contenthostproperty, views and entities.Solution
Use "All hosts page" instead where possible, remove what is "Content host" page related.
Related Issues
https://issues.redhat.com/browse/SAT-42273
PRT test Cases example
Summary by Sourcery
Update errata UI tests to use the new All Hosts / Host UI instead of the removed legacy Content Host page and adjust expectations accordingly.
Bug Fixes:
Enhancements: