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

Make .any.js tests work for service/shared workers too #4210

Closed
domenic opened this issue Nov 15, 2016 · 25 comments
Closed

Make .any.js tests work for service/shared workers too #4210

domenic opened this issue Nov 15, 2016 · 25 comments

Comments

@domenic
Copy link
Member

domenic commented Nov 15, 2016

Right now streams has its own hacky setup for running tests in three environments (window, dedicated worker, shared worker, service worker). Previously discussed in e.g. w3c/testharness.js#168 (cc @youennf). It would be ideal if we could get rid of that and just use the .any.js setup. Right now we use https://github.com/w3c/web-platform-tests/blob/master/streams/resources/test-initializer.js which causes duplicate test names.

I am not sure how the service worker part of this interacts with the HTTPS requirement (which seems to involve adding .https to test filenames for some reason).

I also can't find the docs for the .any.js thing. I've had this problem before. If they could be made more prominent somehow that would be great.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

If we did this, we might need some way to exclude service workers sometimes, e.g., XMLHttpRequest doesn't work there, but does elsewhere.

@domenic
Copy link
Member Author

domenic commented Jul 31, 2017

Note that the abort controller tests are now also generating four boilerplate wrapper files for each test in order to test in all four environments. I think we'll see more of this going forward. I'd love to get this fixed.

I should also add that for streams tests we are using a generator script for the wrappers: https://github.com/w3c/web-platform-tests/blob/75b7060a306053d9392c9a9e99e47798a70e6188/streams/generate-test-wrappers.js

@annevk
Copy link
Member

annevk commented Aug 1, 2017

FWIW, I think we first need to settle which options we want and how to name those. Then introduce an alternative to .all that more specifically means window+worker (can probably always include shared workers?). Then rename existing .all files that are not actually .all to that. And then rebrand all. Or maybe we should just stop using .all altogether given worklets and other future environments we might wish to exclude.

@jgraham
Copy link
Contributor

jgraham commented Aug 1, 2017

Right, I think the implementation is not the hardest thing here, it's deciding on what you want going forward.

@annevk
Copy link
Member

annevk commented Aug 2, 2017

Do we want descriptive or short? If descriptive I suggest we have:

  • .window
  • .worker
  • .worker-no-sw
  • .dedicatedworker
  • .sharedworker
  • .serviceworker

And you can mix .window and one of the latter types as, e.g., .window.worker-no-sw or .window.sharedworker. I think we could get away with not having .dedicatedworker and .sharedworker for v1, but maybe I'm forgetting something important? If we want short we should just abbreviate these, but I'm not sure that's worth it.

@domenic
Copy link
Member Author

domenic commented Aug 2, 2017

I think we'd need .window.dedicatedworker if we want to do a purely mechanical translation from current .any.js tests. But maybe we can assume everything that should pass in dedicated workers should pass in shared workers too?

@annevk
Copy link
Member

annevk commented Aug 2, 2017

I was, yes, but we should probably check (result of find . -name "*.any.js"):

./console/console-count-label-conversion.any.js
./console/console-is-a-namespace.any.js
./console/console-time-label-conversion.any.js
./console/console-timeline-timelineEnd-historical.any.js
./dom/abort/event.any.js
./dom/events/EventTarget-constructible.any.js
./encoding/textdecoder-copy.any.js
./fetch/api/basic/accept-header.any.js
./fetch/api/basic/mode-same-origin.any.js
./fetch/api/basic/referrer.any.js
./fetch/api/basic/request-forbidden-headers.any.js
./fetch/api/basic/request-head.any.js
./fetch/api/basic/request-headers.any.js
./fetch/api/basic/request-referrer.any.js
./fetch/api/basic/request-upload.any.js
./fetch/api/basic/scheme-about.any.js
./fetch/api/basic/scheme-data.any.js
./fetch/api/basic/stream-response.any.js
./fetch/api/cors/cors-basic.any.js
./fetch/api/cors/cors-cookies.any.js
./fetch/api/cors/cors-no-preflight.any.js
./fetch/api/cors/cors-origin.any.js
./fetch/api/cors/cors-preflight-redirect.any.js
./fetch/api/cors/cors-preflight-referrer.any.js
./fetch/api/cors/cors-preflight-star.any.js
./fetch/api/cors/cors-preflight-status.any.js
./fetch/api/cors/cors-preflight.any.js
./fetch/api/cors/cors-redirect-credentials.any.js
./fetch/api/cors/cors-redirect-preflight.any.js
./fetch/api/cors/cors-redirect.any.js
./fetch/api/credentials/authentication-basic.any.js
./fetch/api/credentials/cookies.any.js
./fetch/api/headers/historical.any.js
./hr-time/basic.any.js
./hr-time/monotonic-clock.any.js
./html/dom/self-origin.any.js
./html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/serialization-via-idb.any.js
./html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/serialization-via-notifications-api.any.js
./html/webappapis/system-state-and-capabilities/the-navigator-object/navigator.any.js
./performance-timeline/case-sensitivity.any.js
./performance-timeline/po-callback-mutate.any.js
./performance-timeline/po-disconnect.any.js
./performance-timeline/po-entries-sort.any.js
./performance-timeline/po-getentries.any.js
./performance-timeline/po-mark-measure.any.js
./performance-timeline/po-observe.any.js
./url/historical.any.js
./url/interfaces.any.js
./user-timing/clear_all_marks.any.js
./user-timing/clear_all_measures.any.js
./user-timing/clear_non_existent_mark.any.js
./user-timing/clear_non_existent_measure.any.js
./user-timing/clear_one_mark.any.js
./user-timing/clear_one_measure.any.js
./user-timing/entry_type.any.js
./user-timing/mark.any.js
./user-timing/measure_syntax_err.any.js
./user-timing/user_timing_exists.any.js
./WebCryptoAPI/getRandomValues.any.js
./WebIDL/ecmascript-binding/es-exceptions/DOMException-constants.any.js
./WebIDL/ecmascript-binding/es-exceptions/DOMException-constructor-and-prototype.any.js
./WebIDL/ecmascript-binding/es-exceptions/DOMException-constructor-behavior.any.js
./WebIDL/ecmascript-binding/es-exceptions/DOMException-custom-bindings.any.js
./websockets/close-invalid.any.js
./XMLHttpRequest/send-usp.any.js
./XMLHttpRequest/XMLHttpRequest-withCredentials.any.js

