1421064 - Minor updates to test262-export.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[valerie]

Updates to the test262-export.js script and root shell.js file in order to more easily export js tests to the test262 format. Once exported, a test262 test harness can run these files in other javascript implementations. 

The changes here:
- test262-export.js: Support multiple folders as command line argument
- test262-esport.js: Include references to appropriate shell.js files in exported tests
- shell.js: check for "options" before calling

Comment 1

[valerie]

Attachment: Update export script to accept multiple src dirs

Comment 2

[valerie]

Attachment: 0002-Bug-1421064-Add-automatic-shell.js-includes-to-expor.patch

Comment 3

[valerie]

Attachment: 0003-Bug-1421064-Do-not-fail-if-options-do-not-exist-r-sf.patch

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8932498 [details] [diff] [review]
Update export script to accept multiple src dirs

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

::: js/src/tests/test262-export.py
@@ +333,5 @@
> +                # Copy non-test files as is.
> +                (_, fileExt) = os.path.splitext(fileName)
> +                if fileExt != ".js":
> +                    shutil.copyfile(filePath, os.path.join(currentOutDir, fileName))
> +                    print("C %s" % filePath)

So if I'm reading this correctly, this will change from a relative to an absolute path in the printout. I guess it doesn't matter much either way, but was this change intended? The SAVED message will now be relative while SKIPPED and C are absolute, which seems a little weird.

I guess you're doing this because different srcs might have identical relative names. Given that you're already sort of assuming that the src basenames are unique, I wonder if you would rather be printing out testName but change its definition to

  testName = os.path.join(basename, os.path.relpath(filePath, src))

or something? (I might have that totally wrong, I haven't run any of this to try.)

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8932499 [details] [diff] [review]
0002-Bug-1421064-Add-automatic-shell.js-includes-to-expor.patch

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

I'm not sure whether you intended to request review on this yet or not. To request review, set the review flag to '?' with the requestee ":sfink". I'll change it to '+' when I review it. (It looks like you marked r+ directly on the other patch, which is fine for now, since I would have given it r+ anyway.)

But for this patch, I'd like some background info. It looks like test262 uses includes pretty sparingly. So the intent of this patch is to be able to run all of the non-test262 jstests as plain test262 tests (or at least, all those that don't use things specific to the spidermonkey test shell)? Because it seems like this exports tests to a somewhat non-test262 style. Which makes total sense if the intention is not for uploading to test262. Is that understanding correct?

::: js/src/tests/test262-export.py
@@ +252,5 @@
>                  frontmatter["negative"]["type"]))
>  
> +    # Add the shell specific includes
> +    if includes:
> +        frontmatter["includes"] = includes

I don't think it matters, but this would feel safer to me if you copied the includes list instead of holding a reference to it (ie, includes[:] or list(includes), whichever.)

@@ +418,5 @@
>      parser = argparse.ArgumentParser(description="Export tests to match Test262 file compliance.")
>      parser.add_argument("--out", default="test262/export",
>                          help="Output directory. Any existing directory will be removed! (default: %(default)s)")
> +    parser.add_argument("--shellDir",
> +                         help="The base direction for jstests, used to find all shell.js files. Most reftests will not run without these includes.")

s/direction/directory/

I'd rather name this option --basedir or --testroot or --includeroot or --includebase or something.

But earlier, this script is very particular about making sure it is being run from js/src/tests. Is it expected for --shellDir to ever be something other than os.getcwd() in that case?

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8932500 [details] [diff] [review]
0003-Bug-1421064-Do-not-fail-if-options-do-not-exist-r-sf.patch

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

Is this for running under v8 or something? Anyway, r=me.

Comment 7

[valerie]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)
> Comment on attachment 8932498 [details] [diff] [review]
> Update export script to accept multiple src dirs
> 
> Review of attachment 8932498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/test262-export.py
> @@ +333,5 @@
> > +                # Copy non-test files as is.
> > +                (_, fileExt) = os.path.splitext(fileName)
> > +                if fileExt != ".js":
> > +                    shutil.copyfile(filePath, os.path.join(currentOutDir, fileName))
> > +                    print("C %s" % filePath)
> 
> So if I'm reading this correctly, this will change from a relative to an
> absolute path in the printout. I guess it doesn't matter much either way,
> but was this change intended? The SAVED message will now be relative while
> SKIPPED and C are absolute, which seems a little weird.
> 
> I guess you're doing this because different srcs might have identical
> relative names. Given that you're already sort of assuming that the src
> basenames are unique, I wonder if you would rather be printing out testName
> but change its definition to
> 
>   testName = os.path.join(basename, os.path.relpath(filePath, src))
> 
> or something? (I might have that totally wrong, I haven't run any of this to
> try.)

