New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 821496 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 719827



Sign in to add a comment

http/wpt layout tests can't use generated mojom JS stubs

Project Member Reported by dcheng@chromium.org, Mar 13 2018

Issue description

https://chromium.googlesource.com/chromium/src/+/628771d2f70705a9aba44a5f994dafdac9fd61ea fixed it so layout tests could use resources in the /gen/ directory.

However, this doesn't work for layout tests that run from a non-file:// URL. We need to figure out a solution for that, since bluetooth would like to use some test-only interfaces to drive test-only stubs.

The current workaround would be to check the generated mojom JS directly into the tree; this makes updating the tests clunky, since you have to update the mojom test interface, build once, then copy the JS file into LayoutTests, and then re-run the tests to make sure everything passes. It feels like we should have a way to do this without checking in generated files...
 
Blocking: 719827
Using the code in this change: https://chromium-review.googlesource.com/c/chromium/src/+/909726

And executing the following command from the src/ directory to run the layout tests:
python third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default --seed 4 --no-show-results --clobber-old-results --exit-after-n-failures 5000 --exit-after-n-crashes-or-timeouts 100 --debug-rwt-logging external/wpt/bluetooth

It produces the following result for all except 3 of the tests in third_party/WebKit/LayoutTest/external/wpt/bluetooth:
10:32:46.046 41700   CONSOLE ERROR: line 8: Not allowed to load local resource: fake_bluetooth_chooser.mojom.js
10:32:46.049 41603 [3/156] external/wpt/bluetooth/characteristic/getDescriptors/gen-descriptor-get-same-object.https.html failed unexpectedly (test timed out)

I have attached a screenshot of the error message.
Issue 821496 example
3.7 MB View Download

Comment 2 by dcheng@chromium.org, Mar 15 2018

Cc: reillyg@chromium.org qyears...@chromium.org
One (slightly crazy idea) would be to have content_shell run an http test server for serving files out of gen in layout test mode. Then the actual Blink HTTP test server would proxy to the test server for paths beginning with /gen/ or something...

Definitely feels quite hacky though, so I'm hoping someone else will have a better idea...
I think it makes sense to add generated mojom.js files to the set of directories currently served by the WPT httpd configuration. The path they are served on (which is by definition Chromium-specific) will be only referenced from the Chromium-specific test resource script.
Cc: robertma@chromium.org
Cc: foolip@chromium.org
We do have a bunch of mojom JS in WPT: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/LayoutTests/external/wpt/resources/chromium/

I'm not sure about the convention of these Chrome-specific bits in WPT. cc foolip@
robertma@, there are some important details to be aware of here. The purpose of putting generated mojom.js files in WPT is so that these tests can be run in wpt.fyi. I have filed an issue with the maintainer to get the appropriate flag passed to Chrome in their infrastructure so that this works:

https://github.com/w3c/wptdashboard/issues/81

The Web Bluetooth tests are a special case because they also require some custom browser process code which is only compiled into content_shell and so won't work in Chrome. This makes the above effort futile for these particular tests. The proposal here is specifically to make the generated mojom.js files available so that these tests don't need to check in these files for no good reason.
> The Web Bluetooth tests are a special case because they also require some custom browser process code which is only compiled into content_shell and so won't work in Chrome.

This sounds to me that these Web Bluetooth tests can't be in WPT, as there is no way to run them using the public binary (even with special flags). And I suppose they also won't run in other browsers. Do I understand correctly?

http/ and external/wpt are in fact very different. The former is served via Apache. It could be possible to add one more rule to the config to make /gen/ available. https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py?l=73&rcl=71f63e241b81bcda8dab9d53478ba6d9ab94a83d  And if these tests aren't meant to be in WPT, then we don't need to worry about the latter.
The architecture of both the WebUSB and Web Bluetooth test suite is to have a JavaScript file (webusb-test.js for example) that provides a "test API" that the tests are written to. The contents of this file are browser-specific but the API it presents is not. If another browser implemented WebUSB and the associated test API then it could run the Web Platform Tests unmodified.

For WebUSB this test API is formally specified: https://wicg.github.io/webusb/test/

Web Bluetooth has a similar document however it is out of date: https://webbluetoothcg.github.io/web-bluetooth/tests.html

For Web Bluetooth, even though Chromium's tests can't run outside of a content_shell build due to the current architecture there has been enough interest in the test suite (from Servo) that we want to have the tests in the WPT repository for the benefit of other implementers.
Cc: -roc...@chromium.org ockot@chromium.org kereliuk@chromium.org
Yeah the WebUSB case makes sense. It'd be great if we can make Web Bluetooth the same eventually (i.e. tests can be run on Chrome with some extra flags).

For now, despite a bit hacky, it might be possible to make /gen/ available through wptserve in Blink (content_shell). Those WPT tests will then fail in Chrome in the upstream (wpt.fyi), but as you said might be used by Servo folks. @foolip WDYT about the value/prioritization? Shall we put these tests in WPT (to share with Servo) and add some magic in Blink's wptserve setup to make them work?

