1340583 - Support test262's module tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Extend the jstests harness to support module tests and then enable it to run test262's module tests.

Comment 1

[André Bargull [:anba]]

Attachment: bug1340583-part1-move-static-skipif.patch

Applies on top of bug 1339817.

This moves the static skip-ifs from the import script to jstests.list to reduce the modifications in the imported test files.

Comment 2

[André Bargull [:anba]]

Attachment: bug1340583-part2-prepare-test262-importer.patch

Restructures some parts of the test262 import script, so it's easier to support proper error detection and module support for part 3 and 4.

Comment 3

[André Bargull [:anba]]

Attachment: bug1340583-part3-error-type-support.patch

The jstests harness allows to specify negative tests by appending "-n" to the file name. This approach doesn't work anymore for module tests, because import statements in module test expect the original file name. To work around this limitation I've extended jstests to allow to specify an error type in the |reftest| manifest, similar to the error declaration in the |jit-test| header for jit-tests.

Comment 4

[André Bargull [:anba]]

Attachment: bug1340583-part4-module-test-support.patch

With this change it's now possible to declare tests as module files by adding "module" to the |reftest| header. Again, this works similar to how jit-tests declare a test file as module code. 

The browser.js changes were particularly interesting, I wonder if there's a better to make this work.

Comment 5

[André Bargull [:anba]]

Attachment: bug1340583-part5-regenerate-test-files.patch

Regenerates the test262 files to apply the changes from the previous parts.

Comment 6

[André Bargull [:anba]]

Attachment: bug1340583-part6-update-exclusion-list.patch

Updates the test262 exclusion list.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8838733 [details] [diff] [review]
bug1340583-part3-error-type-support.patch

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

::: js/src/tests/browser.js
@@ +336,3 @@
>      testcase.passed = true;
>    }
> +  if (href.indexOf('error=') !== -1) {

Should this be an |else if|? Can the two styles of negation be meaningfully combined?

@@ +338,5 @@
> +  if (href.indexOf('error=') !== -1) {
> +    // Negative test which expects a specific error type.
> +    var startIndex = href.indexOf('error=');
> +    var endIndex = href.indexOf(';', startIndex);
> +    if (endIndex < 0)

Nit: test for -1 for consistency with the other tests.

::: js/src/tests/lib/manifest.py
@@ +180,5 @@
>      line.append("script")
> +    script = script_name
> +    if properties:
> +        script = ";".join([script] + properties)
> +    line.append(script)

Are there plans to add other properties in the near future? If not, would be clearer to just append the "error=" string directly instead of going through some properties array.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8838737 [details] [diff] [review]
bug1340583-part4-module-test-support.patch

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

Looks good, though I'd like to know the answers to the questions before r+.

::: js/src/tests/browser.js
@@ -549,5 @@
>    document.write('<title>' + properties.test + '<\/title>');
>  
> -  // XXX bc - the first document.written script is ignored if the protocol
> -  // is file:. insert an empty script tag, to work around it.
> -  document.write('<script></script>');

Haha, I wonder if this is still true.

@@ +588,5 @@
> +    }
> +
> +    for (var i = 0; i < scripts.length; i++) {
> +      var src = scripts[i].src;
> +      document.write(`<script src="${src}" charset="utf-8" ${key}="${value}"><\/script>`);

Oo, template strings.

@@ +594,5 @@
> +  } else {
> +    // Modules are loaded asynchronously by default, but for the test harness
> +    // we need to execute all scripts and modules one after the other.
> +
> +    // Saved built-ins (TODO: Move this function into the IIFE).

What IIFE?

@@ +622,5 @@
> +      }
> +      script.src = spec.src;
> +
> +      // XXX: Enabling async triggers the following assertion in nsScriptLoader
> +      // "Non-XSLT Defer script on a document without an active parser; bug 592366."

Is this a bug in our module loading code? That is, are modules supposed to be able to load synchronously? I don't understand that error. Is there a bug on file for the module-related use case?

@@ +632,5 @@
> +        script.addEventListener("afterscriptexecute", callNextAppend, {once: true});
> +
> +        // Module scripts don't fire the "afterscriptexecute" event when there
> +        // was an error, instead the "error" event is emitted. So listen for
> +        // both events when creating module scripts.

Is this also by spec, or a bug in our impl?

Additionally, if an error was raised, why do we need to continue appending script elements instead of determining the test passed/failed?

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8838738 [details] [diff] [review]
bug1340583-part5-regenerate-test-files.patch

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

rs=me

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8838739 [details] [diff] [review]
bug1340583-part6-update-exclusion-list.patch

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

Your filing bugs and organizing this list is amazing, thank you.

Comment 11

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #7)
> ::: js/src/tests/browser.js
> @@ +336,3 @@
> >      testcase.passed = true;
> >    }
> > +  if (href.indexOf('error=') !== -1) {
> 
> Should this be an |else if|? Can the two styles of negation be meaningfully
> combined?

Probably not, I'll change it to an |else if|.

Comment 12

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #8)
> ::: js/src/tests/browser.js
> @@ -549,5 @@
> >    document.write('<title>' + properties.test + '<\/title>');
> >  
> > -  // XXX bc - the first document.written script is ignored if the protocol
> > -  // is file:. insert an empty script tag, to work around it.
> > -  document.write('<script></script>');
> 
> Haha, I wonder if this is still true.

I'll file a follow-up bug to investigate this.

> @@ +594,5 @@
> > +  } else {
> > +    // Modules are loaded asynchronously by default, but for the test harness
> > +    // we need to execute all scripts and modules one after the other.
> > +
> > +    // Saved built-ins (TODO: Move this function into the IIFE).
> 
> What IIFE?

The one at https://dxr.mozilla.org/mozilla-central/rev/c7b015c488cfb2afbcff295a9639acd85df332f8/js/src/tests/browser.js#199-309

> 
> @@ +622,5 @@
> > +      }
> > +      script.src = spec.src;
> > +
> > +      // XXX: Enabling async triggers the following assertion in nsScriptLoader
> > +      // "Non-XSLT Defer script on a document without an active parser; bug 592366."
> 
> Is this a bug in our module loading code? That is, are modules supposed to
> be able to load synchronously? I don't understand that error. Is there a bug
> on file for the module-related use case?