I have to admit I didn't pay attention to the relative/full path name distinction in the output. The "C" message, though, has always been absolute and the changes I made changed SKIPPED from relative to absolute, while SAVED remained the same. 

I'll change everything to relative for ease on the eyes! Including the basename, like you suggested -- but with slightly simpler code:

    testName = os.path.join(fullRelPath, fileName)

Comment 8

[valerie]

(In reply to Steve Fink [:sfink] [:s:] from comment #5)
> Comment on attachment 8932499 [details] [diff] [review]
> 0002-Bug-1421064-Add-automatic-shell.js-includes-to-expor.patch
> 
> Review of attachment 8932499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure whether you intended to request review on this yet or not. To
> request review, set the review flag to '?' with the requestee ":sfink". I'll
> change it to '+' when I review it. (It looks like you marked r+ directly on
> the other patch, which is fine for now, since I would have given it r+
> anyway.)
> 

Oh great -- I had no idea what those boxes were about. Got it now.

> But for this patch, I'd like some background info. It looks like test262
> uses includes pretty sparingly. So the intent of this patch is to be able to
> run all of the non-test262 jstests as plain test262 tests (or at least, all
> those that don't use things specific to the spidermonkey test shell)?
> Because it seems like this exports tests to a somewhat non-test262 style.
> Which makes total sense if the intention is not for uploading to test262. Is
> that understanding correct?


It's true, test262 uses includes very sparingly, and yes, this code definitely should NOT be used when exporting tests with the aim of uploading them to test262.

When doing this edit I was not thinking of the original purpose of the script, to be honest -- I just wanted to export the tests to test262 format so that I could use our test runner to run the tests on another javascript implementation, for the purpose of research. And for that, I needed the shell.js file.

I committed the code in case anyone wants to run spidermonkey tests on another implementation in the future -- I can make the purpose of this functionality clearer in the command line arguments!

(If you are curious, I used nodejs to get the results in the spreadsheet.)

> 
> ::: js/src/tests/test262-export.py
> @@ +252,5 @@
> >                  frontmatter["negative"]["type"]))
> >  
> > +    # Add the shell specific includes
> > +    if includes:
> > +        frontmatter["includes"] = includes
> 
> I don't think it matters, but this would feel safer to me if you copied the
> includes list instead of holding a reference to it (ie, includes[:] or
> list(includes), whichever.)
> 
> @@ +418,5 @@
> >      parser = argparse.ArgumentParser(description="Export tests to match Test262 file compliance.")
> >      parser.add_argument("--out", default="test262/export",
> >                          help="Output directory. Any existing directory will be removed! (default: %(default)s)")
> > +    parser.add_argument("--shellDir",
> > +                         help="The base direction for jstests, used to find all shell.js files. Most reftests will not run without these includes.")
> 
> s/direction/directory/
> 
> I'd rather name this option --basedir or --testroot or --includeroot or
> --includebase or something.
> 
> But earlier, this script is very particular about making sure it is being
> run from js/src/tests. Is it expected for --shellDir to ever be something
> other than os.getcwd() in that case?

Oh great! If it insists on being running from js/src/tests then I will make this flag simpler -- perhaps "--includeshellscripts" ?

Comment 9

[valerie]

Attachment: 0001-Bug-1421064-Update-export-script-to-accept-multiple-.patch

Comment 10

[valerie]

Attachment: 0002-Bug-1421064-Add-automatic-shell.js-includes-to-expor.patch

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8933342 [details] [diff] [review]
0002-Bug-1421064-Add-automatic-shell.js-includes-to-expor.patch

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

Heh. The usage message can't really get any clearer than that!

Comment 12

[Steve Fink [:sfink] [:s:]]

Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c10c3e04e7cb09632b4b11842d875f76f8071e5

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e49df5d6d0
Update export script to accept multiple src dirs r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb18504d487
Add automatic shell.js includes to export script r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a559782a53a
Do not fail if options do not exist r=sfink

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f7e49df5d6d0
https://hg.mozilla.org/mozilla-central/rev/7bb18504d487
https://hg.mozilla.org/mozilla-central/rev/0a559782a53a