Also +kereliuk who works on testdriver.js
Cc: -ockot@chromium.org roc...@chromium.org
Oops sorry I accidentally deleted in a char in the CC list
Cc: ddorwin@chromium.org bajones@chromium.org offenwanger@chromium.org billorr@chromium.org
Labels: Proj-XR
Are there any thoughts on addressing the impact of internal changes in tip of tree on wpt.fyi results until the new version of Chrome reaches stable?

This will also affect WebXR tests.
Re #11 sorry I don't fully understand your comment. Do WebXR layout tests also require mojom JS? And you'd like to see the results on wpt.fyi?
That was two separate thoughts.

Does this issue affect all WPTs that rely on mojom JS? For example, both WebUSB and Web Bluetooth. Only the latter was specifically mentioned. If so, then that will also affect the WebXR WPTs, which we are starting to add.

The other thought was that wpt.fyi uses Chrome stable but we are uploading the mojom JS files from tip of tree. Thus, the files we upload will typically be two versions off from the version under test, right? If so, we may break (previously passing) wpt.fyi results for ~12 weeks by simply doing internal refactoring. If this is all true, then we _may_ want to consider that in the solution to the local developer problem originally reported.
Yes this affects all tests that rely on mojom JS. Let me explain the current situations in Blink and wpt.fyi separately:

In Blink CI (using content_shell), /gen/ is not available to wptserve. In order to run WPT that depend on mojom JS, some generated JS stubs are checked into external/wpt. The original feature request is to make /gen/ available to wptserve in Blink CI.

On wpt.fyi (using released Chrome binaries), currently neither WebUSB or Web Bluetooth tests work. WebUSB requires a special test-only command line flag that's not enabled yet on wpt.fyi, while Web Bluetooth requires special code that's only available in content_shell (i.e. the tests will fail on wpt.fyi anyway regardless of whether the generated JS is checked in).

What's the dependencies of the planned WebXR tests? Mojom JS stubs? Special flags? Code that's only available in content_shell? And yes, checking in generated files will cause the version lock-in issue you described, so even if mojom JS is the only dependency of the coming WebXR tests, it'd still be difficult to make them consistently green on wpt.fyi.



Also regarding the original feature request, I'll give it a try if I find some time. I think there are only benefits and no downsides to make /gen/ available to wptserve in Blink; Web Bluetooth tests don't work with released binaries anyways so there's no concern of breaking the public upstream.
At least currently, WebXR tests just require the Mojom JS and normal flags (--enable-blink-features=MojoJS,MojoJSTest).
Owner: robertma@chromium.org
Status: Started (was: Available)
https://chromium-review.googlesource.com/c/chromium/src/+/1140908

BTW, turns out /gen/ is already available in http tests.
Whatever we do for providing /gen/ for wptserve should not break running tests tests on wpt.fyi with a stable build of Chrome. We are very close to getting the necessary flags passed to Chrome so that this works: https://github.com/web-platform-tests/results-collection/issues/81
Re #17: no, the implementation I chose shouldn't affect upstream WPT (including wpt.fyi) at all. BTW, just to confirm: is that flag the only missing requirement to make webust tests work on wpt.fyi?
Correct, that flag is the only missing requirement to make the WebUSB tests work on wpt.fyi.
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9e49db768da26da0c543d89b61a10c253be7982

commit d9e49db768da26da0c543d89b61a10c253be7982
Author: Robert Ma <robertma@chromium.org>
Date: Wed Jul 18 21:17:03 2018

Make /gen/ available in wptserve

/gen/ is already available in both file tests (via blink_test_runner.cc)
and http tests (via Apache 'Alias' directives). This CL makes /gen/ also
available to wptserve in Blink (both run_web_tests and
run_blink_wptserve).

Besides, this CL adds configuration options (release, debug, etc.) to
run_blink_wptserve so that users can control which out/*/gen is served.

Also fix a leaking file descriptor in filesystem.open_text_tempfile.

Bug: 821496
Change-Id: I60ae0657df470dd319d002bc1c20476d33d9c05e
Reviewed-on: https://chromium-review.googlesource.com/1140908
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576210}
[modify] https://crrev.com/d9e49db768da26da0c543d89b61a10c253be7982/third_party/blink/tools/blinkpy/common/system/filesystem.py
[modify] https://crrev.com/d9e49db768da26da0c543d89b61a10c253be7982/third_party/blink/tools/blinkpy/web_tests/servers/cli_wrapper.py
[modify] https://crrev.com/d9e49db768da26da0c543d89b61a10c253be7982/third_party/blink/tools/blinkpy/web_tests/servers/wptserve.py
[modify] https://crrev.com/d9e49db768da26da0c543d89b61a10c253be7982/third_party/blink/tools/blinkpy/web_tests/servers/wptserve_unittest.py

Status: Assigned (was: Started)
Thanks! I'll assign to myself to finish cleaning up the remaining .mojom.js files that are currently checked in.
Owner: dcheng@chromium.org
(Or you may file a new bug for that. Up to you.)
dcheng@, I'm coming across this via https://foolip.github.io/ecosystem-infra-rotation/ a P2 Blink>Infra bug with no activity in a while. Do you have an update, are you still working on this?
I'll create sub-bugs for the projects that have checked in mojom files to track the process of updating things.
Cc: -roc...@chromium.org rockot@google.com

Sign in to add a comment