837950 - Import test suite for ECMA-402, ECMAScript Internationalization API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Norbert Lindenberg]

Import the conformance tests for ECMA-402, ECMAScript Internationalization API, into one of the SpiderMonkey test suites, along with any parts of the test262 harness that are required to run the tests.

See
http://hg.ecmascript.org/tests/test262/file/927ea8563f7f/test/suite/intl402
http://hg.ecmascript.org/tests/test262/file/927ea8563f7f/test/harness

Comment 1

[Norbert Lindenberg]

Attachment: Update license.html

See bug 843748.

Comment 2

[Norbert Lindenberg]

Attachment: Script to import ECMA-402 conformance tests into the Mozilla tree

Inspired (again) by update-breakpad.sh.

Comment 3

[Norbert Lindenberg]

Attachment: Import test402 source files

Comment 4

[Norbert Lindenberg]

Attachment: Enhance jstests framework to support Test402 tests

Bob, Terrence, it seems together you're the authors of most of the jstests framework, so I hope you can review this.

I'm integrating the tests of test402, the conformance test suite for standard ECMA-402, ECMAScript Internationalization API Specification, into jstests. These tests are originally written to run within the test262 harness, which also runs the conformance test suite for standard ECMA-262, ECMAScript Language Specification.


Running the tests within jstests presents a few problems:

1. The Test262 test suite uses the convention that test cases throw an exception to indicate failure; running to the end without an exception indicates success. This allows for quite simple test cases. The jstests test suite requires test cases to call reportCompare() with equal expected and actual arguments to indicate success; any test case that doesn't do that is considered to have failed.

2. Test402 test cases rely on several JavaScript library files providing test support functions; they call Test262 harness function $INCLUDE() to load them.

3. Test402 test cases and the library files define a variety of global functions and variables, which may clash with the global functions and variables defined by the jstests harness. At least in the case of "options" this is sufficient to cause the jstests harness to malfunction.

4. Some Test402 test cases are destructive: They modify default built-in ECMAScript properties to verify that the functions being tested continue to behave as specified in this situation. At least in the case of modifications to Array.prototype this is sufficient to cause the jstests harness to malfunction.

5. Test402 test cases should be imported without modifications, so there needs to be an alternative mechanism to specify reftest failure types (such as "skip-if(condition)").


The patch addresses these issues as follows:

1. The jstests harness now provides an API function testPassesUnlessItThrows(), which, if called while setting up the execution environment for a test case, indicates that the test should be considered to have passed if it runs to completion without throwing an exception.

2. The jstests harness for the browser environment now provides an API function include(), which, if called while setting up the execution environment for a test case, loads the requested JavaScript file for use by the test case.

3. The jstests harness variable options is saved as jstestsOptions and restored after the test case terminates. A clean solution would be to move the jstests harness into its own namespace object, but some of its variables are widely used by test cases, so this would be a larger project.

4. The jstests harness now provides an API function setRestoreFunction(), which accepts a function that restores the standard built-in ECMAScript properties, and which will be called after the test case terminates.

5. A new jstests.list manifest file can be used to define reftest includes and test items externally to directories of test cases.

The patch also includes the files jstests.list, test402/browser.js, and test402/shell.js, which take advantage of these new capabilities.


