-
Notifications
You must be signed in to change notification settings - Fork 131
[6.17.z] Fix VMware UI e2e test #20359
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.17.z
Are you sure you want to change the base?
[6.17.z] Fix VMware UI e2e test #20359
Conversation
Signed-off-by: Shubham Ganar <[email protected]> (cherry picked from commit 3ea1169)
|
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:
- In the
wait_forpredicate you now callsession.host_new.get_host_statuses(host_fqdn)twice; consider fetching the statuses once per iteration and reusing the dict for both Build and Execution checks to avoid redundant UI/API calls. - The new wait condition only ensures
Executionis not'N/A', but the later assertion expects'Last execution succeeded'; to reduce flakiness it may be safer to wait explicitly for a terminal/expected execution status (e.g.'Last execution succeeded'or other terminal states) before asserting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `wait_for` predicate you now call `session.host_new.get_host_statuses(host_fqdn)` twice; consider fetching the statuses once per iteration and reusing the dict for both Build and Execution checks to avoid redundant UI/API calls.
- The new wait condition only ensures `Execution` is not `'N/A'`, but the later assertion expects `'Last execution succeeded'`; to reduce flakiness it may be safer to wait explicitly for a terminal/expected execution status (e.g. `'Last execution succeeded'` or other terminal states) before asserting.
## Individual Comments
### Comment 1
<location> `tests/foreman/ui/test_computeresource_vmware.py:674-676` </location>
<code_context>
request.addfinalizer(lambda: target_sat.provisioning_cleanup(host_fqdn))
wait_for(
lambda: session.host_new.get_host_statuses(host_fqdn)['Build']['Status']
- != 'Pending installation',
+ != 'Pending installation'
+ and session.host_new.get_host_statuses(host_fqdn)['Execution']['Status'] != 'N/A',
timeout=1800,
delay=30,
</code_context>
<issue_to_address>
**issue (testing):** The new wait condition may still allow `Execution` to be in a transient state like `running` or `scheduled`, so the flakiness risk remains.
Right now the condition only ensures `Execution` is not `'N/A'`, which likely just means "no job yet" and doesn’t guarantee the job has finished. The wait can still exit while `Execution` is `running` or `scheduled`, so the subsequent `assert values['Execution']['Status'] == 'Last execution succeeded'` remains flaky.
Can we instead wait for a terminal `Execution` state (e.g. `Last execution succeeded/cancelled/failed`) along with `Build != 'Pending installation'`, so the test clearly waits for job completion before asserting the final status?
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| lambda: session.host_new.get_host_statuses(host_fqdn)['Build']['Status'] | ||
| != 'Pending installation', | ||
| != 'Pending installation' | ||
| and session.host_new.get_host_statuses(host_fqdn)['Execution']['Status'] != 'N/A', |
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.
issue (testing): The new wait condition may still allow Execution to be in a transient state like running or scheduled, so the flakiness risk remains.
Right now the condition only ensures Execution is not 'N/A', which likely just means "no job yet" and doesn’t guarantee the job has finished. The wait can still exit while Execution is running or scheduled, so the subsequent assert values['Execution']['Status'] == 'Last execution succeeded' remains flaky.
Can we instead wait for a terminal Execution state (e.g. Last execution succeeded/cancelled/failed) along with Build != 'Pending installation', so the test clearly waits for job completion before asserting the final status?
|
PRT Result |
|
trigger: test-robottelo |
|
PRT Result |
Cherrypick of PR: #20217
Problem Statement
VMware UI end to end test is failing while asserting the job execution status which at time is either running or scheduled.
Solution
Add wait_for or sleep and wait till the job is completed.