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.18.z Introduced in or relating directly to Satellite 6.18 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 log check relies on a fixed time.sleep(20) before reading the shell output, which may introduce flakiness across slower/faster environments; consider using wait_for or another condition-based wait on the HTTP request line instead of a hard-coded delay.
  • The sequence shell.close() followed by shell.read() may not be safe depending on the shell API semantics; consider reading the buffer before closing, or explicitly confirming that read() on a closed shell is supported and deterministic.
  • The assertions previously wrapped timeout handling and produced a clear error when the log line was not found; with the new direct assert ... in str(shell.read()) you lose that context, so consider adding a more descriptive assertion message or explicit handling when the expected HTTP request is missing.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new log check relies on a fixed `time.sleep(20)` before reading the shell output, which may introduce flakiness across slower/faster environments; consider using `wait_for` or another condition-based wait on the HTTP request line instead of a hard-coded delay.
- The sequence `shell.close()` followed by `shell.read()` may not be safe depending on the shell API semantics; consider reading the buffer before closing, or explicitly confirming that `read()` on a closed shell is supported and deterministic.
- The assertions previously wrapped timeout handling and produced a clear error when the log line was not found; with the new direct `assert ... in str(shell.read())` you lose that context, so consider adding a more descriptive assertion message or explicit handling when the expected HTTP request is missing.

## Individual Comments

### Comment 1
<location> `tests/foreman/api/test_provisioning.py:393-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):** Fixed `time.sleep` with a single assertion makes this e2e test brittle and timing-sensitive

A fixed `time.sleep(20)` followed by a single `assert ... in shell.read()` introduces a race: if the log line appears just after 20s, the test fails even though behavior is correct. Instead, wrap the log check in a `wait_for`-style loop that repeatedly reads the shell buffer until the substring appears or a timeout is reached, similar to the previous `assert_host_logs` behavior. This will make the test more robust and less timing-dependent.

```suggestion
    shell = sat.session.shell()
    shell.send('foreman-tail')

    def _httpboot_log_present():
        """
        Poll the remote foreman-tail output until the expected HTTPBoot request
        shows up in the logs or the wait_for timeout is reached.
        """
        return (
            f'GET /httpboot/host-config/{host_mac_addr}/grub2/boot.efi with 200'
            in str(shell.read())
        )

    # Wait for the provisioning HTTPBoot request to appear in the logs instead
    # of relying on a fixed sleep, to avoid timing-sensitive flakes.
    wait_for(_httpboot_log_present, timeout=60, delay=5)

    shell.close()
```
</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: 13617
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, 21 warnings, 4 errors in 1509.10s (0:25:09) ==========

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label Nov 27, 2025
@Gauravtalreja1
Copy link
Member

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

@Satellite-QE
Copy link
Collaborator Author

PRT Result

Build Number: 13783
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/api/test_provisioning.py -k test_rhel_httpboot_provisioning --external-logging --include-onprem-provisioning
Test Result : ==== 1 failed, 3 passed, 33 deselected, 514 warnings in 8981.69s (2:29:41) =====

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

Labels

6.18.z Introduced in or relating directly to Satellite 6.18 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.

4 participants