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

Support distinguishing service/shared/dedicated workers in .any.js tests #11928

Closed
lukebjerring opened this issue Jul 11, 2018 · 8 comments
Closed

Comments

@lukebjerring
Copy link
Contributor

@lukebjerring lukebjerring commented Jul 11, 2018

An example use-case for this feature would be any IDL that spans several combinations of subsets of window/sharedworker/serviceworker/dedicatedworker, e.g. push-api has PushManager (Window, Worker) and PushEvent (ServiceWorker).

Currently, distinction is only possible with something like

location.path.includes('.serviceworker.')

or

self instanceof ServiceWorkerGlobalScope

NB: Somewhat related to #11529

@foolip
Copy link
Member

@foolip foolip commented Jul 18, 2018

Example of the four different global scopes currently supported:
http://w3c-test.org/IndexedDB/idlharness.any.html (window)
http://w3c-test.org/IndexedDB/idlharness.any.sharedworker.html
http://w3c-test.org/IndexedDB/idlharness.any.worker.html
https://w3c-test.org/IndexedDB/idlharness.https.any.serviceworker.html

The worker ones are backed by http://w3c-test.org/IndexedDB/idlharness.any.worker.js.

In the output:

self.GLOBAL = {
  isWindow: function() { return true; },
  isWorker: function() { return false; },
};

This is actually documented in https://web-platform-tests.org/writing-tests/testharness.html.

If we could expand this to have isDedicatedWorker(), isSharedWorker() and isServiceWorker(), probably keeping isWorker() as the union of the three, then that should solve the problem.

Technically, that's not entirely straightforward since the same .any.worker.js is used for all three kinds of workers. Options:

  • Turn the generated .any.worker.js into three different generated files with different self.GLOBAL preamble.
  • Just use self instanceof ServiceWorkerGlobalScope to implement these helpers.
@foolip
Copy link
Member

@foolip foolip commented Jul 18, 2018

@lukebjerring, would this be used to make the correct assertions about what interface objects etc. are exposed in each global scope? Does idlharness.js currently just get it wrong?

@lukebjerring
Copy link
Contributor Author

@lukebjerring lukebjerring commented Jul 18, 2018

idlharness already uses self instanceof checks, so it's not farfetched that we continue that pattern. However, it seems inconsistent to have an isWorker function prepended in one case and a computation function elsewhere. Why do we have the isWorker function at all? We could compute that too.

@foolip
Copy link
Member

@foolip foolip commented Jul 19, 2018

It was added in w3c/wpt-tools#88, maybe @Ms2ger or @jgraham know why?

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jul 23, 2018

I added it as an easy way to distinguish the cases that couldn't be affected by browser bugs. I'm not sure that's still a valid concern, though.

I'd generally prefer a solution that didn't involve more generated files.

@foolip
Copy link
Member

@foolip foolip commented Jul 23, 2018

I added it as an easy way to distinguish the cases that couldn't be affected by browser bugs.

I wouldn't be surprised if someone forgot to expose the WorkerGlobalScope interface object somewhere at some point, so that makes sense.

What we could do instead is to add tests under /infrastructure/, with one global per test, that asserts that the self instanceof Something checks work, and then just use that pattern and remove self.GLOBAL. Option is to add non-generated helpers on self.GLOBAL. @lukebjerring, WDYT?

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jul 23, 2018

One other thing to note is that you always need to check "FooScope" in self && self instanceof self.FooScope; it'd be somewhat nice to have a shorthand. Maybe that would be better put in th.js directly, though.

Anyway, I'm fine whichever way, as long as the tests keep working :)

@lukebjerring
Copy link
Contributor Author

@lukebjerring lukebjerring commented Aug 15, 2019

Seems like the verdict here is that further additions to self.GLOBAL isn't desirable.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.