-
Notifications
You must be signed in to change notification settings - Fork 127
Fix for 2 CV tests #19987
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: master
Are you sure you want to change the base?
Fix for 2 CV tests #19987
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:
- Use a helper to select the latest content view version by version number instead of relying on a hardcoded index for more robust tests.
- Add a comment in the test to note that the API now returns versions in reverse chronological order so future maintainers understand the index change.
- Consider refactoring the version selection logic into a shared utility to avoid duplication across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use a helper to select the latest content view version by version number instead of relying on a hardcoded index for more robust tests.
- Add a comment in the test to note that the API now returns versions in reverse chronological order so future maintainers understand the index change.
- Consider refactoring the version selection logic into a shared utility to avoid duplication across tests.
## Individual Comments
### Comment 1
<location> `tests/foreman/cli/test_contentview.py:1482` </location>
<code_context>
)
module_target_sat.cli.ContentView.publish({'id': new_cv['id']})
new_cv_version_2 = module_target_sat.cli.ContentView.info({'id': new_cv['id']})['versions'][
- 1
+ 0
]
module_streams = module_target_sat.cli.ModuleStream.list(
</code_context>
<issue_to_address>
**nitpick:** Add a comment or assertion explaining the index change for future maintainers.
Clarifying the use of index 0 will reduce confusion and help avoid mistakes if the API's version ordering changes in the future.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
trigger: test-robottelo |
PRT Result
|
new_cv_version_2 = module_target_sat.cli.ContentView.info({'id': new_cv['id']})['versions'][ | ||
1 | ||
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.
maybe we could just check the version is included in versions instead of aiming at a specific index -- that would keep working even if the order gets reversed again
target_sat.api.ContentViewFilterRule(content_view_filter=cv_filter, errata=errata).create() | ||
|
||
content_view.publish() | ||
content_view_version_info = content_view.read().version[1].read() |
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.
if version data allows it, would it be possible to sort the versions to get the latest one? this way we wouldn't need to assume the return order
Problem Statement
These 2 CV tests have been failing for a while, and we need to get to 100% pass rate this quarter.
Solution
I'm assuming sometime inbetween when these tests were written, and now, new versions of content views are returned in reverse order of creation, instead of chronological order, ie: version 1 -> cv_version[0], version 2 -> cv_version[0], as well
trigger: test-robottelo
pytest: tests/foreman/api/test_contentviewfilter.py::test_positive_include_exclude_module_stream_filter, tests/foreman/cli/test_contentview.py::test_positive_publish_custom_content_module_stream