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

Service worker fetch abort tests #7674

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jakearchibald
Copy link
Contributor

@jakearchibald jakearchibald commented Oct 11, 2017

@jakearchibald
Copy link
Contributor Author

@mattto I'm not getting a fetch event in the navigation preload test, which I would expect. Have I done some really silly, or have I discovered a weird Chrome bug?

@ghost
Copy link

ghost commented Oct 11, 2017

Build PASSED

Started: 2017-11-24 14:37:44
Finished: 2017-11-24 14:44:33

View more information about this build on:

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Generally this looks okay, though I'm far less of an expert. It seems it might be good to cleanup the repeated boilerplate in the promise_tests. That might make them a little easier to read or at least understand the essential difference between them.

}

promise_test(async t => {
await reset();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a test_cleanup step of sorts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it doesn't support async cleanup steps

Copy link
Member

Choose a reason for hiding this comment

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

Aah okay. A single comment to that effect would be good.

Copy link
Member

Choose a reason for hiding this comment

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

And maybe an issue to add promise_cleanup?

@jakearchibald jakearchibald mentioned this pull request Oct 12, 2017
@jakearchibald
Copy link
Contributor Author

The CI is complaining about setTimeout here, but it seems fair to use it in promise tests, especially as it's promisified. Am I misunderstanding?

@mkruisselbrink
Copy link
Contributor

The CI is complaining about setTimeout here, but it seems fair to use it in promise tests, especially as it's promisified. Am I misunderstanding?

It should still be test.step_timeout to deal with timeout multipliers (when tests are run on slow platforms for example).

@jakearchibald
Copy link
Contributor Author

fbfa457

I've removed the use of setTimeout. I'm now doing this:

  1. Start a service worker controlled fetch.
  2. requestAnimationFrame * 2.
  3. Abort the fetch.
  4. requestAnimationFrame * 2.
  5. Tell the service worker "the fetch should be aborted now" via a BroadcastChannel.

My assumption is that the abort will always reach the service worker before the channel broadcast. Is this a safe assumption?

@jakearchibald
Copy link
Contributor Author

@mkruisselbrink could I get a review on the fixes?

@wpt-pr-bot wpt-pr-bot requested a review from beidson December 19, 2017 14:28
@wanderview
Copy link
Member

@jakearchibald We have a test written that might cover some of this as well:

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1394102&attachment=8903478

Do the tests here cover that case? If so, I'm inclined to just defer to these tests.

@ricea
Copy link
Contributor

ricea commented Dec 20, 2017

My assumption is that the abort will always reach the service worker before the channel broadcast. Is this a safe assumption?

No. This kind of thing is probably why setTimeout is banned in the first place. This will make the test flaky. If you need ordering guarantees, please enforce them explicitly.

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

Successfully merging this pull request may close these issues.

6 participants