The ECMAScript Internationalization API isn't enabled in current builds yet, but a try run is available at
https://tbpl.mozilla.org/?tree=Try&rev=d20b02d1dfc2
(I added a few more test cases after this try run, some of which show up in jstests.list, but it's close enough.)

Comment 5

[Bob Clary [:bc] (inactive)]

Luke, Waldo: would you rather review this is my stead?

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 728872 [details] [diff] [review]
Enhance jstests framework to support Test402 tests

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

(In reply to Norbert Lindenberg from comment #4)
> Created attachment 728872 [details] [diff] [review]
> Enhance jstests framework to support Test402 tests
> 
> Bob, Terrence, it seems together you're the authors of most of the jstests
> framework, so I hope you can review this.

This is mostly all my code, so I'd hope I am adequate to review these changes.

> I'm integrating the tests of test402, the conformance test suite for
> standard ECMA-402, ECMAScript Internationalization API Specification, into
> jstests. These tests are originally written to run within the test262
> harness, which also runs the conformance test suite for standard ECMA-262,
> ECMAScript Language Specification.

Thanks for taking this on. This work should help us to also integrate the jit-tests so it's definitely code I want to see added. I do have quite a few reservations about the current implementation, however, it isn't anything that we can't resolve with a bit more work.

::: js/src/tests/README.txt
@@ +5,5 @@
>  tests for the Spidermonkey engine. Two harnesses run these tests: the shell test
>  harness in this directory and the "reftest" harness built into the browser, used
>  by Tinderbox. The browser reftests require additional manifest files; these are
> +generated automatically by the build phase 'package-tests' using
> +jstests.py --make-manifests

Please rephrase this as "using the '--make-manifests' option to jstests.py."

@@ +34,5 @@
>  
>          <fineprint> Either // or /* */ style comments may be used. The entire
>          comment must appear in the first 512 bytes of the file. The control
>          string must be in its own comment block. </fineprint>
> +        

Strip trailing whitespace.

@@ +38,5 @@
> +        
> +    When adding such comments to individual files is not feasible (e.g., for
> +    imported tests), reftest manifest entries can also be added to jstests.list.
> +    The <type> "include" indicates that options should apply to all
> +    test cases within a directory.

This should also point out that the manifest is merged with the generated manifest and is not the complete manifest.

::: js/src/tests/browser.js
@@ +122,5 @@
> +  jstestsTestPassesUnlessItThrows = false;
> +
> +  // Restore options in case a test case used this common variable name.
> +  options = jstestsOptions;
> +  

Remove trailing whitespace.

@@ +404,5 @@
> +  if (suitepath.indexOf('/') !== -1) {
> +    var base = suitepath.slice(0, suitepath.indexOf('/'));
> +    outputscripttag(base + '/shell.js', properties);
> +    outputscripttag(base + '/browser.js', properties);
> +  }

I'm not sure I entirely understand why this is using indexOf instead of lastIndexOf -- an absolute path would surely not work here? Can you explain what sort of path suitepath is expected to be and why this will load the correct code, even if we change the test directory?

::: js/src/tests/jstests.list
@@ +1,2 @@
> +# Manifest entries for imported test suites whose individual test cases
> +# we don't want to change.

I don't think the syntax here would actually be parsable by the reftests directly -- it certainly would not have been processable by the manifest parser we used internally: it expected |include| to be a top level statement. I'm not really okay with putting these listings in our tree when they have a filename and format that looks deceptively like manifests but have wildly different semantics. Also, the code you've added to handle |include| is quite tricky and appears to be totally unused at the moment. It seems like it would be a much better solution would be to just make all the skip's that you want at the top level and not have any of the include weirdness. Alternatively, since you're only using skip, just make the import script delete the skipped tests and don't make any changes to the test loading.

::: js/src/tests/lib/manifest.py
@@ +275,5 @@
>      testcase.comment = matches.group(4)
>  
>      _parse_one(testcase, xul_tester)
>  
> +def parseExternalManifest(location, xul_tester):

This should be _parse_external_manifest to match the file's style.

@@ +280,5 @@
> +    """
> +    Reads an external manifest file for test suites whose individual test cases
> +    can't be decorated with reftest comments.
> +    """
> +    filename = os.path.join(location, 'jstests.list')

Lets move the computation of the name outside of this file so that the code here only deals with the _parsing_external_manifest and not with the location of the manifest.

@@ +281,5 @@
> +    Reads an external manifest file for test suites whose individual test cases
> +    can't be decorated with reftest comments.
> +    """
> +    filename = os.path.join(location, 'jstests.list')
> +    file = open(filename, 'r')

"file" is the name of a python builtin. It is more canonical to use "fp" for these. Also, this should be in a with-statement to ensure that you don't run out file handles in a garbage-collected python implementation.

with open(filename, 'r') as fp:
    ...

@@ +289,5 @@
> +
> +    comment_re = re.compile(r'^\s*(#.*)?$')
> +    manifest_re = re.compile(r'^\s*([^#]*)\s+(include|script)\s+(\S+)(?:\s*#\s*(.*))?$')
> +    for line in file:
> +        if comment_re.match(line):

It would both more robust and faster to do:

line, _, _ = line.partition('#')
line = line.strip()
if not line:
    continue

@@ +292,5 @@
> +    for line in file:
> +        if comment_re.match(line):
> +            continue
> +        matches = manifest_re.match(line)
> +        if matches:

Please restructure this to reduce the indentation level:

if not matches:
    print(...)
    continue

@@ +356,5 @@
>  
> +        # Failure type and comment for a test case can come from
> +        # - an external manifest entry for the test case,
> +        # - an external manifest entry for a containing directory,
> +        # - most commonly: the header of the test case itself.

Please move this comment up to the comments for the method.

@@ +365,5 @@
> +                found = True
> +                break
> +
> +        if found:
> +            continue

This is really gross and inefficient. Please return externalManifestTests as {testcase.path: testcase for testcase in tests} so that you can do:

# The TestCase was already loaded from a manifest.
if filename in externalManifestTests:
    tests.append(externalManifestTests[filename])
    continue

@@ +376,5 @@
> +                testcase.comment = directory["comment"]
> +                _parse_one(testcase, xul_tester)
> +                found = True
> +                break
> +        if not found:

The non-local requirement that |found| happens to be false is truly shudder inducing. If you need to handle state like this at least re-assign it at the loop head so that it is clear to the reader what is going on and to make it easier to move the code later. However...

@@ +377,5 @@
> +                _parse_one(testcase, xul_tester)
> +                found = True
> +                break
> +        if not found:
> +            _parse_test_header(fullpath, testcase, xul_tester)

...since you don't expect these sets to intersect (and even if we did), it would be fine to unconditionally _parse_test_header: you're just going to be over-writing fields it set at worst. I'd want this to look something like:

testcase = TestCase(os.path.join(reldir, filename))
_parse_test_header(fullpath, testcase, xul_tester)
_apply_external_manifests(testcase, externalManifestsDirs)

::: js/src/tests/lib/results.py
@@ +64,5 @@
>                  msg = line[len(' PASSED! '):]
>                  results.append((cls.PASS, msg))
> +            elif line == ' STARTED TEST CASE THAT PASSES UNLESS IT THROWS ':
> +                # This is used by test cases for which normal completion means
> +                # success, and failure is indicated by an exception.

Why are we not adding these tests to the results list? Note: the actual failure-ness of the test is determined solely by the return code. It is perfectly normal for a test to report "PASSED!" then fail afterwards by crashing or excepting, e.g. in the shutdown GC or a JSContext deletion.

For the shell, rc==3 is "an exception was thrown." This gets reported as FAIL, rather than CRASH, which is the behavior you want, if I'm reading correctly. If we just emit ' PASSED! ' for the 402 tests then things should just work.

::: js/src/tests/shell.js
@@ +48,5 @@
> + * Signals to results.py that the current test case should be considered to
> + * have passed if it doesn't throw an exception.
> + * This avoids the need to add otherwise useless calls to reportCompare to
> + * every test case in an imported test suite that assumes that test cases pass
> + * if they run to completion.

I think it's fine to leave off this second sentence.

@@ +50,5 @@
> + * This avoids the need to add otherwise useless calls to reportCompare to
> + * every test case in an imported test suite that assumes that test cases pass
> + * if they run to completion.
> + *
> + * This function is replaced in browser.js.

This is a bit confusingly worded as it's only replaced sometimes. How about "When the test suite is run in the browser, this function gets overridden by the same-named function in browser.js."

@@ +53,5 @@
> + *
> + * This function is replaced in browser.js.
> + */
> +function testPassesUnlessItThrows() {
> +  print(' STARTED TEST CASE THAT PASSES UNLESS IT THROWS ');

This string is a bit absurdly over-long. How about...

::: js/src/tests/test402/browser.js
@@ +24,5 @@
> +}()));
> +
> +/*
> + * Loading include files into the browser from a script so that they become
> + * synchronously available to that same script is 難しい. Instead, request

If I ask for a translation of this, will I regret it? Please rephrase this to something more like "Pre-load test402's library code."

::: js/src/tests/test402/shell.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +

Remove one of these \n.

Comment 7

[Norbert Lindenberg]

Attachment: Enhance jstests framework to support Test402 tests

Updated per comment 6. Try build:
https://tbpl.mozilla.org/?tree=Try&rev=af3dddfe0f62

A general comment on manifests: The full specification for manifests, referenced by the README file, is layout/tools/reftest/README.txt. It seems some of comment 6 arises because the existent manifest parser in jstests and my addition support different subsets of the manifest format. I think in both cases reasonable subsets were chosen for the task at hand, and support for the full manifest language would greatly complicate the code for no benefit.

(In reply to Terrence Cole [:terrence] from comment #6)

> ::: js/src/tests/README.txt
> @@ +38,5 @@
> > +        
> > +    When adding such comments to individual files is not feasible (e.g., for
> > +    imported tests), reftest manifest entries can also be added to jstests.list.
> > +    The <type> "include" indicates that options should apply to all
> > +    test cases within a directory.
> 
> This should also point out that the manifest is merged with the generated
> manifest and is not the complete manifest.

Not quite. I updated this as well as the pre-existing documentation, trying to be more precise in what really happens, including the changes made for later comments.

> ::: js/src/tests/browser.js
> @@ +404,5 @@
> > +  if (suitepath.indexOf('/') !== -1) {
> > +    var base = suitepath.slice(0, suitepath.indexOf('/'));
> > +    outputscripttag(base + '/shell.js', properties);
> > +    outputscripttag(base + '/browser.js', properties);
> > +  }
> 
> I'm not sure I entirely understand why this is using indexOf instead of
> lastIndexOf -- an absolute path would surely not work here? Can you explain
> what sort of path suitepath is expected to be and why this will load the
> correct code, even if we change the test directory?

Are absolute paths allowed here? I couldn't find any documentation on running jstests in a browser; I tried a few ways, but the only environment where I could get it to work is the try server, and there absolute paths aren't used. The intent here is to enable a test suite that has more than two levels of directories to provide browser.js and shell.js in its base directory.

> ::: js/src/tests/jstests.list
> @@ +1,2 @@
> > +# Manifest entries for imported test suites whose individual test cases
> > +# we don't want to change.
> 
> I don't think the syntax here would actually be parsable by the reftests
> directly -- it certainly would not have been processable by the manifest
> parser we used internally: it expected |include| to be a top level
> statement.

It's a proper subset of the full reftests manifest format, so the reftests parser should be able to parse it. It's a different subset than that implemented in the jstests manifest parser, so that one would fail.

> I'm not really okay with putting these listings in our tree when
> they have a filename and format that looks deceptively like manifests but
> have wildly different semantics.

Not sure what you mean by "wildly different" - it's a subset.

> Also, the code you've added to handle
> |include| is quite tricky and appears to be totally unused at the moment.

It's used for lines 4 and 5 of jstests.list.

> It seems like it would be a much better solution would be to just make all the
> skip's that you want at the top level and not have any of the include
> weirdness.

Not sure what you mean - how would you rewrite lines 4 and 5?

> Alternatively, since you're only using skip, just make the import
> script delete the skipped tests and don't make any changes to the test
> loading.

Line 4 is using a skip-if. For the skips, I'd like to keep the currently failing tests in sight and available for testing the upcoming fixes (these are not obsolete tests), and the files in test402/lib are include files that are needed to run most of the test cases but don't make sense to run by themselves.

> ::: js/src/tests/lib/manifest.py
> @@ +377,5 @@
> > +                _parse_one(testcase, xul_tester)
> > +                found = True
> > +                break
> > +        if not found:
> > +            _parse_test_header(fullpath, testcase, xul_tester)
> 
> ...since you don't expect these sets to intersect (and even if we did), it
> would be fine to unconditionally _parse_test_header: you're just going to be
> over-writing fields it set at worst. I'd want this to look something like:
> 
> testcase = TestCase(os.path.join(reldir, filename))
> _parse_test_header(fullpath, testcase, xul_tester)
> _apply_external_manifests(testcase, externalManifestsDirs)

The "even if we did" part makes me think this is not a good idea: If we get two manifest statements, _parse_one would combine them in a hard-to-predict fashion; most likely not reflecting the semantics specified for include statements in the manifest specification. I think it's clearer to say that only one of them is used.

> ::: js/src/tests/lib/results.py
> @@ +64,5 @@
> >                  msg = line[len(' PASSED! '):]
> >                  results.append((cls.PASS, msg))
> > +            elif line == ' STARTED TEST CASE THAT PASSES UNLESS IT THROWS ':
> > +                # This is used by test cases for which normal completion means
> > +                # success, and failure is indicated by an exception.
> 
> Why are we not adding these tests to the results list? Note: the actual
> failure-ness of the test is determined solely by the return code. It is
> perfectly normal for a test to report "PASSED!" then fail afterwards by
> crashing or excepting, e.g. in the shutdown GC or a JSContext deletion.

To use your terminology, that seems gross to me, but OK.

> For the shell, rc==3 is "an exception was thrown." This gets reported as
> FAIL, rather than CRASH, which is the behavior you want, if I'm reading
> correctly. If we just emit ' PASSED! ' for the 402 tests then things should
> just work.

It works.

> ::: js/src/tests/test402/browser.js
> @@ +24,5 @@
> > +}()));
> > +
> > +/*
> > + * Loading include files into the browser from a script so that they become
> > + * synchronously available to that same script is 難しい. Instead, request
> 
> If I ask for a translation of this, will I regret it?

Probably not: It means "difficult", also used as "too difficult to consider doing" or "no way".

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 729951 [details] [diff] [review]
Enhance jstests framework to support Test402 tests

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

(In reply to Norbert Lindenberg from comment #7)
> Created attachment 729951 [details] [diff] [review]
> Enhance jstests framework to support Test402 tests
> 
> Updated per comment 6. Try build:
> https://tbpl.mozilla.org/?tree=Try&rev=af3dddfe0f62

This is looking much better.
 
> A general comment on manifests: The full specification for manifests,
> referenced by the README file, is layout/tools/reftest/README.txt.

Ah, thank you for reminding me of that document!

> It seems
> some of comment 6 arises because the existent manifest parser in jstests and
> my addition support different subsets of the manifest format. I think in
> both cases reasonable subsets were chosen for the task at hand, and support
> for the full manifest language would greatly complicate the code for no
> benefit.

Indeed. However, it seems that if we pick a slightly broader subset, we can simplify the core algorithm expressed in |load| substantially. A great deal of the complexity comes from tracking where we found the information. More on this below.

> (In reply to Terrence Cole [:terrence] from comment #6)
> 
> > ::: js/src/tests/README.txt
> > @@ +38,5 @@
> > > +        
> > > +    When adding such comments to individual files is not feasible (e.g., for
> > > +    imported tests), reftest manifest entries can also be added to jstests.list.
> > > +    The <type> "include" indicates that options should apply to all
> > > +    test cases within a directory.
> > 
> > This should also point out that the manifest is merged with the generated
> > manifest and is not the complete manifest.
> 
> Not quite. I updated this as well as the pre-existing documentation, trying
> to be more precise in what really happens, including the changes made for
> later comments.
> 
> > ::: js/src/tests/browser.js
> > @@ +404,5 @@
> > > +  if (suitepath.indexOf('/') !== -1) {
> > > +    var base = suitepath.slice(0, suitepath.indexOf('/'));
> > > +    outputscripttag(base + '/shell.js', properties);
> > > +    outputscripttag(base + '/browser.js', properties);
> > > +  }
> > 
> > I'm not sure I entirely understand why this is using indexOf instead of
> > lastIndexOf -- an absolute path would surely not work here? Can you explain
> > what sort of path suitepath is expected to be and why this will load the
> > correct code, even if we change the test directory?
> 
> Are absolute paths allowed here?

I have no idea either.

> I couldn't find any documentation on
> running jstests in a browser; I tried a few ways, but the only environment
> where I could get it to work is the try server, and there absolute paths
> aren't used.

It's possible, but enough of a hassle that we generally just try server everything too. IIRC, you need to manually |make stage-packages|, then point ./layout/tools/reftest/runreftest.py at the generated toplevel manifest.

> The intent here is to enable a test suite that has more than
> two levels of directories to provide browser.js and shell.js in its base
> directory.

Please add a comment to this effect above these lines and also note that absolute paths are not allowed.

> > ::: js/src/tests/jstests.list
> > Also, the code you've added to handle
> > |include| is quite tricky and appears to be totally unused at the moment.
> 
> It's used for lines 4 and 5 of jstests.list.

I was not able to find test402/jstests.list anywhere in your patches. Is it included with the 402 tests?
 
> > It seems like it would be a much better solution would be to just make all the
> > skip's that you want at the top level and not have any of the include
> > weirdness.
> 
> Not sure what you mean - how would you rewrite lines 4 and 5?

skip-if(!this.hasOwnProperty("Intl")) script test402/path/to/script/1.js
skip-if(!this.hasOwnProperty("Intl")) script test402/path/to/script/2.js
...
skip-if(!this.hasOwnProperty("Intl")) script test402/path/to/script/N.js

You would need to automate generation of this manifest in the import script, but it shouldn't be too hard. That said, I'm okay with adding support for the "<failure-type>* include <relative_path>" manifest lines, since I see now that they are actually spec.
 
> > Alternatively, since you're only using skip, just make the import
> > script delete the skipped tests and don't make any changes to the test
> > loading.
> 
> Line 4 is using a skip-if. For the skips, I'd like to keep the currently
> failing tests in sight and available for testing the upcoming fixes (these
> are not obsolete tests), and the files in test402/lib are include files that
> are needed to run most of the test cases but don't make sense to run by
> themselves.

Fair enough.

> > ::: js/src/tests/lib/manifest.py
> > @@ +377,5 @@
> > > +                _parse_one(testcase, xul_tester)
> > > +                found = True
> > > +                break
> > > +        if not found:
> > > +            _parse_test_header(fullpath, testcase, xul_tester)
> > 
> > ...since you don't expect these sets to intersect (and even if we did), it
> > would be fine to unconditionally _parse_test_header: you're just going to be
> > over-writing fields it set at worst. I'd want this to look something like:
> > 
> > testcase = TestCase(os.path.join(reldir, filename))
> > _parse_test_header(fullpath, testcase, xul_tester)
> > _apply_external_manifests(testcase, externalManifestsDirs)
> 
> The "even if we did" part makes me think this is not a good idea: If we get
> two manifest statements, _parse_one would combine them in a hard-to-predict
> fashion; most likely not reflecting the semantics specified for include
> statements in the manifest specification. I think it's clearer to say that
> only one of them is used.

Well, my intuition is that they would apply broadest include to narrowest include, followed by the script entry. The more specific specifications would override the less specific. This matches with what the spec says. From the section describing include:

   However, the failure type on a manifest is combined with the failure
   type on the test (or on a nested manifest) with the rule that the 
   last in the following list wins:  fails, random, skip.  (In other 
   words, skip always wins, and random beats fails.)

Given this specification, we can simply apply all of the matching rules in order and remove the distinction between directories and scripts that complicates the current implementation. I've added comments inline in the code to this effect.

> > ::: js/src/tests/lib/results.py
> > @@ +64,5 @@
> > >                  msg = line[len(' PASSED! '):]
> > >                  results.append((cls.PASS, msg))
> > > +            elif line == ' STARTED TEST CASE THAT PASSES UNLESS IT THROWS ':
> > > +                # This is used by test cases for which normal completion means
> > > +                # success, and failure is indicated by an exception.
> > 
> > Why are we not adding these tests to the results list? Note: the actual
> > failure-ness of the test is determined solely by the return code. It is
> > perfectly normal for a test to report "PASSED!" then fail afterwards by
> > crashing or excepting, e.g. in the shutdown GC or a JSContext deletion.
> 
> To use your terminology, that seems gross to me, but OK.

Yeah, me too, but it's the way the current test harness works. Since it seems to work for you as-is, I'd prefer to not add any more semantics to a test suite that already has too many.

> > For the shell, rc==3 is "an exception was thrown." This gets reported as
> > FAIL, rather than CRASH, which is the behavior you want, if I'm reading
> > correctly. If we just emit ' PASSED! ' for the 402 tests then things should
> > just work.
> 
> It works.

Excellent!
 
> > ::: js/src/tests/test402/browser.js
> > @@ +24,5 @@
> > > +}()));
> > > +
> > > +/*
> > > + * Loading include files into the browser from a script so that they become
> > > + * synchronously available to that same script is 難しい. Instead, request
> > 
> > If I ask for a translation of this, will I regret it?
> 
> Probably not: It means "difficult", also used as "too difficult to consider
> doing" or "no way".

Thank you for the translation.

::: js/src/tests/lib/manifest.py
@@ +295,5 @@
> +            if not matches:
> +                print('warning: unrecognized line in jstests.list: {0}'.format(line))
> +                continue
> +            if matches.group(2) == 'script':
> +                testcase = TestCase(os.path.normpath(matches.group(3)))

Hmm. It occurs to me that this path calculation won't work for nested manifests: this path needs to be relative to the manifest directory itself. Something like:
    path = os.path.normpath(os.path.join(os.path.dirname(filename), matches.group(3)))

Also, it seems that we just need the path and terms and that we can do the same thing here as we do with directories. I think we just need the path and terms. The comments and tag fields were always a bit overkill: I don't think it is important to preserve them for these tests.

@@ +300,5 @@
> +                testcase.tag = None
> +                testcase.terms = matches.group(1)
> +                testcase.comment = comment.strip()
> +                _parse_one(testcase, xul_tester)
> +                tests[testcase.path] = testcase;

I think we can replace this entire branch with:
path = os.path.normpath(os.path.join(os.path.dirname(filename), matches.group(3)))
directories.append({'dirname': path, 'terms': matches.group(1), 'comment': comment.strip()})

@@ +317,5 @@
> +                directory["comment"] = comment.strip()
> +                directories.append(directory)
> +
> +    # if one directory name is a prefix of another, we want the longer one first
> +    directories.sort(key=lambda x: x["dirname"], reverse=True)

We actually want shortest first now, to follow spec.

@@ +318,5 @@
> +                directories.append(directory)
> +
> +    # if one directory name is a prefix of another, we want the longer one first
> +    directories.sort(key=lambda x: x["dirname"], reverse=True)
> +    return tests, directories

We should probably also rename directories to something more generic.

@@ +372,5 @@
> +                _parse_one(testcase, xul_tester)
> +                found = True
> +                break
> +        if not found:
> +            _parse_test_header(fullpath, testcase, xul_tester)

With the above changes, we can now simplify this to:

testcase = TestCase(os.path.join(reldir, filename))
_apply_external_manifests(testcase, externalManifestEntries)
_parse_test_header(fullpath, testcase, xul_tester)

def _apply_external_manifests(testcase, entries):
    for entry in entries:
        if filename.startswith(entry["dirname"]):
            testcase.terms = entry["terms"]
            testcase.comment = entry["comment"]
            _parse_one(testcase, xul_tester)

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 728867 [details] [diff] [review]
Update license.html

Apologies if this is my mistake or bad advice from me, but: about:license only needs to contain information about code shipped in Firefox. As we aren't shipping tests, the info doesn't need to be in there.

Please put a copy of the Ecma license in a LICENSE file in the top-level directory containing the code.

Thanks,

Gerv

Comment 10

[Norbert Lindenberg]

Comment on attachment 728867 [details] [diff] [review]
Update license.html

(In reply to Gervase Markham [:gerv] from comment #9)

> Apologies if this is my mistake or bad advice from me, but: about:license
> only needs to contain information about code shipped in Firefox. As we
> aren't shipping tests, the info doesn't need to be in there.
> 
> Please put a copy of the Ecma license in a LICENSE file in the top-level
> directory containing the code.

Sorry, my fault entirely. The import already includes the Ecma LICENSE file, so we're good here.

Comment 11

[Norbert Lindenberg]

Attachment: Enhance jstests framework to support Test402 tests

Updated per comment 8. Thanks for all the suggestions for manifest.py - it looks a lot simpler now!

(In reply to Terrence Cole [:terrence] from comment #8)

> > > ::: js/src/tests/jstests.list
> > > Also, the code you've added to handle
> > > |include| is quite tricky and appears to be totally unused at the moment.
> > 
> > It's used for lines 4 and 5 of jstests.list.
> 
> I was not able to find test402/jstests.list anywhere in your patches. Is it
> included with the 402 tests?

No. As for all the other test directories, test402/jstests.list is constructed for the browser environment by "jstests.py --make-manifests", and the python/shell environment acts as if it existed.

> > > It seems like it would be a much better solution would be to just make all the
> > > skip's that you want at the top level and not have any of the include
> > > weirdness.
> > 
> > Not sure what you mean - how would you rewrite lines 4 and 5?
> 
> skip-if(!this.hasOwnProperty("Intl")) script test402/path/to/script/1.js
> skip-if(!this.hasOwnProperty("Intl")) script test402/path/to/script/2.js
> ...
> skip-if(!this.hasOwnProperty("Intl")) script test402/path/to/script/N.js
> 
> You would need to automate generation of this manifest in the import script,
> but it shouldn't be too hard.

Ah, OK. But that would mean baking information about the current state of the Intl module into the import script, and modifying and rerunning that script whenever a bug gets fixed.

> That said, I'm okay with adding support for
> the "<failure-type>* include <relative_path>" manifest lines, since I see
> now that they are actually spec.

Good.

> > > ::: js/src/tests/lib/manifest.py
> ::: js/src/tests/lib/manifest.py
> @@ +295,5 @@
> > +            if not matches:
> > +                print('warning: unrecognized line in jstests.list: {0}'.format(line))
> > +                continue
> > +            if matches.group(2) == 'script':
> > +                testcase = TestCase(os.path.normpath(matches.group(3)))
> 
> Hmm. It occurs to me that this path calculation won't work for nested
> manifests: this path needs to be relative to the manifest directory itself.
> Something like:
>     path = os.path.normpath(os.path.join(os.path.dirname(filename),
> matches.group(3)))

Not quite - the paths in TestCase instances are all relative to the main directory of the test suite, while filename comes in as an absolute path. I added a relpath argument for future use.

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 731354 [details] [diff] [review]
Enhance jstests framework to support Test402 tests

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

Righteous! Thanks for all the hard work getting it to this polished state.

(In reply to Norbert Lindenberg from comment #11)
> (In reply to Terrence Cole [:terrence] from comment #8)
> 
> > > > ::: js/src/tests/jstests.list
> > > > Also, the code you've added to handle
> > > > |include| is quite tricky and appears to be totally unused at the moment.
> > > 
> > > It's used for lines 4 and 5 of jstests.list.
> > 
> > I was not able to find test402/jstests.list anywhere in your patches. Is it
> > included with the 402 tests?
> 
> No. As for all the other test directories, test402/jstests.list is
> constructed for the browser environment by "jstests.py --make-manifests",
> and the python/shell environment acts as if it existed.

Ah, excellent. Since we are constructing it from the information in the outer manifest, even more reason not to worry about nested manifests clobbering each other.

::: js/src/tests/lib/manifest.py
@@ +324,5 @@
> +            # consider skip-if, random-if, and fails-if with as-yet unresolved
> +            # conditions.
> +            # At this point, we use external manifests only for test cases
> +            # that can't have their own failure type comments, so we simply
> +            # use the terms for the most specific path.

Excellent comment!

Comment 13

[Norbert Lindenberg]

Attachment: Enhance jstests framework to support Test402 tests

Sorry, had to change load to loadRelativeToScript in test402/shell.js due to changes made under bug 853541. Assuming that this doesn't invalidate the r+terrence.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 731596 [details] [diff] [review]
Enhance jstests framework to support Test402 tests

https://hg.mozilla.org/integration/mozilla-inbound/rev/be2f7045bf15

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be2f7045bf15

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 728869 [details] [diff] [review]
Script to import ECMA-402 conformance tests into the Mozilla tree

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

::: js/src/tests/update-test402.sh
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Usage: update-test402.sh <URL of test262 hg>
> +# E.g.: update-test402.sh http://hg.ecmascript.org/tests/test262/
> +# Note that test402 is part of the test262 repository.

Could you put a little blurb here explaining what test402 is?

@@ +15,5 @@
> +# Test402 uses the test262 harness, and its test cases are part of the test262
> +# repository. Mercurial doesn't have a way to download just a part of a
> +# repository, or to just get the working copy - we have to clone the entire
> +# thing. We use a temporary test262 directory for that.
> +tmp_dir=`dirname $0`/test262_tmp

I would put this in /tmp, maybe just `mktemp -d`, to avoid cluttering up the srcdir.

@@ +46,5 @@
> +hg revert --no-backup test402/shell.js
> +
> +# Keep a record of what we imported.
> +echo "URL:         $1" > ${test402_dir}/HG-INFO
> +hg -R ${tmp_dir} log -l 1 >> ${test402_dir}/HG-INFO

Probably "hg -R ${tmp_dir} log -r.", since '.' is the revision of the working directory.

Comment 17

[Norbert Lindenberg]

Attachment: Script to import ECMA-402 conformance tests into the Mozilla tree

Updated per comment 16. Carrying r+ted.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0608d181dac6

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0608d181dac6

Comment 20

[Norbert Lindenberg]

Comment on attachment 728870 [details] [diff] [review]
Import test402 source files

My latest proposal from an email discussion that didn't quite reach conclusion on how to handle the test cases:

1) As part of SpiderMonkey development, Test402 is treated as an externally maintained resource that's imported and updated without modifications through the import script. Reviews are rubberstamps. Having the tests run on a daily basis helps avoid regressions, and if any tests fail because they're invalid, that's reported into bugs.ecmascript.org, and they're disabled until they're updated through the Test262 process.

2) As ECMA-402 contributors, Mozilla engineers can review and update Test402 test cases (and I agree with Jeff that some review is called for). Reviews have to focus on whether tests accurately test conformance with ECMA-402; SpiderMonkey coding style doesn't apply. Any bug reports or changes resulting from reviewing the patches are contributed to (bugs|hg).ecmascript.org, following the Test262 process.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 728870 [details] [diff] [review]
Import test402 source files

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

The BSD license, as opposed to CC0 or public domain, is some Ecma hurdle, right?  Sad.  :-\

My concern is that any tests that haven't been looked at by multiple people from different engines has substantial chance of encoding beyond-the-spec requirements -- either from not sufficiently isolating vendor-specific behaviors, or because the implementation under test has a bug that the tests implicitly encode.  I see these all the time in Mozilla's test suites.  It's not an issue of any person or vendor -- it's something that hits everyone.

The former's often pretty clear when it's a syntax extension, but other cases are possible as well.  How Array sorts work (in terms of when comparators are called, with which arguments) is one case.  How properties get enumerated is another.  (Yes, there's still cross-engine variation on the point.)

I also see the latter pretty often in tests -- even in tests specifically written to recognize failure due to a temporary implementation shortcoming.  We have one [].splice test for behavior on an array with non-writable length, written by someone else at my request and reviewed by me.  The test's expected to throw an "InternalError" right now, because we don't implement that.  But even in an engine that does implement non-writable length, the test still fails -- because we didn't think hard enough, and the test expects a property past the length of the array.  And that's even when we were trying!  We have lots more tests that fail when non-writable length is properly implemented, some even with adjacent comments claiming correctness.

Put simply: nobody, not us, not Microsoft, not Google, no one -- is smart enough to write correct, only-to-the-spec tests.  Even if they have an implementation to write them against, and even if they have other people from that one implementation look at them.  There's no way to completely fix the problem, but reviews from different people representing at least two different engines is the best way I can think of to reduce the problem.  And given the large degree of implementation-dependent behavior permitted by Intl, I think the issue's a good deal bigger there.

So if we could get upstream to adopt that policy -- and I would hope there wouldn't be opposition to this -- I'll be happy.  Bonus points if test submissions happen in small amounts (one or a few tests at a time), as that'll make reviews to commit them significantly lower-friction.

(In reply to Norbert Lindenberg from comment #20)
> 1) As part of SpiderMonkey development, Test402 is treated as an externally
> maintained resource that's imported and updated without modifications
> through the import script. Reviews are rubberstamps.
> 
> 2) As ECMA-402 contributors, Mozilla engineers can review and update Test402
> test cases (and I agree with Jeff that some review is called for).

I think this can basically work.  (And for what it's worth, we've deliberately never enforced any sort of coding style on JS tests, either jit-test or jstest.  Although on very rare occasions I will ask for a change when the particular structure/style used so strongly misleading as to make it hard to understand/verify the test at all.  I think I could count the times I've done that on one hand.)

Comment 22

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5100f9d172cf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5100f9d172cf