I've filed bug 1340865 for this assertion error. 

The default loading mode for module scripts seems to be similar to <script defer>, if I interpret the diagram at https://html.spec.whatwg.org/multipage/scripting.html#the-script-element (https://html.spec.whatwg.org/images/asyncdefer.svg) correctly. For the test harness to work, we need to emulate the loading behaviour of classic scripts, i.e. blocking the html parser while fetching and executing the module. So I guess using the "async" attribute wouldn't help us at all?


> 
> @@ +632,5 @@
> > +        script.addEventListener("afterscriptexecute", callNextAppend, {once: true});
> > +
> > +        // Module scripts don't fire the "afterscriptexecute" event when there
> > +        // was an error, instead the "error" event is emitted. So listen for
> > +        // both events when creating module scripts.
> 
> Is this also by spec, or a bug in our impl?

Ms2ger once mentioned that (after|before)scriptexecute are no longer part of the HTML spec, but AFAICT there's no suitable replacement resp. I don't know if there's a suitable replacement. We're basically just waiting for the DOM people to fix our misdeeds. :-)
http://logs.glob.uno/?c=mozilla%23jsapi&s=15+Nov+2016&e=15+Nov+2016#c809583

> Additionally, if an error was raised, why do we need to continue appending
> script elements instead of determining the test passed/failed?

https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js-test-driver-end.js needs to be loaded last to inform the test runner that all tests have finished, even after an error was raised.

Comment 13

[André Bargull [:anba]]

(In reply to André Bargull from comment #12)
> > Additionally, if an error was raised, why do we need to continue appending
> > script elements instead of determining the test passed/failed?
> 
> https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js-test-driver-
> end.js needs to be loaded last to inform the test runner that all tests have
> finished, even after an error was raised.

And I'm not sure if it's possible/valid to directly call jsTestDriverEnd() in window.onerror and the module "onerror" handler, if there are still pending jobs or HTML tasks (e.g. some tests are calling window.setTimeout()).

Comment 14

[André Bargull [:anba]]

Attachment: bug1340583-part1-move-static-skipif.patch

Updated to fix reviewer name in commit message, otherwise no changes. Carrying r+ from shu.

Comment 15

[André Bargull [:anba]]

Attachment: bug1340583-part3-error-type-support.patch

Updated per review comments, carrying r+ from shu.

Comment 16

[André Bargull [:anba]]

Attachment: bug1340583-part5-regenerate-test-files.patch

Update to change review marker from r to rs in commit message, carrying r+.

Comment 17

[André Bargull [:anba]]

Attachment: bug1340583-part6-update-exclusion-list.patch

Update exclusion list because two module bugs have already been fixed, also merged exclusions for async generators and added bugzilla link for SIMD failures. Carrying r+.

Comment 18

[André Bargull [:anba]]

Attachment: bug1340583-part1-move-static-skipif.patch

Updating part 1 to apply cleanly on inbound, carrying r+.

Comment 19

[André Bargull [:anba]]

Attachment: bug1340583-part4-module-test-support.patch

Removed the --module-load-path workaround from js/src/tests/lib/tests.py because the shell module loader has been updated to be able to load modules from relative paths.

Comment 20

[André Bargull [:anba]]

Attachment: bug1340583-part6-update-exclusion-list.patch

Updating part 6 because we've fixed a couple of module bugs in the mean time. Carrying r+ from shu.

Comment 21

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=781ef5f37100f6608a69acf54b8cb3e5deedbd20

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0579aeea019
Part 1: Move static skip-if statements to the top-level jstests.list. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/697f87b0c34e
Part 2: Change some test262 importer functions in preparation for next patches. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f315db9d404
Part 3: Extend jstests reftest line to allow to define error types. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/414e8ae08ba6
Part 4: Add support for module tests to jstests. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/966c7ce57282
Part 5: Regenerate test262 files. rs=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bbd04e33a93
Part 6: Update test262 exclusion list. r=shu

Comment 23

[André Bargull [:anba]]

(In reply to André Bargull from comment #12)
> (In reply to Shu-yu Guo [:shu] from comment #8)
> > ::: js/src/tests/browser.js
> > @@ -549,5 @@
> > >    document.write('<title>' + properties.test + '<\/title>');
> > >  
> > > -  // XXX bc - the first document.written script is ignored if the protocol
> > > -  // is file:. insert an empty script tag, to work around it.
> > > -  document.write('<script></script>');
> > 
> > Haha, I wonder if this is still true.
> 
> I'll file a follow-up bug to investigate this.

Filed bug 1345600.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b0579aeea019
https://hg.mozilla.org/mozilla-central/rev/697f87b0c34e
https://hg.mozilla.org/mozilla-central/rev/9f315db9d404
https://hg.mozilla.org/mozilla-central/rev/414e8ae08ba6
https://hg.mozilla.org/mozilla-central/rev/966c7ce57282
https://hg.mozilla.org/mozilla-central/rev/6bbd04e33a93