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

Add service worker support #3419

Merged
merged 26 commits into from
Mar 11, 2024
Merged

Add service worker support #3419

merged 26 commits into from
Mar 11, 2024

Conversation

beaufortfrancois
Copy link
Collaborator

@beaufortfrancois beaufortfrancois commented Feb 19, 2024

Spec PR: gpuweb/gpuweb#4465


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

Base automatically changed from shared-worker to main February 21, 2024 15:35
@beaufortfrancois beaufortfrancois force-pushed the service-worker branch 5 times, most recently from 6057e70 to 7a96c3c Compare March 4, 2024 13:35
@beaufortfrancois beaufortfrancois marked this pull request as ready for review March 4, 2024 13:38
@beaufortfrancois
Copy link
Collaborator Author

@kainino0x After many efforts, I was able to get them working. /me happy!

I'll appreciate your review as I had to take corners due to my unfamiliarity with the WebGPU CTS infrastructure.

@kainino0x
Copy link
Collaborator

Nice!! Sorry, I ended up with a lot of code reviewing to do today. I will review soon.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

This looks great so far, not too complex!

src/common/runtime/helper/test_worker.ts Outdated Show resolved Hide resolved
src/common/runtime/helper/test_worker.ts Outdated Show resolved Hide resolved
src/common/tools/dev_server.ts Outdated Show resolved Hide resolved
src/common/tools/dev_server.ts Outdated Show resolved Hide resolved
src/common/tools/dev_server.ts Outdated Show resolved Hide resolved
src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/worker/worker.spec.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/worker/worker.spec.ts Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@beaufortfrancois beaufortfrancois left a comment

Choose a reason for hiding this comment

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

Thank you @kainino0x for the review!

src/common/tools/dev_server.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/worker/worker.spec.ts Outdated Show resolved Hide resolved
src/webgpu/web_platform/worker/worker.spec.ts Outdated Show resolved Hide resolved
src/common/runtime/helper/test_worker.ts Outdated Show resolved Hide resolved
src/common/runtime/helper/test_worker.ts Outdated Show resolved Hide resolved
src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/common/tools/dev_server.ts Outdated Show resolved Hide resolved
kainino0x and others added 5 commits March 6, 2024 13:44
This will be needed to launch Service Workers, which cannot be generated
at runtime and cannot use dynamic import().
@kainino0x
Copy link
Collaborator

@beaufortfrancois
Copy link
Collaborator Author

nit: Avoid force-pushing: https://github.com/gpuweb/cts/blob/main/docs/intro/developing.md#pull-requests

I will be more vigilant in the future. Sorry for that.

@kainino0x
Copy link
Collaborator

All good! Left one more comment.

src/common/tools/gen_listings_and_webworkers.ts Outdated Show resolved Hide resolved
src/common/tools/dev_server.ts Outdated Show resolved Hide resolved
@kainino0x
Copy link
Collaborator

@beaufortfrancois I pushed some commits polishing up some bits. Please review, then this should be ready to squash-and-merge!

@beaufortfrancois
Copy link
Collaborator Author

beaufortfrancois commented Mar 8, 2024

@kainino0x Everything but #3419 (comment) looks good to me. Thank you for polishing this PR!

@beaufortfrancois
Copy link
Collaborator Author

@kainino0x I think we're good to merge now. I'll let you do it.

@kainino0x kainino0x enabled auto-merge (squash) March 11, 2024 19:51
@kainino0x kainino0x merged commit 5ad7589 into main Mar 11, 2024
1 check passed
@kainino0x kainino0x deleted the service-worker branch March 11, 2024 19:54
@beaufortfrancois
Copy link
Collaborator Author

@kainino0x Thank you!

ben-clayton pushed a commit to ben-clayton/cts that referenced this pull request Mar 18, 2024
* Generate Web Worker script for every test file

This will be needed to launch Service Workers, which cannot be generated
at runtime and cannot use dynamic import().

* Add service worker support

* Update wpt.ts

* Run tests

* Address kainino0x feedback

* Address feedback | part 2

* Address feedback | part 3

* Address feedback | part 4

* Address feedback | part 5

* Address feedback | part 6

* Address feedback | part 7

* Address feedback | part 8

* Apply suggestions from code review

* use WorkerTestRunRequest in the postMessage/onmessage interface

* Clean up resolvers map

* Use express routing for .worker.js

* Skip worker tests when run in a worker that can't support them

DedicatedWorker can be created from DedicatedWorker, but none of the
other nested worker pairs are allowed.

* Clean up all service workers on startup and shutdown

* Avoid reinitializing service workers for every single case

* lint fixes

* Catch errors in wrapTestGroupForWorker onMessage

* Make sure the service worker has the correct URL

---------

Co-authored-by: Kai Ninomiya <[email protected]>
ben-clayton pushed a commit to ben-clayton/cts that referenced this pull request Mar 18, 2024
* Generate Web Worker script for every test file

This will be needed to launch Service Workers, which cannot be generated
at runtime and cannot use dynamic import().

* Add service worker support

* Update wpt.ts

* Run tests

* Address kainino0x feedback

* Address feedback | part 2

* Address feedback | part 3

* Address feedback | part 4

* Address feedback | part 5

* Address feedback | part 6

* Address feedback | part 7

* Address feedback | part 8

* Apply suggestions from code review

* use WorkerTestRunRequest in the postMessage/onmessage interface

* Clean up resolvers map

* Use express routing for .worker.js

* Skip worker tests when run in a worker that can't support them

DedicatedWorker can be created from DedicatedWorker, but none of the
other nested worker pairs are allowed.

* Clean up all service workers on startup and shutdown

* Avoid reinitializing service workers for every single case

* lint fixes

* Catch errors in wrapTestGroupForWorker onMessage

* Make sure the service worker has the correct URL

---------

Co-authored-by: Kai Ninomiya <[email protected]>
ben-clayton pushed a commit to ben-clayton/cts that referenced this pull request Mar 19, 2024
* Generate Web Worker script for every test file

This will be needed to launch Service Workers, which cannot be generated
at runtime and cannot use dynamic import().

* Add service worker support

* Update wpt.ts

* Run tests

* Address kainino0x feedback

* Address feedback | part 2

* Address feedback | part 3

* Address feedback | part 4

* Address feedback | part 5

* Address feedback | part 6

* Address feedback | part 7

* Address feedback | part 8

* Apply suggestions from code review

* use WorkerTestRunRequest in the postMessage/onmessage interface

* Clean up resolvers map

* Use express routing for .worker.js

* Skip worker tests when run in a worker that can't support them

DedicatedWorker can be created from DedicatedWorker, but none of the
other nested worker pairs are allowed.

* Clean up all service workers on startup and shutdown

* Avoid reinitializing service workers for every single case

* lint fixes

* Catch errors in wrapTestGroupForWorker onMessage

* Make sure the service worker has the correct URL

---------

Co-authored-by: Kai Ninomiya <[email protected]>
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.

2 participants