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

Exposing vendor specific API for testing (e.g. terminateServiceWorker) #6866

Open
makotoshimazu opened this issue Aug 14, 2017 · 23 comments
Open

Comments

@makotoshimazu
Copy link
Contributor

I'm now trying to add a test like the following steps:

  1. Register a service worker (and it launches the worker)
  2. Terminate the worker
  3. Invoke fetch event

For chromium, we have a testing API (internals.terminateServiceWorker()) for stopping the worker so we can make the test as a chromium specific test, but I think it's worth considering how to get the test in the web platform tests.

I also realized we don't have any tests in which service workers get launched by events.
The flow of current tests is like (1) registration (and it creates the worker thread), (2) install/activate events on the worker's thread, (3) run some tests (on the worker and the page), and we don't have any tests for stopped but installed service workers though usually the path to get the installed worker's scripts is different from the path to load the worker's scripts from the network.

Is it possible to have an unified way to expose some internals API for testing to WPT?

@kereliuk
Copy link
Contributor

@makotoshimazu Are there existing manual tests that require this?

@makotoshimazu
Copy link
Contributor Author

Sorry, can I ask you what's the manual tests?

In chromium, we have tests which requires vendor specific APIs in a separated directory, and it runs on chrome specific test runner.
For example, some of service worker tests requires terminateServiceWorker API (which isn't in spec) as I mentioned in previous comment, and we have these tests in here.
Thinking of the test coverage, ideally they could be moved to the web platform tests. That's why I filed this issue.

@youennf
Copy link
Contributor

youennf commented Nov 21, 2017

Test runner APIs are being used in Chrome (and probably WebKit in the future) to directly implement what is done otherwise with WebDriver. This is used for manual tests in cases like triggering a user click on a button.

WebKit is close in terms of model to Chrome, so it would be probably fine (and useful) to share such test runner based tests. I am not sure how well that would apply to Gecko and Edge.
Since these tests cannot run on regular browsers, they would need special error casing there (at least for the dashboard).

There would also be a need to specify somewhere such test runner API.
In the case of terminateServiceWorker API, this is probably easy.

@makotoshimazu
Copy link
Contributor Author

Chromium has --expose-internals-for-testing flag to expose the terminateServiceWorker API.
So, I'm not quite familiar with other browsers and test runner implementation, but I originally imagined something adding additional flag when launching the browser, and using it in a interface function.
For example, we could have these vendor specific non-spec'ed APIs in one place:

// in a JS file in WPT
DefaultVendorSpecificAPI = function() {}
DefaultVendorSpecificAPI.prototype.terminateServiceWorker = function() {
    throw SomeSpecialError();
}

and implement them for each browser, and use it as follows.

if (IsChromium()) {
  VendorSpecificAPI = new ChromiumVendorSpecificAPI();
} else if (...) {
 ...
} else {
  VendorSpecificAPI = new DefaultVendorSpecificAPI();
}

Some tests using not implemented APIs would fail with SomeSpecialError(), so we would be able to distinguish them.

@foolip
Copy link
Member

foolip commented Nov 24, 2017

@makotoshimazu, I see that there are 9 tests in Chromium that use terminateServiceWorker. How valuable do you think these tests would be to other implementers, how likely are they to reveal interop problems for example? (Trying to decide on what priority label to give this.)

We now have a mechanism on which testing API like this can be built:
http://web-platform-tests.org/writing-tests/testdriver.html

However, it's currently only for wrapping WebDriver APIs, and it's not widely used and battle tested yet. @JKereliuk, can you work with @makotoshimazu to see if this would make sense as a WebDriver API, or if something else is needed?

@foolip
Copy link
Member

foolip commented Nov 24, 2017

Tentatively calling it roadmap, but if this issue comes up in the infra issue triage and there doesn't seem to be that much value in sharing these tests, we should downgrade to backlog.

@makotoshimazu
Copy link
Contributor Author

I see, testdriver.js looks good place to do that:)

Restarting workers would reveal issues around headers attached to the response of the installed service worker. For example, we can test if CSP header or referrer header is working well even after the script is loaded from a storage.
Also, we can test the most important path - launching installed service workers from events. It should work on all platforms, but currently we don't have the way to test that.

@youennf
Copy link
Contributor

youennf commented Nov 27, 2017

I see, testdriver.js looks good place to do that:)

I would prefer keeping it separate.
WebKit will probably try to fully support testdriver.js for both Safari and test runner.
This terminate worker API might be supported in WebKit test runner but I am unsure it will ever get exposed in Safari even behind a command line flag.

That would mean these tests will probably always fail for Safari in https://wpt.fyi.
Dashboard should make it clear that tests are n/a instead of failing.

Restarting workers would reveal issues around headers attached to the response of the installed service worker. For example, we can test if CSP header or referrer header is working well even after the script is loaded from a storage.
Also, we can test the most important path - launching installed service workers from events. It should work on all platforms, but currently we don't have the way to test that.

Sharing such tests look promising.

@foolip
Copy link
Member

foolip commented Nov 27, 2017

@youennf, sounds like regardless of the shape of the testing mechanism, the tests would be failing in Safari, right? Given that, wouldn't it be OK to put it in testdriver.js if at least on WebDriver implementation has some API for this?

@youennf
Copy link
Contributor

