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 waitForTimeout usages and implementation #17656

Open
timvandermeij opened this issue Feb 10, 2024 · 2 comments
Open

Remove waitForTimeout usages and implementation #17656

timvandermeij opened this issue Feb 10, 2024 · 2 comments
Labels

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Feb 10, 2024

In #17655 we had to implement a waitForTimeout helper function ourselves because Puppeteer 22+ no longer provides it. Waiting for an arbitrary amount of time is discouraged because of what we already found before: it can cause intermittently failing tests if the timeout is too low, and it can add extra runtime to the tests if the timeout is too large. In puppeteer/puppeteer#11780 better alternatives are listed.

Fortunately back when we fixed the intermittent failures in the integration tests we already replaced most occurrences with waitForSelector instead, and we have seen the integration tests being very stable since, but this issue tracks removing the remaining occurrences so we can eventually get rid of the helper function altogether and further reduce any possible flakiness.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Apr 8, 2024

Would it make sense to add an test/integration/.eslintrc file with a "no-restricted-syntax" rule, see e.g. these ones, that prevents waitForTimeout without a disable-comment?

We'd obviously need to add a bunch of disable-comments all over the tests for now, but it'd be more difficult to accidentally add new waitForTimeout calls.

timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue Apr 9, 2024
… tests

The `waitForTimeout` function should not be used anymore and only exists
for old usages that have to be rewritten, but there was nothing in place
to signal this. This commit therefore implements a linting rule, specific
to the integration tests, to make it clear that this function should no
longer be used. We exclude the old usages from it because we are already
tracking those in mozilla#17656 (so this patch is mostly to not make the scope
of that issue bigger).
@timvandermeij
Copy link
Contributor Author

Would it make sense to add an test/integration/.eslintrc file with a "no-restricted-syntax" rule, see e.g. these ones, that prevents waitForTimeout without a disable-comment?

We'd obviously need to add a bunch of disable-comments all over the tests for now, but it'd be more difficult to accidentally add new waitForTimeout calls.

I think that's a great idea! I have implemented it in the PR above so we indeed make sure that the scope of this issue won't accidentally become bigger.

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

No branches or pull requests

2 participants