@annevk
Copy link
Member

annevk commented Aug 2, 2017

I think all of these should work in shared workers and a lot should also work in service workers (in principle, not sure how the wrapping works there currently).

@jakearchibald
Copy link
Contributor

I think it's only XHR that we actively disallow in service workers, so we could allow "all" to mean all, but change the XHR tests to be something else.

We have

  • window
  • worker (worker & shared worker)
  • serviceworker
  • Potentially worklets etc in future

Could we just allow multiple extensions

  • mytest.window.worker.js
  • mytest.worker.serviceworker.js

The problem is that .worker.serviceworker.js and serviceworker.worker.js are the same thing, but we should catch that in review. We already have these kind of ambiguities.

So mytest.html would check for:

  • mytest.html
  • /mytest(\.[^.]+)+\.js/ where one of the matching groups is ".window"
  • mytest.any.js

jakearchibald added a commit to jakearchibald/web-platform-tests that referenced this issue Aug 4, 2017
@annevk
Copy link
Member

annevk commented Aug 4, 2017

I think worker should also mean serviceworker since that is what it means in the standard (and have something explicit for XMLHttpRequest and maybe other APIs to exclude them such as worker-no-sw). Allowing multiple extensions seems fine, but I agree that ideally we enforce an order. We could just not make certain orders work?

@annevk
Copy link
Member

annevk commented Aug 4, 2017

An alternative might be to just enumerate all reasonable combinations:

  • window
  • worker (includes SW)
  • worker-no-sw
  • serviceworker
  • window-worker
  • window-worker-no-sw

Then we don't have to worry about order and such.

@jakearchibald
Copy link
Contributor

Only thing I'm worried about is that list exponentially growing.

@annevk
Copy link
Member

annevk commented Aug 4, 2017

That would then also be directly indicative of how complex browsers are becoming (and might make folks think twice of where to expose features). The server implementation could still share a bunch of logic for the various identifiers of course.

@domenic
Copy link
Member Author

domenic commented Aug 4, 2017

I don't think a single list is tenable given worklets?

Worklets in fact have a much-constrained API surface (e.g. no fetch() in most of them), so might be special anyway...

@annevk
Copy link
Member

annevk commented Aug 4, 2017

Yeah, they might not use this mechanism or get their own mechanism that is somewhat more tailored. I don't think it'll really make this list explode.

@annevk
Copy link
Member

annevk commented Aug 4, 2017

Note that if you don't go for a "big" list you get:

  • window
  • worker (includes SW)
  • worker-no-sw
  • serviceworker

and then you need to have logic that prevents combinations, enforces order, etc. when in fact there's only two reasonable combinations as I listed above. It really doesn't seem that bad to hard code them.

@domenic
Copy link
Member Author

domenic commented Aug 4, 2017

I guess upon reviewing the actual worklets we have today they don't expose any functionality that is expected to work in multiple contexts. So including them would only be necessary for testing baseline functionality (e.g. typed array or module web platform integration). So the list would only need to be expanded to include some distinction between "all" (JS module tests) and "all-but-worklet" (fetch tests).

@annevk
Copy link
Member

annevk commented Aug 8, 2017

@jgraham came up with a different approach that seems worth considering given that the framework already needs to read the contents of files for timeouts. We have *.multi.js as signifier and we indicate inside the file for which environments it works:

//META variants=worker,window,!serviceworker

Would be equivalent to window-worker-no-sw.

@jakearchibald
Copy link
Contributor

That seems like the best option so far

@annevk
Copy link
Member

annevk commented Aug 9, 2017

Created #6792. Review appreciated.

@foolip
Copy link
Member

foolip commented Oct 4, 2017

Since a PR exists for this, I'm adding the roadmap prio label. To whoever comes along next: if there's no action on the review, randomly assign someone on the ecosystem infra team to review it.

@leonhsl
Copy link

leonhsl commented Nov 24, 2017

Set the PR's creator @annevk as owner of this issue for now, to make it be owned :) If not proper, please help to reassign. Thanks.

@foolip
Copy link
Member

foolip commented Feb 21, 2018

What's up for review in #6792 doesn't match the comment #4210 (comment), is that no longer an idea to pursue, or is #6792 just the first step?

@annevk
Copy link
Member

annevk commented Feb 21, 2018

@foolip no, the PR pursues that proposal, roughly. multi is now any and variants is global, but otherwise it's basically that.

@foolip
Copy link
Member

foolip commented Feb 21, 2018

I see, just the names are different, and tools/manifest/sourcefile.py has the stuff around //META.

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

No branches or pull requests

7 participants