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

Tests rely on missing infrastructure #16455

Closed
jugglinmike opened this issue Apr 23, 2019 · 23 comments
Closed

Tests rely on missing infrastructure #16455

jugglinmike opened this issue Apr 23, 2019 · 23 comments
Assignees

Comments

@jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Apr 23, 2019

Tests for a few proposals rely on files in a top-level directory named /gen/:

$ git grep -l '/gen/'
idle-detection/interceptor.https.html
shape-detection/resources/shapedetection-helpers.js
webxr/resources/webxr_util.js

Further references have recently been proposed for tests of the "sms" proposal.

This directory does not exist in WPT's source tree, and to the best of my knowledge, it is not generated by any of WPT's infrastructure.

@Hexcles mentioned that this is generated code in Chromium's infrastructure. In that case, I don't think this can be resolved by checking in the files (doing so would presumably interfere with Chromium's tests, and the files are unlikely to be useful for other implementations).

@Hexcles @LukeZielinski Can we formalize a contract which would allow others to use these tests? Do you think it could be stable enough to support implementation by other browsers?

@jugglinmike jugglinmike added the infra label Apr 23, 2019
@jdm
Copy link
Contributor

@jdm jdm commented Apr 23, 2019

I brought up the idle-detection issue in #15753.

@Hexcles
Copy link
Member

@Hexcles Hexcles commented Apr 23, 2019

These are all Mojo stuff. These tests won't work even on Chrome unless 1. generated Mojo JS files are available (this is non-trivial and I'll explain further); 2. a few test flags are turned on (see web-platform-tests/results-collection#582).

Mojo JS stubs are generated from source code at compile time so technically we need the Mojo files generated from the specific revision of Chrome we're testing against, which means we need to publish & download a bundle of files instead of simply checking them into WPT.

More discussions can be found in web-platform-tests/results-collection#81 from last year and cc @foolip .

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Apr 23, 2019

That's helpful information for getting results from Chromium, but it doesn't seem like it will help others run these tests.

In the issue that Josh created for idle-detection, there's been some discussion about documenting a testing API. That's just the kind of contract I had in mind. It seems essential for the tests to be shared. @jdm would you agree?

I can't find instruction like that for idle-detection, shape-detection, webxr, or sms. @samuelgoto @reillyeon would you mind referencing those docs from the tests?

@reillyeon
Copy link
Contributor

@reillyeon reillyeon commented Apr 23, 2019

Are you looking for documentation on how to run these tests or documentation of the API that the tests expect to be provided by a browser implementation?

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Apr 24, 2019

The latter--something along the lines of the document @Hexcles shared for WebXR

@thejohnjansen
Copy link
Contributor

@thejohnjansen thejohnjansen commented Aug 16, 2019

I feel like there is a larger concern here: Google (or anyone working in Chromium) adding tests that rely on internal infra and therefore cannot be run in other browsers. It may be even larger than that: adding tests that are not designed to be run in other browsers. I think this needs to be addressed at that level: given the 2-way sync between Chromium and WPT, I worry this may become a larger problem over time. Should these specific tests even be in the WPT repo?

If not, how do we prevent them from syncing in the future?
if they should be, how do we get them to run in other browsers?

@reillyeon
Copy link
Contributor

@reillyeon reillyeon commented Aug 16, 2019

@thejohnjansen the details matter a lot here. Every browser running WPT comes with a bit of implementation-specific infrastructure for automation. A well-written test abstracts over these differences in order to test a feature on all implementations. The reason why this issue was filed was because those tests weren't properly abstracted from the implementation-specific automation they require.

To get these tests to run in other browsers (aside from the browser implementing the API being tested) the engine needs to add its own version of necessary automation hooks.

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Aug 16, 2019

Thanks for bringing this up again, @thejohnjensen.

given the 2-way sync between Chromium and WPT, I worry this may become a larger problem over time.

You're already correct. Today, there are more references than when this issue was reported:

$ git grep -l '/gen/'
contacts/resources/helpers.js
idle-detection/interceptor.https.html
shape-detection/resources/shapedetection-helpers.js
sms/sms_provider.js
web-nfc/resources/nfc-helpers.js
webxr/resources/webxr_util.js

It's been four months, so I think some action is necessary from WPT. gh-18509 is a patch that causes the project linter to flag these references. Removing the existing tests from WPT directly would likely regress coverage in Chromium, so the patch makes exceptions for those. Probably they should be relocated in the Chromium source tree (and hopefully reintroduced when documentation is available). The new linting rule will at least help us avoid further regression.

@Hexcles
Copy link
Member

@Hexcles Hexcles commented Sep 17, 2019

Following TPAC discussion today, I think we have a consensus here:

  1. It's fine to have test APIs backed by browser-specific implementations (be it test APIs behind prefs or MojoJS interfaces).
  2. We should not check automatically generated MojoJS stubs (i.e. the /gen/ stuff) into WPT (these are build artifacts that are specific to Chromium revisions). In order to make MojoJS work with public Chrome, we would need to figure out a way to archive and release MojoJS stubs per Chrome version. (I'm looking into this.)
  3. It's fine to check the implementations of test APIs that rely on MojoJS into WPT. However, currently these impls would fetch some /gen/ URLs that only exist in Chromium infra, which is unfortunate. We should first at least make the fetches conditional and asynchronous so that they don't cause network errors in the upstream. Once 2 is resolved, we will work out a way to make this more transparent.

Thanks for pushing for the right thing and taking some actions, @jugglinmike and sorry that I haven't done more -- in case it's not clear, I'd like to tackle this. Here are the concrete and parallel steps I'm taking:

  • Make sure the lint rule is working in Chromium (for some reason, it's not currently). Once it is, new references of /gen/ would need to be whitelisted in lint.whitelist, of which I'm an owner in Chromium and I'll be vigilant reviewing the changes.
  • Convert existing fetches of MojoJS to non-blocking & conditional; the tests should fail in some more obvious way other than a network error.
  • Explore releasing MojoJS stubs and make it available to upstream WPT.
@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 17, 2019

@Hexcles Thanks for looking in to this! All those changes sound really positive to me.

I believe the Chromedriver maintainers have recently been working to strengthen the relationship between releases of Chromium and Chromedriver. This is probably a stretch, but I wonder if you could reuse some part of their infrastructure for releasing MojoJS stubs.

@foolip
Copy link
Member

@foolip foolip commented Sep 18, 2019

@makotoshimazu pointed out a Gecko case that isn't using any internal APIs, but does seem a bit odd:

<!-- This test requires browser to treat all registrations are older than 24 hours.
Preference 'dom.serviceWorkers.testUpdateOverOneDay' should be enabled during
the execution of the test -->

For this to change the time while the test is running, that pref would have to cause something to happen while the test is running, and yet I can't see a call to internals.

@jgraham do you have any insight into what this is, is there an implicit internal API hiding here?

@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike commented Sep 18, 2019

The passive voice of a verb like "enable" makes this statement ambiguous. Another interpretation is that the person/system running the test should enable the preference prior to executing the test and that the preference should remain enabled for the duration of the execution. (Come to think of it, I don't know if "enabled" is a verb in that context.)

This alternative interpretation seems more likely in light of the questions @foolip has asked.

/cc @sideshowbarker because they like this sort of thing.

@foolip
Copy link
Member

@foolip foolip commented Sep 18, 2019

That is certainly the right interpretation of when to enable the pref, but what I wonder is what the pref does.

I thought that for it to work, the registration age has to be increased while the test is running, but what's the trigger? But.. perhaps it just causes any registration to immediately be considered 24 hours old. That would work and is probably what this does.

@jgraham
Copy link
Contributor

@jgraham jgraham commented Mar 20, 2020

So I just noticed this. Yes, that sounds like a thing that ought to be documented as part of a testing API. Can we file a specific issue?

stephenmcgruer added a commit that referenced this issue Apr 10, 2020
Due to the difficulty in testing webxr until test-only APIs are
supported in WPT (see
#16455 (comment)),
the added test only tests the XRSession interface additions for now.

Closes #22155
foolip pushed a commit that referenced this issue Apr 20, 2020
Due to the difficulty in testing webxr until test-only APIs are
supported in WPT (see
#16455 (comment)),
the added test only tests the XRSession interface additions for now.

Closes #22155
@Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Apr 21, 2020

I think one thing is being missed when these things are being discussed: The tests do not rely on missing infra here.

Browsers are expected to implement the WebXR Test API and enable it when running WPT. In Chrome's case, they've put their implementation in some autogenerated code, and for some reason references to that code are in WPT, but those codepaths are skipped for browsers which implement the Test API.

@jgraham
Copy link
Contributor

@jgraham jgraham commented Apr 21, 2020

@Manishearth I don't think that's being missed; #16455 (comment) sets that out in detail, and has followup actions for @Hexcles to ensure that things continue to work as expected. I'm not sure if those actions are complete.

My comment at #16455 (comment) was specifically about the service-worker test identified in #16455 (comment) and not related to WebXR.

So I think this bug could either be closed or used to track the work @Hexcles is doing, depending on his preference.

@Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Apr 21, 2020

@jgraham It seemed like this issue was misinterpreted in #22836 , and I recently had another ping that misunderstood the status quo, hence this comment 😄

@jgraham
Copy link
Contributor

@jgraham jgraham commented Apr 21, 2020

Oh OK then. The current situation is suboptimal and confusing, just not actualy broken.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 1, 2020
…testonly

Automatic update from web-platform-tests
Add interfaces/hit-test.idl and test (#22836)

Due to the difficulty in testing webxr until test-only APIs are
supported in WPT (see
web-platform-tests/wpt#16455 (comment)),
the added test only tests the XRSession interface additions for now.

Closes web-platform-tests/wpt#22155
--

wpt-commits: 8a9e492c5bbfafeeb074902ab39910348a20776a
wpt-pr: 22836
xeonchen pushed a commit to xeonchen/gecko that referenced this issue May 1, 2020
…testonly

Automatic update from web-platform-tests
Add interfaces/hit-test.idl and test (#22836)

Due to the difficulty in testing webxr until test-only APIs are
supported in WPT (see
web-platform-tests/wpt#16455 (comment)),
the added test only tests the XRSession interface additions for now.

Closes web-platform-tests/wpt#22155
--

wpt-commits: 8a9e492c5bbfafeeb074902ab39910348a20776a
wpt-pr: 22836
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 1, 2020
…testonly

Automatic update from web-platform-tests
Add interfaces/hit-test.idl and test (#22836)

Due to the difficulty in testing webxr until test-only APIs are
supported in WPT (see
web-platform-tests/wpt#16455 (comment)),
the added test only tests the XRSession interface additions for now.

Closes web-platform-tests/wpt#22155
--

wpt-commits: 8a9e492c5bbfafeeb074902ab39910348a20776a
wpt-pr: 22836
xeonchen pushed a commit to xeonchen/gecko that referenced this issue May 1, 2020
…testonly

Automatic update from web-platform-tests
Add interfaces/hit-test.idl and test (#22836)

Due to the difficulty in testing webxr until test-only APIs are
supported in WPT (see
web-platform-tests/wpt#16455 (comment)),
the added test only tests the XRSession interface additions for now.

Closes web-platform-tests/wpt#22155
--

wpt-commits: 8a9e492c5bbfafeeb074902ab39910348a20776a
wpt-pr: 22836
@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Jun 24, 2020

To keep folks updated: @Hexcles continues to work on making MojoJS work in upstream WPT. We believe we now have a workable solution that will involve publishing the necessary mojom files alongside Chromium builds, which WPT can then fetch and make available (similar to Chromedriver today). We expect to be MVP-ing this in Q3, with an eye to turning the MVP into the real thing if/when we get sign off :)

Hexcles added a commit that referenced this issue Aug 12, 2020
We now have established a formal way to do this (more documentation will
be available in Chromium, as this is Chrome-only) and should no longer
accept these auto-generated files in WPT.

Related to #16455.
Hexcles added a commit that referenced this issue Aug 12, 2020
We now have established a formal way to do this (more documentation will
be available in Chromium, as this is Chrome-only) and should no longer
accept these auto-generated files in WPT.

Related to #16455.
Hexcles added a commit that referenced this issue Aug 13, 2020
We now have established a formal way to do this (more documentation will
be available in Chromium, as this is Chrome-only) and should no longer
accept these auto-generated files in WPT.

Related to #16455.
@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Sep 6, 2020

To keep folks updated: @Hexcles has completed the majority of the work to make MojoJS work in upstream WPT, and is currently in clean-up + follow-through mode. We expect to close this issue soon.

@Hexcles
Copy link
Member

@Hexcles Hexcles commented Sep 16, 2020

Alright, I'm happy to announce that we've officially finished the cleanup. Here's the full list of tests that use MojoJS:

wpt/lint.ignore

Lines 714 to 729 in 70f1fc9

# Tests that depend on resources in /gen/ in Chromium:
# https://github.com/web-platform-tests/wpt/issues/16455
# Please consult with ecosystem-infra@chromium.org before adding more.
MISSING DEPENDENCY: bluetooth/resources/bluetooth-test.js
MISSING DEPENDENCY: contacts/resources/helpers.js
MISSING DEPENDENCY: credential-management/support/otpcredential-helper.js
MISSING DEPENDENCY: generic-sensor/resources/generic-sensor-helpers.js
MISSING DEPENDENCY: idle-detection/resources/idle-detection-helper.js
MISSING DEPENDENCY: mediacapture-image/resources/imagecapture-helpers.js
MISSING DEPENDENCY: orientation-event/resources/orientation-event-helpers.js
MISSING DEPENDENCY: resources/test-only-api.js
MISSING DEPENDENCY: screen_enumeration/resources/screenenumeration-helpers.js
MISSING DEPENDENCY: shape-detection/resources/shapedetection-helpers.js
MISSING DEPENDENCY: web-nfc/resources/nfc-helpers.js
MISSING DEPENDENCY: webusb/resources/usb-helpers.js
MISSING DEPENDENCY: webxr/resources/webxr_util.js

All of them use the newly introduced https://github.com/web-platform-tests/wpt/blob/master/resources/test-only-api.js to load Chrome-specific resources, which are also now available (downloaded automatically) with upstream wpt run chrome (#24979 ), which means you can see the results on wpt.fyi. Furthermore, these tests will not fail on other browsers with an obscure network error/timeout (but with a more meaningful assertion failure), and other browsers can fill in their implementations in this new framework (currently the only example is WebXR).

Lastly, we've removed all *.mojom.js from the WPT tree and added a lint.

@Hexcles Hexcles closed this Sep 16, 2020
@Hexcles
Copy link
Member

@Hexcles Hexcles commented Sep 16, 2020

And huge thanks to our friends from Intel @arskama @rakuco for migrating many device-related tests!

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
9 participants
You can’t perform that action at this time.