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

Remove most waitForTimeout usage from the scripting integration tests #17974

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

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Apr 19, 2024

This commit replaces most waitForTimeout occurrences with calls to waitForFunction or waitForSandboxTrip. Note that the occurrences in the "must check that focus/blur callbacks aren't called" test remain until we find a good way to ensure that nothing happened after the tab switches (because currently we can't be sure that nothing happens since there is nothing to await).

Fixes a part of #17656.

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@timvandermeij
Copy link
Contributor Author

The failure above is unrelated to this patch and is tracked separately in #17931 already (and will also be part of the final bits for #17656).

@Snuffleupagus
Copy link
Collaborator

(I'm deferring to @calixteman on this.)

@Snuffleupagus Snuffleupagus removed their request for review April 20, 2024 11:07
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Apr 24, 2024

@calixteman Do you perhaps have time to review this PR and #17969? Those should already give us more stable integration tests and we can then continue with the final waitForTimeout occurrences, but I'd prefer we only touch those after these PRs so they have some time to settle and we don't change too much at once. Thanks!

@timvandermeij timvandermeij force-pushed the integration-tests-timeout-scripting branch from 190123c to e0a8c40 Compare April 24, 2024 13:49
@timvandermeij timvandermeij changed the title Remove waitForTimeout usage from the scripting integration tests Remove most waitForTimeout usage from the scripting integration tests Apr 24, 2024
@timvandermeij timvandermeij force-pushed the integration-tests-timeout-scripting branch 3 times, most recently from 8d7e437 to 10121c2 Compare April 30, 2024 13:55
@timvandermeij
Copy link
Contributor Author

I have rebased the patch, updated it to use waitForSandboxTrip where possible and reverted the template string change (since it's unrelated to the other changes and it seemed better to keep this patch as small as possible). I have audited all changes again to make sure we either await something deterministic (using waitForFunction or waitForSandboxTrip) or that the waitForTimeout is really obsolete (such as in the last two tests where the actions themselves are instant and we already have other code there in place to wait for what we need).

@calixteman You can best view the changes since the last review in https://github.com/mozilla/pdf.js/compare/8d7e4378fbe70961dd7503077aab14b31644dae0..10121c2060f1f8bc9b2c896122b36221592edf22#diff-c0973b2c3b147db1eccd21c6b1af307f33a7e5d2658b4a0738a2386470b9c0ca (since the whole interdiff is a bit big due to the rebase).

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@timvandermeij timvandermeij force-pushed the integration-tests-timeout-scripting branch 3 times, most recently from 0f35367 to 7f64fae Compare May 7, 2024 13:38
This commit replaces most `waitForTimeout` occurrences with calls to
`waitForFunction` or `waitForSandboxTrip`. Note that the occurrences in
the "must check that focus/blur callbacks aren't called" test remain
until we find a good way to ensure that nothing happened after the tab
switches (because currently we can't be sure that nothing happens since
there is nothing to await).
@timvandermeij timvandermeij force-pushed the integration-tests-timeout-scripting branch from 7f64fae to 0985f7b Compare May 14, 2024 14:08
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@mozilla mozilla deleted a comment from moz-tools-bot May 14, 2024
@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/02f3f99a2dd0165/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/f52ea60ecf1beaa/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/02f3f99a2dd0165/output.txt

Total script time: 7.39 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/f52ea60ecf1beaa/output.txt

Total script time: 17.94 mins

  • Integration Tests: FAILED

@timvandermeij
Copy link
Contributor Author

Nothing changed in this PR since #17974 (comment) other than a rebase onto the current master. @calixteman Gentle review ping; do you have additional feedback here or is it good to go like this?

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

Successfully merging this pull request may close these issues.

None yet

4 participants