-
Notifications
You must be signed in to change notification settings - Fork 208
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
t: Fix sporadic stale element failures in t/ui/18-tests-details.t #6040
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6040 +/- ##
=======================================
Coverage 98.98% 98.98%
=======================================
Files 395 395
Lines 39380 39382 +2
=======================================
+ Hits 38980 38982 +2
Misses 400 400 ☔ View full report in Codecov by Sentry. |
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.
I suppose the expectation was that my $candidates_menu = wait_for_element(selector => '#candidatesMenu', is_displayed => 1) or return {};
will wait until the ajax call is done because only then the element will be there. So there's no need to explicitly wait for the ajax call.
If this PR makes a difference that means the element might appear before the ajax call has finished and when the ajax call finishes the element might be replaced again. I suppose that's a valid theory so this PR might actually fix the problem.
It passed 100 times in a row now.
And could you reproduce the issue before (in the environment where it now passes 100 times)?
t/ui/18-tests-details.t
Outdated
unless ($candidates_menu->is_enabled) { | ||
enable_timeout; | ||
return {}; | ||
} |
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.
Good catch.
I don't think disabling the timeout has any effect on is_enabled
here. So we can probably just move the early return before disable_timeout
.
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.
Yes, I could reproduce it before, usually after less than 20 runs.
Then I added the enable_timeout
thing before the return, and from then on it started to fail even more often, inside of the wait_for_element
, which is when I started looking for other places where we call a module step and saw that we always do a wait_for_ajax
there.
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.
I changed it as you suggested and moved the disable_timeout
below
Issue: https://progress.opensuse.org/issues/169096 Calling a URL with a module step makes an ajax call, so we need wait_for_ajax here.
5786740
to
c53c994
Compare
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.
Looks good, i also just ran it 100 times without a failure 👍 Thank you!
Issue: https://progress.opensuse.org/issues/169096
Calling a URL with a module step makes an ajax call, so we need
wait_for_ajax
here.It passed 100 times in a row now.
Maybe @r-richardson can try it out too.