Skip to content

Conversation

@Satellite-QE
Copy link
Collaborator

Cherrypick of PR: #19906

Fix httpboot provisioning test failing due to BoxKeyError

Signed-off-by: Shubham Ganar <[email protected]>
(cherry picked from commit d32fa9b)
@Satellite-QE Satellite-QE added 6.17.z Auto_Cherry_Picked Automatically cherrypicked PR using GHA No-CherryPick PR doesnt need CherryPick to previous branches labels Nov 27, 2025
@Satellite-QE
Copy link
Collaborator Author

trigger: test-robottelo
pytest: tests/foreman/api/test_provisioning.py -k test_rhel_httpboot_provisioning
provisioning: true

@Satellite-QE Satellite-QE added the AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing label Nov 27, 2025
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:

  • The new approach of time.sleep(20) followed by shell.close() and then shell.read() is prone to race conditions and may drop buffered output; consider using wait_for or a polling loop to read from the shell until the expected log line appears (or a timeout), and avoid calling close() before reading.
  • The new hard-coded expectation GET /httpboot/host-config/{host_mac_addr}/grub2/boot.efi with 200 assumes a specific log format and path; if this path is derived from configuration elsewhere, it would be safer to construct the expected string from those settings instead of embedding it directly.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new approach of `time.sleep(20)` followed by `shell.close()` and then `shell.read()` is prone to race conditions and may drop buffered output; consider using `wait_for` or a polling loop to read from the shell until the expected log line appears (or a timeout), and avoid calling `close()` before reading.
- The new hard-coded expectation `GET /httpboot/host-config/{host_mac_addr}/grub2/boot.efi with 200` assumes a specific log format and path; if this path is derived from configuration elsewhere, it would be safer to construct the expected string from those settings instead of embedding it directly.

## Individual Comments

### Comment 1
<location> `tests/foreman/api/test_provisioning.py:395-397` </location>
<code_context>
+    host_mac_addr = host_mac_addr.replace(":", "-")
+    shell = sat.session.shell()
     shell.send('foreman-tail')
+    time.sleep(20)
     shell.close()
-    assert_host_logs(shell, f'GET /httpboot/grub2/grub.cfg-{host_mac_addr} with 200')
+    assert f'GET /httpboot/host-config/{host_mac_addr}/grub2/boot.efi with 200' in str(shell.read())
+
     # Host should do call back to the Satellite reporting
</code_context>

<issue_to_address>
**suggestion (testing):** Replace fixed sleep with a `wait_for`-style polling to avoid flakiness in detecting the HTTPBoot request.

Previously we used `wait_for` with a 300s timeout to reliably wait for the log entry; this change switches to a single `time.sleep(20)` followed by one read/assert. That’s likely to be flaky on slower or loaded systems. Please wrap the log check in `wait_for` (or an equivalent polling loop) with a reasonable timeout/interval, repeatedly reading until the `GET /httpboot/host-config/{host_mac_addr}/grub2/boot.efi with 200` line appears or the timeout expires, so the test remains stable while still failing if the request never occurs.
</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.

@Satellite-QE
Copy link
Collaborator Author

PRT Result

Build Number: 13618
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/api/test_provisioning.py -k test_rhel_httpboot_provisioning --external-logging --include-onprem-provisioning
Test Result : ========== 33 deselected, 22 warnings, 4 errors in 1584.48s (0:26:24) ==========

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.17.z Auto_Cherry_Picked Automatically cherrypicked PR using GHA AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing No-CherryPick PR doesnt need CherryPick to previous branches PRT-Failed Indicates that latest PRT run is failed for the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants