web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.98k stars 3.09k forks source link

idle-detection tests use Blink-only features #15753

Open jdm opened 5 years ago

jdm commented 5 years ago

From https://github.com/web-platform-tests/wpt/pull/14539:

<script src="/gen/layout_test_data/mojo/public/js/mojo_bindings.js"></script>
<script src="/gen/mojo/public/mojom/base/string16.mojom.js"></script>
<script src="/gen/mojo/public/mojom/base/time.mojom.js"></script>
<script src="/gen/third_party/blink/public/platform/modules/idle/idle_manager.mojom.js"></script>

And mock.js has this comment:

 * The mocking API is blink agnostic and is designed such that other engines
 * could implement it too. Here are the symbols that are exposed to tests:

But it's not clear to me how other engines are supposed to override the blink-specific implementation that exists in that file.

It would be nice to add a lint check for things like this. @foolip The author isn't associated with a github account; who is an appropriate party to tag in here?

foolip commented 5 years ago

@samuelgoto, this issue is about the tests from https://chromium-review.googlesource.com/c/chromium/src/+/1371186.

Is the purpose of the mocking to change what navigator.idle.query resolves with? If so, this is probably a case where a WebDriver extension command in the style of https://w3c.github.io/reporting/#generate-test-report-command could work.

If there are reasons why Mojo mocking has to be used, then a setup more like webxr/resources/webxr_util.js and a spec of the test-only APIs which other browsers would have to implement would be great.

@LukeZielinski FYI.

samuelgoto commented 5 years ago

We (@reillyeon and I) originally had in mind something along the lines of the following:

https://wicg.github.io/webusb/test/

So, some type of specification that we write that (a) is only implemented in tests and (b) gives other engines the ability to make these assertions on their implementation.

I haven't yet gotten to the point where that's written down, but I believe it would look similar to the WebUSB test API in shape, context and functionality.

Does that make sense? Totally cool if it doesn't, happy to course correct here where it is needed, but original goal was to create a testing framework that other engines would be able to use.

foolip commented 5 years ago

@samuelgoto do you have a rough idea of what the testing API would look like in this case? Something like this?

partial interface IdleManager {
  Promise<void> set(IdleState state);
};

Perhaps more arguments for set are needed, but the key question is if information only flows in one direction, or if there's also test-only information being returned, somehow, by the API?

If the API would look something like the above, then a setup along the lines of https://w3c.github.io/sensors/#section-extension-commands or https://w3c.github.io/reporting/#generate-test-report-command might work. @LukeZielinski do we have clear steps to follow for adding test automation for a case like this?

reillyeon commented 5 years ago

Mojo mocking doesn't have to be used here but for an API that is still under active development the overhead of implementing WebDriver extension commands will add significant overhead which is why a lighter-weight approach has been taken here. The tests here originally isolated the Blink-specific code more thoroughly but that was reverted before the patch landed and should be fixed. (AI for @samuelgoto )

If properly isolated then the Blink-specific code can be transparently replaced with code using a WebDriver extension command or another engine-specific implementation.

Is there any guidance for how to implement a WebDriver extension command end-to-end in Chromium? The last time I looked into it it seemed rather complex.

LukeZielinski commented 5 years ago

We don't have formal documentation on the process, but there is pretty decent guidance in the form of an example here. This runs through the case of adding the generateTestReport API and covers each of the four major steps required.

But you're right, the process is fairly involved (and likely hasn't changed much since you last looked at it), so I agree that it makes sense for the feature to be complete before revisiting the WebDriver extension.

The blink-specific code isolation you refer to: that should prevent us from getting the errors that we're seeing on upstream WPT, right? wpt.fyi link

reillyeon commented 5 years ago

Which errors do you want to prevent? I filed web-platform-tests/results-collection#81 a while back to have Chrome run with the right flags so that if we check in all the generated Mojo bindings (as is done for the Generic Sensors and WebUSB tests) then the tests should pass on wpt.fyi. Without those flags enabled an official Chrome build as is used for upstream WPT won't work. Do you mean that or the errors in other browsers. That's just a question of what kind of failure we want to hit when the API is unimplemented. I recall someone cleaning up the errors in the Generic Sensors test but I honestly don't really see the point since if the browser doesn't implement the API does it matter exactly which error is reported when one tries to test it?

LukeZielinski commented 5 years ago

I was referring to the Chrome failures, which (as you described) are because of the missing flags/bindings in WPT. Mostly because it's not as nice that we have the test failing in wpt.fyi.

I'd like to log an issue to keep track of whether we can upgrade this to use WebDriver at some point down the road. Can you describe what the testing API would look like for this feature? Is it as simple as what Philip described above:

partial interface IdleManager {
  Promise<void> set(IdleState state);
};

Or is there more to it? (Or do we not know yet, because it's still under development)?

Thanks!

reillyeon commented 5 years ago

I've filed samuelgoto/idle-detection#6 to track this work.

rakuco commented 4 years ago

FWIW, @arskama's #25426 has recently improved the situation in that the idle-detection/interceptor.https.html no longer directly includes Blink-specific JS files and per https://wpt.fyi/results/idle-detection/interceptor.https.html?run_id=667290001 the test is passing in wpt.fyi.

Spec and documentation for other implementations likely still need to work on though (I'm not working on this API myself).