-
Notifications
You must be signed in to change notification settings - Fork 801
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
Address tab-related flakiness in integration tests #2754
Conversation
@@ -17,8 +17,7 @@ const windowLoaded = async () => { | |||
await executeAsyncAndCatch(async (cb) => { | |||
const loaded = () => { | |||
if (!window.Workbox) { | |||
const error = new Error('Workbox not yet loaded...'); | |||
cb({error: error.stack}); | |||
cb({error: `window.Workbox is undefined; location is ${location.href}`}); |
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.
This is what helped me realize that when the workbox-window
tests were failing, it was because a tab navigated to the workbox-broadcast-update
URL had the focus...
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.
LGTM with one question.
R: @philipwalton
I did a little digging, and I think this change fixes #2321, which was caused by a leftover tab from the
workbox-broadcast-update
test sometimes getting the focus in the middle of theworkbox-window
test suite. This adds a bit of structure around how tabs are opened and closed to hopefully prevent that.That being said, the
workbox-window
integration test for externally triggered registrations is still skipped in Safari, because it just doesn't work. I tried doing the same process manually, and at that point, it follows the steps that should be covered in test, so I don't know if it's just a timing issue, or if there's something off about the webdriver environment with regard to service worker registrations in other Safari tabs.In any case, I hope this PR is a still towards a bit less flakiness.