-
Notifications
You must be signed in to change notification settings - Fork 131
[6.18.z] Remove RHcloud report assertion #20447
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] Remove RHcloud report assertion #20447
Conversation
* Remove rhcloud report assertion * fix common assertions in rhcloud tests (cherry picked from commit 8ec8fa1)
|
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:
- The hard-coded report paths and terminal messages (e.g., under
/var/lib/foreman/red_hat_inventory/...) are duplicated and string-concatenated in several places; consider centralizing them as constants/helpers to reduce the risk of drift when those paths/messages change again. - The use of
tempfile.mkdtemp()without cleanup in the new download branch can leak temporary directories; consider usingtempfile.TemporaryDirectory()as a context manager or ensuring explicit cleanup after use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded report paths and terminal messages (e.g., under `/var/lib/foreman/red_hat_inventory/...`) are duplicated and string-concatenated in several places; consider centralizing them as constants/helpers to reduce the risk of drift when those paths/messages change again.
- The use of `tempfile.mkdtemp()` without cleanup in the new download branch can leak temporary directories; consider using `tempfile.TemporaryDirectory()` as a context manager or ensuring explicit cleanup after use.
## Individual Comments
### Comment 1
<location> `tests/foreman/ui/test_rhcloud_inventory.py:174-179` </location>
<code_context>
+ assert result.status == 0, f"Report file not found at {remote_report_path}"
+
+ # Copy report from Satellite to local temp location
+ temp_dir = tempfile.mkdtemp()
+ local_report_name = f'report_for_{org.id}.tar.xz'
+ report_path = os.path.join(temp_dir, local_report_name)
+
+ # Download the file from satellite
+ module_target_sat.get(remote_path=remote_report_path, local_path=report_path)
+
inventory_data = session.cloudinventory.read(org.name)
</code_context>
<issue_to_address>
**suggestion (testing):** Use a context manager for the temporary directory to avoid leftover files and improve test hygiene
This path uses `tempfile.mkdtemp()` without cleanup, which can leave stray directories across runs. Wrap it in `with tempfile.TemporaryDirectory() as temp_dir:` so the directory is cleaned up automatically after the test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| temp_dir = tempfile.mkdtemp() | ||
| local_report_name = f'report_for_{org.id}.tar.xz' | ||
| report_path = os.path.join(temp_dir, local_report_name) | ||
|
|
||
| # Download the file from satellite | ||
| module_target_sat.get(remote_path=remote_report_path, local_path=report_path) |
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 (testing): Use a context manager for the temporary directory to avoid leftover files and improve test hygiene
This path uses tempfile.mkdtemp() without cleanup, which can leave stray directories across runs. Wrap it in with tempfile.TemporaryDirectory() as temp_dir: so the directory is cleaned up automatically after the test.
|
PRT Result |
Cherrypick of PR: #20230
Test was failing because we updated the message output in the report logging. I propose we just remove it all together since we assert other checks in the tests themselves...