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

idle-detection tests use Blink-only features #15753

Open
jdm opened this issue Mar 10, 2019 · 9 comments
Open

idle-detection tests use Blink-only features #15753

jdm opened this issue Mar 10, 2019 · 9 comments

Comments

@jdm
Copy link
Contributor

jdm commented Mar 10, 2019

From #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
Copy link
Member

foolip commented Mar 14, 2019

@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
Copy link

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
Copy link
Member

foolip commented Mar 26, 2019

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@rakuco
Copy link
Member

rakuco commented Sep 11, 2020

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).

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

6 participants