youennf commented Nov 30, 2017

WebKit is starting to implement a terminateServiceWorker testRunner API.
Ideally it should be aligned with Chrome implementation.
Apparently Chrome implementation is synchronous, while webkit would prefer it to be promise-based.
Any chance Chrome might be able to change API and related tests?

I am unsure whether a WebDriver "terminate service worker" API makes sense.
Some users may clear their history and kill all service workers at that time.
Web sites might want to test that?
If so, putting it in testdriver.js makes sense.

@mfalken
Copy link
Member

mfalken commented Dec 1, 2017

The current Chrome implementation is just wrong: it doesn't wait for the worker to terminate. So the tests are probably not always testing what's intended.

We should probably change it to be Promise-based.

I'm not familiar with WebDriver and testdriver.js. Chrome's tests in question are not manual tests, just tests that want to terminate a service worker and test something after starting it up again. I agree these tests are probably useful for all implementors.

[edit: "I'm familiar" -> "I'm not familiar"]

@foolip
Copy link
Member

foolip commented Dec 1, 2017

Is it the case that this API would make no sense from the outside (WebDriver) because there's no handle for the thing to terminate, or would it be weird even given such a handle?

@youennf
Copy link
Contributor

youennf commented Dec 2, 2017

This API is currently given a ServiceWorker object, a web page can get access to all its related service workers through getRegistrations. I guess web driver could get access to this list and request termination of any of these.
The question is whether there are enough use cases to justify this feature in Web Driver.

@alijuma
Copy link
Contributor

alijuma commented Feb 13, 2018

It's been a while since there was last updated. Is there still interest in adding this feature to WebDriver, and should this still be priority:roadmap?

@foolip
Copy link
Member

foolip commented Feb 22, 2018

@makotoshimazu, what ended up happening with the tests in questions? Are they simply Blink-specific for now?

@mfalken
Copy link
Member

mfalken commented Feb 23, 2018

Yes these are Blink-specific for now as they're using the internals API.

Incidentally, @bashi is looking into making Chrome's terminateServiceWorker API return a promise.

@foolip
Copy link
Member

foolip commented Feb 23, 2018

I wonder what shape of a solution would be most preferable to all implementers here? Can this API be sensibly be expressed as a WebDriver API? How would the ServiceWorker instance be represented? @kereliuk, what do you think?

@kereliuk
Copy link
Contributor

kereliuk commented Feb 23, 2018

I probably lack some context, but I would image it wouldn't be too difficult to specify this as an extension in the Service Worker spec.

Maybe something like this with the details filled in.

  1. Register a service worker (and it launches the worker)

This could look like :sessionId/serviceworker/register and return an ID for the worker

  1. Terminate the worker

:sessionId/serviceworker/:id/terminate, terminate the worker with the ID returned from registering it

  1. Invoke fetch event

:sessionId/serviceworker/:id/fetch

@mfalken
Copy link
Member

mfalken commented Feb 27, 2018

Update: @bashi changed Chrome's internals.terminateServiceWorker() to return a promise now: https://bugs.chromium.org/p/chromium/issues/detail?id=816328#c2.

I'm probably also missing context... but ideally these tests could register() and run fetch as normal instead of going through a special API just in order to get the ability to do terminateServiceWorker().

@makotoshimazu
Copy link
Contributor Author

If I understand the context correctly, the difficulty of adding service workers to the WebDriver API may be how to identify a service worker.
For example, suppose that there is a page which registers a service worker, now we can get a handle to the window. However, we need to get a handle for the service worker somehow to manipulate it through WebDriver API (e.g. getting id for the service worker and terminate it). There is an regular JavaScript API to get the service worker representation (navigator.serviceWorker.controller), but I'm not sure if it can be touched from WebDriver API.
I assume that we can do something similar to dedicated workers since it also has separated context. @foolip @kereliuk , how are we handling the workers?

@kereliuk
Copy link
Contributor

kereliuk commented May 2, 2018

@makotoshimazu Sorry this slipped through my notifications :( but no excuses.

here is an regular JavaScript API to get the service worker representation (navigator.serviceWorker.controller), but I'm not sure if it can be touched from WebDriver API.

WebDriver can execute scripts, so I think we should be able to get a handle to a given service worker with WebDriver that way if its possible with the regular js API, but correct me if I'm making an incorrect assumption here :)

@foolip
Copy link
Member

foolip commented May 9, 2018

Since we didn't actually assign an owner for this in Q2 and though there were more serious infra/automation issues to tackle first, downgrading the priority to be transparent about that.

@kereliuk, I wonder if adding testdriver.js API that isn't backed by WebDriver is something we'd ever consider? It would mean that the semantics would have to be defined in some other way, and maybe there wouldn't be a default implementation, but I wonder if we'll have a gray area (including this case) where the overhead of a WebDriver API and the plumbing is such that in practice people just won't do it?

@foolip
Copy link
Member

foolip commented Feb 25, 2019

It sounds from off-issue discussions like it would be possible to define a WebDriver endpoint that takes a scopeURL argument to identify which service workers to terminate, and that this is globally unique.

Perhaps it could be as simple as DELETE /session/{session id}/serviceworker/{scopeURL} with a testdriver.js wrapper test_driver.terminate_service_worker(scopeURL)?

cc @LukeZielinski

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