Skip to content

Conversation

@ColeHiggins2
Copy link
Member

@ColeHiggins2 ColeHiggins2 commented Dec 16, 2025

Test for disabling/enabling recommendations on IOP.
SAT-38139
SatelliteQE/airgun#2247

@ColeHiggins2 ColeHiggins2 self-assigned this Dec 16, 2025
@ColeHiggins2 ColeHiggins2 added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels Dec 16, 2025
@ColeHiggins2 ColeHiggins2 marked this pull request as draft December 16, 2025 22:52
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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 final assertion, consider using the existing OPENSSH_RECOMMENDATION constant instead of the hard-coded string 'Decreased security: OpenSSH config permissions' to avoid duplication and keep the test resilient to future text changes.
  • Since you touched the surrounding comment, this is a good place to fix the typo recommnedationsrecommendations for clarity.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In the final assertion, consider using the existing `OPENSSH_RECOMMENDATION` constant instead of the hard-coded string `'Decreased security: OpenSSH config permissions'` to avoid duplication and keep the test resilient to future text changes.
- Since you touched the surrounding comment, this is a good place to fix the typo `recommnedations``recommendations` for clarity.

## Individual Comments

### Comment 1
<location> `tests/foreman/ui/test_rhcloud_iop.py:344-349` </location>
<code_context>

-        # Verify that Disabled recommnedations are 0
+        # Disable recommendation
+        session.recommendationstab.disable_recommendation_for_system(
+            recommendation_name=OPENSSH_RECOMMENDATION, hostname=rhel_insights_vm.hostname
+        )
+        # Verify that the disabled recommendation is filtered
         result = session.recommendationstab.apply_filter("Status", "Disabled")
-        assert 'No recommendations' in result[0]['Name']
+        assert 'Decreased security: OpenSSH config permissions' in result[0]['Name']
</code_context>

<issue_to_address>
**suggestion (testing):** Make the assertion on the disabled recommendations more robust than relying on the first list element

This assertion still assumes the expected recommendation is at `result[0]`. If multiple disabled recommendations are returned or ordering changes, the test may fail or give a false sense of correctness. Instead, assert that at least one item in `result` has the expected name, e.g. `assert any('Decreased security: OpenSSH config permissions' in r['Name'] for r in result)`, and optionally assert `len(result) > 0` to document the expectation that something is returned.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 344 to 349
session.recommendationstab.disable_recommendation_for_system(
recommendation_name=OPENSSH_RECOMMENDATION, hostname=rhel_insights_vm.hostname
)
# Verify that the disabled recommendation is filtered
result = session.recommendationstab.apply_filter("Status", "Disabled")
assert 'No recommendations' in result[0]['Name']
assert 'Decreased security: OpenSSH config permissions' in result[0]['Name']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Make the assertion on the disabled recommendations more robust than relying on the first list element

This assertion still assumes the expected recommendation is at result[0]. If multiple disabled recommendations are returned or ordering changes, the test may fail or give a false sense of correctness. Instead, assert that at least one item in result has the expected name, e.g. assert any('Decreased security: OpenSSH config permissions' in r['Name'] for r in result), and optionally assert len(result) > 0 to document the expectation that something is returned.

@ColeHiggins2 ColeHiggins2 force-pushed the update-test-for-enable-disable-iop branch from d44222e to c5ff1d7 Compare December 19, 2025 16:40
@ColeHiggins2 ColeHiggins2 marked this pull request as ready for review December 19, 2025 23:05
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_rhcloud_iop.py -k "test_iop_recommendations_remediation_type_and_status"

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • The recommendation name 'Decreased security: OpenSSH config permissions' is hard-coded multiple times; consider using the existing OPENSSH_RECOMMENDATION constant (or another shared constant) to avoid duplication and keep the test resilient to name changes.
  • Before indexing result[0] after applying filters, consider asserting that result is not empty to avoid potential index errors if the UI returns no rows.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The recommendation name `'Decreased security: OpenSSH config permissions'` is hard-coded multiple times; consider using the existing `OPENSSH_RECOMMENDATION` constant (or another shared constant) to avoid duplication and keep the test resilient to name changes.
- Before indexing `result[0]` after applying filters, consider asserting that `result` is not empty to avoid potential index errors if the UI returns no rows.

## Individual Comments

### Comment 1
<location> `tests/foreman/ui/test_rhcloud_iop.py:344-345` </location>
<code_context>
-        assert 'No recommendations' in result[0]['Name']
+        assert 'Decreased security: OpenSSH config permissions' in result[0]['Name']
+
+        session.recommendationstab.enable_recommendation(
+            recommendation_name='Decreased security: OpenSSH config permissions'
+        )
+
</code_context>

<issue_to_address>
**suggestion:** Use the same recommendation identifier consistently (e.g. the `OPENSSH_RECOMMENDATION` constant) when enabling/disabling.

The disable path uses `OPENSSH_RECOMMENDATION`, but the enable path uses the raw string `'Decreased security: OpenSSH config permissions'`. If the constant changes, the test could disable one recommendation and try to enable another. Please use `OPENSSH_RECOMMENDATION` consistently (including in assertions), or derive the string from a single shared source.

Suggested implementation:

```python
        # Verify that the disabled recommendation is filtered
        result = session.recommendationstab.apply_filter("Status", "Disabled")
        assert OPENSSH_RECOMMENDATION in result[0]['Name']

```

Search the rest of `tests/foreman/ui/test_rhcloud_iop.py` for any other hard-coded instances of `'Decreased security: OpenSSH config permissions'` and replace them with `OPENSSH_RECOMMENDATION` to keep the identifier consistent everywhere (including any other assertions or enable/disable calls).
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
airgun: 2247
pytest: tests/foreman/ui/test_rhcloud_iop.py -k "test_iop_recommendations_remediation_type_and_status"

1 similar comment
@ColeHiggins2
Copy link
Member Author

trigger: test-robottelo
airgun: 2247
pytest: tests/foreman/ui/test_rhcloud_iop.py -k "test_iop_recommendations_remediation_type_and_status"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant