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

fix: handler delay not being respected #49

Merged
merged 2 commits into from Dec 14, 2022

Conversation

valendres
Copy link
Owner

@SetupCoding this PR should address #45

@valendres valendres linked an issue Dec 13, 2022 that may be closed by this pull request
await page.goto('/users');

// wait a little bit just to make sure DOM has had time to update
await page.waitForTimeout(10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe one of the waitUntil option on page.goto would suffice?

https://playwright.dev/docs/api/class-page#page-goto

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I didn't know about this. I've updated the test to use it :)

@SetupCoding
Copy link
Contributor

Very nice! Thank you! I will try it out tomorrow! :)

@SetupCoding
Copy link
Contributor

Just tested it in my codebase :) Works just as expected :D Thank you!

@valendres
Copy link
Owner Author

Awesome, thanks @SetupCoding. I'll get this merged and released shortly!

@valendres valendres merged commit 3d08816 into main Dec 14, 2022
@valendres valendres deleted the fix/delay-not-being-respected branch December 14, 2022 16:33
@gejgalis
Copy link

gejgalis commented Feb 2, 2023

Awesome, thanks @SetupCoding. I'll get this merged and released shortly!

Hi @valendres

How can we help you to release this fix to NPM repo? :)

@valendres
Copy link
Owner Author

valendres commented Feb 2, 2023

My apologies for the delay @gejgalis (and @SetupCoding). A simple nudge should be sufficient.

In all honesty, the reason why this has not been released is because I identified that the tests for this feature were flaky shortly after mentioning it, but did not get around to identifying why. I (regrettably) forgot about it. Normally i'd be a bit more attentive, but i've just been really busy of late with getting life sorted after relocating from Australia to the US 😥

Anyway - I plan to spend some time today to fix these flaky tests, but if you're willing to help resolve them, that might speed things along :)

Regardless, thank you for reminding me!

@gejgalis
Copy link

gejgalis commented Feb 2, 2023

@valendres no worries!
In my opinion you took too risky intervals in tests: 200ms, then 200ms, etc...
NodeJS timers and browser timers are running in different processes so they may be desynced in a matter of machine speed. Github Actions VMs are basically slow. I would multiply intervals by 10: delay: 5000 then verifivations every 2000 ms

@valendres
Copy link
Owner Author

Thanks, @gejgalis. I completely agree, the current timeouts are way too risky. I've increased them in #55 to use your suggested timings along with addressing some floating promises and it has resolved the flakiness 🎉

@valendres
Copy link
Owner Author

A fix for this has been released in playwright-msw@2.2.0

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.

Delay is ignored
3 participants