improved rex UI tests reliability#21742
Open
rmynar wants to merge 1 commit into
Open
Conversation
rmynar
added
CherryPick
Contributor
Reviewer's GuideImproves reliability and robustness of Remote Execution (REX) UI tests by adding wait/retry around host searches and by rewriting the scheduled-job-by-IP test to poll and validate realistic job state transitions instead of relying on fixed sleeps and narrow status expectations. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
trigger: test-robottelo |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The host search with
wait_foris now duplicated in several tests; consider extracting a small helper (e.g.wait_for_host(session, hostname)) to remove repetition and keep search behavior consistent across tests. - The manual polling loop for job state transitions in
test_positive_run_scheduled_job_template_by_ipreimplements timeout/loop logic; consider expressing this with one or morewait_forcalls so the timing, retry behavior, and failure messages are consistent with the rest of the suite.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The host search with `wait_for` is now duplicated in several tests; consider extracting a small helper (e.g. `wait_for_host(session, hostname)`) to remove repetition and keep search behavior consistent across tests.
- The manual polling loop for job state transitions in `test_positive_run_scheduled_job_template_by_ip` reimplements timeout/loop logic; consider expressing this with one or more `wait_for` calls so the timing, retry behavior, and failure messages are consistent with the rest of the suite.
## Individual Comments
### Comment 1
<location path="tests/foreman/ui/test_remoteexecution.py" line_range="133-139" />
<code_context>
session.organization.select(module_org.name)
- assert session.host.search(hostname)[0]['Name'] == hostname
+ # Search with retry to handle potential context/indexing delays
+ result = wait_for(
+ lambda: session.host.search(hostname),
+ timeout=30,
+ delay=2,
+ fail_condition=[],
+ ).out
+ assert result[0]['Name'] == hostname
command = 'ls'
session.jobinvocation.run(
</code_context>
<issue_to_address>
**suggestion:** Consider extracting the retrying host search into a shared helper to avoid duplication and keep behavior consistent across tests.
This retrying `session.host.search` logic now appears in several tests (`test_positive_run_default_job_template`, `test_positive_run_custom_job_template`, `test_positive_run_job_template_multiple_hosts`, `test_positive_run_scheduled_job_template_by_ip`). Please consider a small helper (e.g. `search_host_with_retry(session, hostname)`) that wraps `wait_for` with the shared timeout/delay/fail_condition so tests share the same semantics and future changes only need to be made in one place.
Suggested implementation:
```python
with target_sat.ui_session() as session:
session.organization.select(module_org.name)
result = search_host_with_retry(session, hostname)
assert result[0]['Name'] == hostname
command = 'ls'
session.jobinvocation.run(
{
```
```python
job_template_name = gen_string('alpha')
with target_sat.ui_session() as session:
session.organization.select(module_org.name)
session.location.select(default_location.name)
result = search_host_with_retry(session, hostname)
```
To fully implement the helper and remove duplication across the file, also make these changes:
1. **Add the shared helper** near the top of `tests/foreman/ui/test_remoteexecution.py`, at module scope (outside any test functions), after the existing imports and any module-level constants:
```python
def search_host_with_retry(session, hostname):
"""Search host with retry to handle potential context/indexing delays."""
return wait_for(
lambda: session.host.search(hostname),
timeout=30,
delay=2,
fail_condition=[],
).out
```
`wait_for` is already imported in this file (since the current code uses it), so no new imports should be necessary.
2. **Update all other duplicated usages** in the same file (e.g. in `test_positive_run_default_job_template`, `test_positive_run_custom_job_template`, `test_positive_run_job_template_multiple_hosts`, `test_positive_run_scheduled_job_template_by_ip`) by replacing the inline `wait_for(...).out` blocks that wrap `session.host.search(...)` with:
```python
result = search_host_with_retry(session, hostname)
```
(or the appropriate variable such as `hostname_1`, `hostname_2`, etc.).
Once these changes are applied, all tests will share the same retry semantics and future adjustments to timeout/delay/fail_condition can be made in a single place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+133
to
+139
| result = wait_for( | ||
| lambda: session.host.search(hostname), | ||
| timeout=30, | ||
| delay=2, | ||
| fail_condition=[], | ||
| ).out | ||
| assert result[0]['Name'] == hostname |
Contributor
There was a problem hiding this comment.
suggestion: Consider extracting the retrying host search into a shared helper to avoid duplication and keep behavior consistent across tests.
This retrying session.host.search logic now appears in several tests (test_positive_run_default_job_template, test_positive_run_custom_job_template, test_positive_run_job_template_multiple_hosts, test_positive_run_scheduled_job_template_by_ip). Please consider a small helper (e.g. search_host_with_retry(session, hostname)) that wraps wait_for with the shared timeout/delay/fail_condition so tests share the same semantics and future changes only need to be made in one place.
Suggested implementation:
with target_sat.ui_session() as session:
session.organization.select(module_org.name)
result = search_host_with_retry(session, hostname)
assert result[0]['Name'] == hostname
command = 'ls'
session.jobinvocation.run(
{ job_template_name = gen_string('alpha')
with target_sat.ui_session() as session:
session.organization.select(module_org.name)
session.location.select(default_location.name)
result = search_host_with_retry(session, hostname)To fully implement the helper and remove duplication across the file, also make these changes:
- Add the shared helper near the top of
tests/foreman/ui/test_remoteexecution.py, at module scope (outside any test functions), after the existing imports and any module-level constants:
def search_host_with_retry(session, hostname):
"""Search host with retry to handle potential context/indexing delays."""
return wait_for(
lambda: session.host.search(hostname),
timeout=30,
delay=2,
fail_condition=[],
).outwait_for is already imported in this file (since the current code uses it), so no new imports should be necessary.
- Update all other duplicated usages in the same file (e.g. in
test_positive_run_default_job_template,test_positive_run_custom_job_template,test_positive_run_job_template_multiple_hosts,test_positive_run_scheduled_job_template_by_ip) by replacing the inlinewait_for(...).outblocks that wrapsession.host.search(...)with:
result = search_host_with_retry(session, hostname)(or the appropriate variable such as hostname_1, hostname_2, etc.).
Once these changes are applied, all tests will share the same retry semantics and future adjustments to timeout/delay/fail_condition can be made in a single place.
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
Collaborator
|
PRT Result |
rmynar
force-pushed
the
sat-41520
branch
2 times, most recently
from
a89f97f to
83f9ec1
Compare
Contributor
Author
|
trigger: test-robottelo |
Collaborator
|
PRT Result |
Contributor
Author
|
trigger: test-robottelo |
Collaborator
|
PRT Result |
Satellite-QE
added
PRT-Passed
LadislavVasina1
previously approved these changes
Jul 1, 2026
Satellite-QE
removed
the
PRT-Passed
synkd
approved these changes
Jul 7, 2026
Contributor
Author
|
trigger: test-robottelo |
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
LadislavVasina1
dismissed
their stale review
Want to check related airgun implementation as it has new changes.
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
PRT-Failed
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
PRT-Passed
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
removed
the
PRT-Passed
Contributor
Author
|
Contributor
|
Member
|
trigger: test-robottelo |
1 similar comment
Member
|
trigger: test-robottelo |
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
Contributor
|
Collaborator
|
PRT Result |
Contributor
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Together with changes to airgun SatelliteQE/airgun#2454 this improves reliability of REX UI tests.
Summary by Sourcery
Improve reliability and robustness of remote execution (REX) UI tests by adding resilient host lookup and more deterministic job scheduling assertions.
Bug Fixes:
Tests: