-
Notifications
You must be signed in to change notification settings - Fork 127
[6.18.z] [RFE][IOP] Add tests for iop recommendations e2e #19988
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
base: 6.18.z
Are you sure you want to change the base?
[6.18.z] [RFE][IOP] Add tests for iop recommendations e2e #19988
Conversation
* Add test for iop recommendations e2e * add iop test module and move tests * update fixture * add test for host details iop * update fixture to module_satellite_iop * update docstring and function name * Update recommendations method and address comments * update fixture testing * run pre-commit * update fixtures to use target_sat_insights * fix test for prt * pre-commit * add functionality for teardown and e2e test * update syntax * remove fixutre (cherry picked from commit a2f8aaf)
trigger: test-robottelo |
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:
- Extract repeated UI session setup and recommendation remediation steps into shared fixtures or helper functions to reduce duplication across tests.
- Avoid brittle long string matching for the 'no recommendations' state by adding a dedicated method or using a more reliable UI indicator instead of raw text assertions.
- Use explicit numerical comparisons (e.g. assert status['Succeeded'] > 0) when checking job success for clearer intent in assertions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract repeated UI session setup and recommendation remediation steps into shared fixtures or helper functions to reduce duplication across tests.
- Avoid brittle long string matching for the 'no recommendations' state by adding a dedicated method or using a more reliable UI indicator instead of raw text assertions.
- Use explicit numerical comparisons (e.g. assert status['Succeeded'] > 0) when checking job success for clearer intent in assertions.
## Individual Comments
### Comment 1
<location> `tests/foreman/ui/test_rhcloud_iop.py:28` </location>
<code_context>
[email protected]_client
[email protected]_containers
[email protected]_ver_match('N-1')
[email protected]('module_target_sat_insights', [False], ids=['local'], indirect=True)
+def test_iop_recommendations_e2e(
+ rhel_insights_vm,
+ rhcloud_manifest_org,
</code_context>
<issue_to_address>
**suggestion (testing):** Parametrization is used, but only with a single value.
Expanding the parametrization to include more values for 'module_target_sat_insights' would help test additional deployment scenarios and improve test coverage.
```suggestion
@pytest.mark.parametrize(
'module_target_sat_insights',
[False, True],
ids=['local', 'satellite'],
indirect=True
)
```
</issue_to_address>
### Comment 2
<location> `tests/foreman/ui/test_rhcloud_iop.py:18-20` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 3
<location> `tests/foreman/ui/test_rhcloud_iop.py:145-146` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 4
<location> `tests/foreman/ui/test_rhcloud_iop.py:247-249` </location>
<code_context>
@pytest.mark.e2e
@pytest.mark.pit_server
@pytest.mark.pit_client
@pytest.mark.no_containers
@pytest.mark.rhel_ver_match('N-1')
@pytest.mark.parametrize('module_target_sat_insights', [False], ids=['local'], indirect=True)
def test_iop_recommendations_host_details_e2e(
rhel_insights_vm,
rhcloud_manifest_org,
module_target_sat_insights,
):
"""Set up Satellite with iop enabled, create condition on the host that violates advisor rules,
see the recommendation on the host details page, and apply the remediation.
:id: 282a7ef0-33a4-4dd4-8712-064a30cb54c6
:steps:
1. Set up Satellite with iop enabled.
2. In Satellite UI, go to All Hosts > Hostname > Recommendations.
3. Run remediation for "OpenSSH config permissions" recommendation against host.
4. Verify that the remediation job completed successfully.
5. Search for previously remediated issue.
:expectedresults:
1. Host recommendation related to "OpenSSH config permissions" issue is listed
for misconfigured machine.
2. Remediation job finished successfully.
3. Host recommendation related to "OpenSSH config permissions" issue is not listed.
:CaseImportance: Critical
:Verifies: SAT-32566
:parametrized: yes
:CaseAutomation: Automated
"""
org_name = rhcloud_manifest_org.name
# Verify insights-client package is installed
assert rhel_insights_vm.execute('insights-client --version').status == 0
# Prepare misconfigured machine and upload data to Insights
create_insights_recommendation(rhel_insights_vm)
with module_target_sat_insights.ui_session() as session:
session.organization.select(org_name=org_name)
# Verify that we can see the rule hit via insights-client
result = rhel_insights_vm.execute('insights-client --diagnosis')
assert result.status == 0
assert 'OPENSSH_HARDENING_CONFIG_PERMS' in result.stdout
result = session.host_new.get_recommendations(rhel_insights_vm.hostname)
assert any(row.get('Description') == OPENSSH_RECOMMENDATION for row in result), (
f"No row found with Recommendation == {OPENSSH_RECOMMENDATION}"
)
# Remediate the Affected System.
result = session.host_new.remediate_host_recommendation(
rhel_insights_vm.hostname,
OPENSSH_RECOMMENDATION,
)
# Verify that the job Succeeded
assert result['status']['Succeeded'] != 0
assert result['overall_status']['is_success']
# Verify that the recommendation is not listed anymore.
result = session.host_new.get_recommendations(rhel_insights_vm.hostname)
assert not any(row.get('Description') == OPENSSH_RECOMMENDATION for row in result), (
f"Recommendation found: {OPENSSH_RECOMMENDATION}"
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
assert all(
row.get('Description') != OPENSSH_RECOMMENDATION for row in result
), f"Recommendation found: {OPENSSH_RECOMMENDATION}"
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
assert not any(row.get('Description') == OPENSSH_RECOMMENDATION for row in result), ( | ||
f"Recommendation found: {OPENSSH_RECOMMENDATION}" | ||
) |
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.
suggestion (code-quality): Invert any/all to simplify comparisons (invert-any-all
)
assert not any(row.get('Description') == OPENSSH_RECOMMENDATION for row in result), ( | |
f"Recommendation found: {OPENSSH_RECOMMENDATION}" | |
) | |
assert all( | |
row.get('Description') != OPENSSH_RECOMMENDATION for row in result | |
), f"Recommendation found: {OPENSSH_RECOMMENDATION}" |
PRT Result
|
Cherrypick of PR: #19654
iop recommendations end to end tests
SatelliteQE/airgun#1983