Make test_positive_task_status faster and more robust#21685
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMakes the dashboard task status UI test faster and less brittle by using a guaranteed-invalid hostname for the repo sync and loosening the assertion to focus on task result instead of specific backend error text. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The test still expects
tasks['table'][0]['Result'] == 'warning'but now assertsvalues['task']['result'] == 'error'; please confirm this mismatch is intentional and, if not, align the expected task result with the dashboard status to avoid flaky or confusing behavior. - Consider extracting the repository URL (
http://doesnotexist.invalid) into a local variable or constant and reusing it for both creation and any related assertions so that future URL changes don’t silently diverge across the test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test still expects `tasks['table'][0]['Result'] == 'warning'` but now asserts `values['task']['result'] == 'error'`; please confirm this mismatch is intentional and, if not, align the expected task result with the dashboard status to avoid flaky or confusing behavior.
- Consider extracting the repository URL (`http://doesnotexist.invalid`) into a local variable or constant and reusing it for both creation and any related assertions so that future URL changes don’t silently diverge across the test.
## Individual Comments
### Comment 1
<location path="tests/foreman/ui/test_dashboard.py" line_range="136-139" />
<code_context>
product = target_sat.api.Product(organization=org).create()
repo = target_sat.api.Repository(
- url=f'http://{url}', product=product, content_type='yum'
+ url='http://doesnotexist.invalid', product=product, content_type='yum'
).create()
with pytest.raises(TaskFailedError):
</code_context>
<issue_to_address>
**suggestion:** Consider documenting the rationale for using the `.invalid` TLD in this test
Because this behavior depends on RFC 2606’s guarantee that `.invalid` always returns NXDOMAIN, please add a brief comment near this line noting that this hostname is intentionally chosen to avoid DNS timeouts and resolver-dependent behavior, so future changes don’t “simplify” it back to a real domain.
```suggestion
product = target_sat.api.Product(organization=org).create()
# Use a .invalid TLD as specified in RFC 2606 to guarantee NXDOMAIN and avoid
# DNS timeouts or resolver-dependent behavior in this test.
repo = target_sat.api.Repository(
url='http://doesnotexist.invalid', product=product, content_type='yum'
).create()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
| f"500, message='Internal Server Error', url='http://{url}'", | ||
| f'Cannot connect to host {url}:80 ssl:default [Domain name not found]', | ||
| ] | ||
| assert values['task']['result'] == 'error' |
There was a problem hiding this comment.
Why the switch from warning to error? I would expect that switching one non-resolvable hostname for another shouldn't have this kind of effect
There was a problem hiding this comment.
As far as I understand .invalid domain is different in the fact that the resolver does not even try to resolve it but returns NXDOMAIN immediately. The previous domain probably caused resolver to return different error type after attempting to resolve it.
There was a problem hiding this comment.
PRT seems to agree that this is odd
> assert values['task']['result'] == 'error'
E AssertionError: assert 'warning' == 'error'
E
E - error
E + warning
tests/foreman/ui/test_dashboard.py:161: AssertionError
There was a problem hiding this comment.
In my local environment, the task result did change to error, but in PRT it remained 'warning'. I need to investigate further to see if this can be made more robust.
There was a problem hiding this comment.
It seems that the result of that assertion can be different based on environment while the assertion itself does not bring much value. I think removing the assertion completely is the best option.
|
PRT Result |
6834ae7 to
745fdfd
Compare
Switched the repository URL from 'www.non_existent_repo_url.org' to 'doesnotexist.invalid'. The .invalid TLD (RFC 2606) is guaranteed to return NXDOMAIN immediately from any DNS resolver, eliminating DNS timeout variance and thus making the task fail faster. Removed the LatestFailedTasks navigation and task details read that followed it. The TaskDetails navigator always re-navigates to the tasks list via the menu and searches for the task independently, so the call provided no coverage for whether the dashboard link itself worked correctly. The task result assertion it enabled was also deterministic. 'warning' and 'error' were achieved respectively in PRT and local environment, making it a source of flakiness rather than meaningful coverage. Made with Cursor.
745fdfd to
b964bca
Compare
|
trigger: test-robottelo |
|
PRT Result |
|
trigger: test-robottelo |
|
PRT Result |
|
Dependency merged, this is ready for re-review. |
Switched the repository URL from 'www.non_existent_repo_url.org' to
'doesnotexist.invalid'. The .invalid TLD (RFC 2606) is guaranteed to
return NXDOMAIN immediately from any DNS resolver, eliminating DNS
timeout variance and thus making the task fail faster.
Removed the LatestFailedTasks navigation and task details read that
followed it. The TaskDetails navigator always re-navigates to the tasks
list via the menu and searches for the task independently, so the call
provided no coverage for whether the dashboard link itself worked
correctly. The task result assertion it enabled was also deterministic.
'warning' and 'error' were achieved respectively in PRT and local
environment, making it a source of flakiness rather than meaningful
coverage.
Made with Cursor.
Depends on: SatelliteQE/airgun#2441
Summary by Sourcery
Improve the reliability and speed of the dashboard task status UI test when syncing an invalid repository.
Bug Fixes: