1480454 - Various fixes for wpt-in-jsshell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Add a wasm setting to the wpt .ini expectation files

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Improve assertion in _emit_manifest_at

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Give RefTestCase a useful __repr__ implementation

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d: Don't run wpt jstests in the jsreftest harness

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e: Stop running disabled wpt tests in jstests.

Comment 6

[James Graham [:jgraham]]

Comment on attachment 8997068 [details] [diff] [review]
Part a: Add a wasm setting to the wpt .ini expectation files

I guess I imagined an actual command line option. Could be worth adding a comment about who uses the wasm value.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8997069 [details] [diff] [review]
Part b: Improve assertion in _emit_manifest_at

Review of attachment 8997069 [details] [diff] [review]:
-----------------------------------------------------------------

wow, it seems we'd almost want a linter that errors if there's any use of the pdb global variable. Thanks.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8997071 [details] [diff] [review]
Part d: Don't run wpt jstests in the jsreftest harness

Review of attachment 8997071 [details] [diff] [review]:
-----------------------------------------------------------------

Does this mean that a JS WPT test will run only in the browser and not in the JS shell? We'd actually like to have them run in both environments, even if it's a waste in resources consumption, for a few reasons:

- a WPT test failing in the shell will tell us the equivalent WPT browser test needs to be updated too (so we don't need to always run WPT in try builds).
- more generally, JS engine hackers mostly use the shell for testing purposes. 
- some behaviors are different in a browser vs in the shell. At least I can remember one or two cases where the JS shell tests were passing and the equivalent browser tests were not, so it's been useful in the past.

Does it make sense, or am I misunderstanding this patch?

Comment 9

[James Graham [:jgraham]]

Comment on attachment 8997072 [details] [diff] [review]
Part e: Stop running disabled wpt tests in jstests.

Review of attachment 8997072 [details] [diff] [review]:
-----------------------------------------------------------------

I expect this is fine, but I'd like a comment that explains the intent.

::: js/src/tests/jstests.py
@@ +382,5 @@
>  
>      return [
>          RefTestCase(
>              wpt,
> +            strip_leading_slash(test.url),

I'm not really sure what the intent of this change is. It seems we are going from passing in a path, which may contain several tests, to passing in a url with path, query and fragment but without a leading slash. Which might be fine? I'm not sure.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8997071 [details] [diff] [review]
Part d: Don't run wpt jstests in the jsreftest harness

No, the manifests are only used for js*ref*tests, i.e., the way we usually run jstests in the browser. With this change, we keep running these tests in the shell, but don't run them in two different harnesses (jsreftest and wpt) in the browser.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8997071 [details] [diff] [review]
Part d: Don't run wpt jstests in the jsreftest harness

Review of attachment 8997071 [details] [diff] [review]:
-----------------------------------------------------------------

Ha, I missed the jsreftest bits, thanks.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to James Graham [:jgraham] from comment #9)
> Comment on attachment 8997072 [details] [diff] [review]
> Part e: Stop running disabled wpt tests in jstests.
> 
> Review of attachment 8997072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I expect this is fine, but I'd like a comment that explains the intent.
> 
> ::: js/src/tests/jstests.py
> @@ +382,5 @@
> >  
> >      return [
> >          RefTestCase(
> >              wpt,
> > +            strip_leading_slash(test.url),
> 
> I'm not really sure what the intent of this change is. It seems we are going
> from passing in a path, which may contain several tests, to passing in a url
> with path, query and fragment but without a leading slash. Which might be
> fine? I'm not sure.

The thing is that loader.iter_tests() also returns disabled tests. loader.tests has removed those (they're in loader.disabled_tests), but it also drops the test_path I was using before. The closest equivalent seems to be test.url, but that has a leading slash that would cause the string to be interpreted as relative to the file system root.

I suppose there could be an issue with variants, but in that case I'm screwed anyway, because the shell doesn't support the location object to figure out which variant is running. I could perhaps add a lint for that.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #12)
> (In reply to James Graham [:jgraham] from comment #9)
> > Comment on attachment 8997072 [details] [diff] [review]
> > Part e: Stop running disabled wpt tests in jstests.
> > 
> > Review of attachment 8997072 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I expect this is fine, but I'd like a comment that explains the intent.
> > 
> > ::: js/src/tests/jstests.py
> > @@ +382,5 @@
> > >  
> > >      return [
> > >          RefTestCase(
> > >              wpt,
> > > +            strip_leading_slash(test.url),
> > 
> > I'm not really sure what the intent of this change is. It seems we are going
> > from passing in a path, which may contain several tests, to passing in a url
> > with path, query and fragment but without a leading slash. Which might be
> > fine? I'm not sure.
> 
> The thing is that loader.iter_tests() also returns disabled tests.
> loader.tests has removed those (they're in loader.disabled_tests), but it
> also drops the test_path I was using before. The closest equivalent seems to
> be test.url, but that has a leading slash that would cause the string to be
> interpreted as relative to the file system root.
> 
> I suppose there could be an issue with variants, but in that case I'm
> screwed anyway, because the shell doesn't support the location object to
> figure out which variant is running. I could perhaps add a lint for that.

Talked on IRC and I'm using test.path instead of test.url.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/009f3d9799b6
Part a: Add a wasm setting to the wpt .ini expectation files; r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ed2387d4a4
Part b: Improve assertion in _emit_manifest_at; r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2e6da29d5f
Part c: Give RefTestCase a useful __repr__ implementation; r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b60feef3ce6
Part d: Don't run wpt jstests in the jsreftest harness; r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd2bfad3d7a
Part e: Stop running disabled wpt tests in jstests; r=jgraham

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12333 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/009f3d9799b6
https://hg.mozilla.org/mozilla-central/rev/a0ed2387d4a4
https://hg.mozilla.org/mozilla-central/rev/3c2e6da29d5f
https://hg.mozilla.org/mozilla-central/rev/6b60feef3ce6
https://hg.mozilla.org/mozilla-central/rev/dbd2bfad3d7a

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9e2ddc97adc2c2f30ac2cc3bed4d1c1f6db8c7
Bug 1480454 - Part f: Run mach wpt-manifest-update to pick up change from part a.  r=me

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5f9e2ddc97ad