-
Notifications
You must be signed in to change notification settings - Fork 55
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
https://github.com/jakartaee/faces/issues/1765 #1767
Conversation
Add missing waitForBackgroundJavaScript calls
@@ -101,6 +100,7 @@ private void testSingleSelection(String form) throws Exception { | |||
input.setValueAttribute(file.getAbsolutePath()); | |||
|
|||
page = page.getHtmlElementById(form + ":submit").click(); | |||
webClient.waitForBackgroundJavaScript(3000); |
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'm not sure if the waitForBackgroundJavaScript
method is smart enough to not wait if there are no background javascript functions. If it isn't then I would suggest introducing a boolean flag that will allow us to dynamically wait depending on the test case.
Otherwise, we are waiting for no reason.
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'll take your word for it and approve the PR if you think waitForBackgroundJavaScript
is smart enough.
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. Moreover, I already ran it locally. And it also exists in a bunch of other tests. Note that it also waits for any non-ajax scripts to load completely, e.g. onload/deferred/async scripts.
Thanks for getting this fix put in. |
You're explicitly requested as reviewer. |
Fixes #1765 Add missing waitForBackgroundJavaScript calls