Skip to content
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

waitForJobs: don't wait for setInterval() jobs. #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexgo91
Copy link

@alexgo91 alexgo91 commented Jan 3, 2020

In JavaScriptJobManagerImpl, method waitForJobs() waits for all kinds of
jobs, including never-ending jobs created by setInterval(). The caller
wouldn't expect this method to block indefinitely and it causes some
scenarios to get stuck.
Also, for correctness - synchronized block should wrap final job count
check as well.

In JavaScriptJobManagerImpl, method waitForJobs() waits for all kinds of
jobs, including never-ending jobs created by setInterval(). The caller
wouldn't expect this method to block indefinitely and it causes some
scenarios to get stuck.
Also, for correctness - synchronized block should wrap final job count
check as well.
@rbri
Copy link
Member

rbri commented Jan 4, 2020

What about using waitForJobsStartingBefore(final long delayMillis, final JavaScriptJobFilter filter)?

@alexgo91
Copy link
Author

alexgo91 commented Jan 6, 2020

I am thinking about the user of HtmlUnit - it will be more simpler for him to say "wait for all tasks", instead of defining filter + delay (which is not always clear as it depends on the performance of the website).
The wait command is mostly used as part of a larger test scenario, and therefore the user just wants the website to finish loading and become ready, including all tasks. It's easier to call waitForJobs - and there shouldn't be a scenario in which a user expects this call to block forever (like happens in case of setInterval() jobs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants