1374290 - test262-update.py imports Pull Requests

Status: ASSIGNED

Description

[Leo Balter]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36

Steps to reproduce:

This bug has a patch to import Pull Request files from Test262

Example:

on `js/src/tests`:

```
./test262-update.py  --pull 945
```

Comment 1

[Leo Balter]

Created attachment 8879147 [details] [diff] [review]
0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch

This is a minor patch to the Test262 update script to import contents from Pull Requests. It doesn't require any authentication.

It creates a test262/pr<Number>/ folder. If the respective PR status is closed or merged it removes the local PR folder.

Files are listed in the process to help addition into the skip list.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8879147 [details] [diff] [review]
0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch

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

::: js/src/tests/test262-update.py
@@ +384,5 @@
>          else:
>              subprocess.check_call(["git", "clone", "--single-branch", "--branch=%s" % branch, url, inDir])
>              subprocess.check_call(["git", "-C", inDir, "reset", "--hard", revision])
>  
> +        if prNumber:

The PR mode of operation deserves an overview comment, at least about how it blows away all tests in the git checkout of the tree and reflects only the PR's tests, then syncs those.

@@ +386,5 @@
>              subprocess.check_call(["git", "-C", inDir, "reset", "--hard", revision])
>  
> +        if prNumber:
> +            prsTestsDir = "test/pr{}".format(prNumber)
> +            prsTestsOutDir = os.path.join(outDir, prsTestsDir)

Why the test/ prefix?

Am I reading this right that for PR #N, it'd put it in test262/test/prN? Shouldn't it go in test262/local/prN, or it'll get blown away by a subsequent non-PR update?

@@ +394,5 @@
> +
> +            # Reuses current Test262 clone's harness and tools folders only
> +            shutil.rmtree(os.path.join(inDir, "test"))
> +
> +            prRequest = requests.get("https://api.github.com/repos/tc39/test262/pulls/%s" % prNumber)

You use both |"{}".format| and % to format strings. I don't know which one is better style, but I guess please use % for consistency.

@@ +395,5 @@
> +            # Reuses current Test262 clone's harness and tools folders only
> +            shutil.rmtree(os.path.join(inDir, "test"))
> +
> +            prRequest = requests.get("https://api.github.com/repos/tc39/test262/pulls/%s" % prNumber)
> +            prRequest.raise_for_status()

Is there a more ergonomic way besides calling |raise_for_status| after every |get|?

@@ +408,5 @@
> +                files.raise_for_status()
> +
> +                data = files.json()
> +
> +                for item in data:

Clearer as |for item in files.json():|

@@ +413,5 @@
> +                    # Do not add deleted files
> +                    if item["status"] == "deleted":
> +                        continue
> +
> +                    if not item["filename"].startswith("test/"):

Does GH's API always use Unix-style directory separators? (i.e. '/' instead of '\')

@@ +418,5 @@
> +                        continue
> +
> +                    contents = requests.get(item["raw_url"])
> +                    contents.raise_for_status()
> +                    fileText = contents.text

What does the GH API do for binary files? Are there ever binary files that start with test/ in these PRs?

@@ +430,5 @@
> +
> +                    if not os.path.isdir(file_path_dirs):
> +                        os.makedirs(file_path_dirs)
> +
> +                    filename = os.path.join(inDir, prsTestsDir, *filename)

The filename splitting logic is much simpler as:

filename = os.path.join(inDir, prsTestDir, *(item["filename"].split("/")[1:]))
filename_dir = os.path.dirname(filename)

if not os.path.isdir(filename_dir):
  os.makedirs(filename_dir)

@@ +433,5 @@
> +
> +                    filename = os.path.join(inDir, prsTestsDir, *filename)
> +
> +                    print("Saving {} ... ".format(filename))
> +                    with open(filename, "w") as output_file:

"wb" in case of Windows.

Comment 3

[Leo Balter]

> The PR mode of operation deserves an overview comment, at least about how it blows away all tests in the git checkout of the tree and reflects only the PR's tests, then syncs those.

Yes, and I got this fixed.

> Why the test/ prefix?
> Am I reading this right that for PR #N, it'd put it in test262/test/prN? Shouldn't it go in test262/local/prN, or it'll get blown away by a subsequent non-PR update?

I couldn't use the `test262/local` folder properly due to the current structure used for file processing, but I set a `test262/prs/<number>` folder format which allows the script to process files while maintaining the folder, and avoiding it to be removed in a general test262-update run.

> You use both |"{}".format| and % to format strings. I don't know which one is better style, but I guess please use % for consistency.

Sure, thanks!

> Is there a more ergonomic way besides calling |raise_for_status| after every |get|?

Maybe if we restrict this to Python 3, but I'm not following this path. raise_for_status helps when the API returns a HTTP error. I can remove and let the script raises colateral errors, but I don't think it would be clear.

> Clearer as |for item in files.json():|

This is fixed now.

> Does GH's API always use Unix-style directory separators? (i.e. '/' instead of '\')

My best guess is yes, but I sent a message to their support confirming this information.

> What does the GH API do for binary files? Are there ever binary files that start with test/ in these PRs?

I'm capturing the raw contents, we don't expect any binary files in the test folder. The GH API support custom media types, but I don't think this is necessary for a workaround. Unless you tell me otherwise.

ref https://developer.github.com/v3/repos/contents/#get-contents

> The filename splitting logic is much simpler as: ...
> "wb" in case of Windows.

Thanks, fixed!

I'm uploading the updated patch in a couple minutes

Comment 4

[Leo Balter]

Created attachment 8881494 [details] [diff] [review]
0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch

the updated patch after the review feedback

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8881494 [details] [diff] [review]
0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch

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

Looks pretty good.

My remaining question is what happens with non-mergeable PRs. If the PR has conflicts, what're the contents of the files? I couldn't find the answer in GH's API docs.

::: js/src/tests/test262-update.py
@@ +432,5 @@
> +                    contents.raise_for_status()
> +
> +                    fileText = contents.text
> +
> +                    # Prefix the PRs tests dir with test/ so files are processed as Test262 file

Nit: files

@@ +446,5 @@
> +                        output_file.write(fileText)
> +        # Without a specific PR, follows through a regular copy.
> +        else:
> +            # Stash test262/local. Currently the test262 repo does not have any
> +            # top-level subdirectory named "local".

Update comment to also include "prs"

@@ +473,5 @@
>  
>          # Move test262/local back.
>          if restoreLocalTestsDir:
>              shutil.move(os.path.join(inDir, "local"), outDir)
> +        

Nit: trailing whitespace

Comment 6

[Leo Balter]

Cool, I'm sending new patch soon to fix the remaining parts.

> My remaining question is what happens with non-mergeable PRs. If the PR has conflicts, what're the contents of the files? I couldn't find the answer in GH's API docs.


If a PR has conflicts, it's only in a case of a merge with the target branch. The import script will get the current file contents from the branch used. There won't be any merge with the original script.

The import tool lists all the files, including those removed, so the maintainer can add the originals (from the master branch) to the ignore list as well.

Comment 7

[Leo Balter]

Created attachment 8881941 [details] [diff] [review]
0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch

This file contains the updates from the latest review:


```
diff --git a/js/src/tests/test262-update.py b/js/src/tests/test262-update.py
index 9059d0d86303..6475ab548a2b 100755
--- a/js/src/tests/test262-update.py
+++ b/js/src/tests/test262-update.py
@@ -433,7 +433,7 @@ def update_test262(args):

                     fileText = contents.text

-                    # Prefix the PRs tests dir with test/ so files are processed as Test262 file
+                    # Prefix the PRs tests dir with test/ so files are processed as Test262 files
                     prsTestsDir = "test/prs/%s" % prNumber
                     filePathDirs = os.path.join(inDir, prsTestsDir, *filename.split("/")[1:-1])

@@ -446,8 +446,9 @@ def update_test262(args):
                         output_file.write(fileText)
         # Without a specific PR, follows through a regular copy.
         else:
-            # Stash test262/local. Currently the test262 repo does not have any
-            # top-level subdirectory named "local".
+            # Stash test262/local and test262/prs. Currently the Test262 repo does not have any
+            # top-level subdirectories named "local" or "prs".
+            # This prevents these folders from being removed during the update process.
             if os.path.isdir(localTestsOutDir):
                 shutil.move(localTestsOutDir, inDir)
                 restoreLocalTestsDir = True
@@ -474,7 +475,7 @@ def update_test262(args):
         # Move test262/local back.
         if restoreLocalTestsDir:
             shutil.move(os.path.join(inDir, "local"), outDir)
-
+
         # Restore test262/prs if necessary after a general Test262 update.
         if restorePrsTestsDir:
             shutil.move(os.path.join(inDir, "prs"), outDir)
```

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8881941 [details] [diff] [review]
0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch

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

Looks good to me, thanks for being thorough!

Comment 9

[Leo Balter]

Thank you!

Comment 10

[Leo Balter]

Created attachment 8884088 [details] [diff] [review]
0002-Bug-1374290-Fix-encoding-on-imported-PR-files.-r-shu.patch

this fixes an encoding issue found writing files with non-ascii chars.

Comment 11

[Leo Balter]

Created attachment 8884089 [details] [diff] [review]
0003-Bug-1374290-Import-diff-contents-from-a-local-Test26.patch

this patch adds an import script for local branches. It's working but it might need some discussion before landing. Reviews are pretty welcome!

Comment 12

[Leo Balter]

Shu, I got a rebased and noticed the update script had no other change. I'm assuming the first patch does not need an update here, as I got it untouched on my base since you already reviewed it.

I can squash these if necessary, for now, I just want to keep the review easier for you.

The current script for local imports works this way, with a simulated output:

```
 ./test262-update.py --local ~/dev/test262/
From the branch shvaikalesh-undefined-proxy-traps in /Users/leo/dev/test262/

Files being copied to the local folder:
test/built-ins/Proxy/apply/trap-is-missing.js
test/built-ins/Proxy/apply/trap-is-null-or-undefined.js
test/built-ins/Proxy/construct/call-parameters-new-target.js
test/built-ins/Proxy/construct/trap-is-missing-proto-from-ctor-realm.js
test/built-ins/Proxy/construct/trap-is-missing.js
test/built-ins/Proxy/construct/trap-is-null-or-undefined.js

Deleted files (use this list to update the skip list):
test/built-ins/Proxy/apply/trap-is-undefined.js
test/built-ins/Proxy/construct/trap-is-undefined-no-property.js
test/built-ins/Proxy/construct/trap-is-undefined.js

Renamed files (already added with the new names):
 rename test/built-ins/Proxy/apply/{trap-is-undefined-no-property.js => trap-is-missing.js} (50%)
 rename test/built-ins/Proxy/construct/{trap-is-undefined-proto-from-ctor-realm.js => trap-is-missing-proto-from-ctor-realm.js} (100%)
```

So it creates a `test262/local/<branch_name>` folder.

---

One thing I'm considering is flattening the file paths so we list all of them on the local folder. What do you think?

e.g.: `built-ins/Proxy/apply/trap-is-missing.js` would become: `built-ins__Proxy__apply__trap-is-missing.js`

This improves navigation, and I'm unsure of any cons so far.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8884088 [details] [diff] [review]
0002-Bug-1374290-Fix-encoding-on-imported-PR-files.-r-shu.patch

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

Thanks for refactoring the PR stuff.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8884089 [details] [diff] [review]
0003-Bug-1374290-Import-diff-contents-from-a-local-Test26.patch

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

My main concern about this workflow is I don't yet understand how it meshes with the new workflow you proposed in our previous call. IIRC, the workflow you proposed was for us to write reftests in the local/ folder directly, and then have an export functionality instead of the import functionality seen here. Would this importing workflow be superseded by the exporting workflow?

::: js/src/tests/test262-update.py
@@ +330,5 @@
>          if relPath == ".":
>              continue
>  
>          # Skip creating a "prs" directory if it already exists
> +        if not os.path.exists(os.path.join(test262OutDir, relPath)) or relPath != "prs" and relPath != "local":

Why move the string checks to the right?

@@ +360,5 @@
>  
>          # Add shell.js and browers.js files for the current directory.
>          writeShellAndBrowserFiles(test262OutDir, harnessDir, includesMap, localIncludesMap, relPath)
>  
> +def fetch_local_changes(inDir, outDir, srcDir, strictTests):

I like block comments for these things. Am I correct in the following understanding of what's going on here:

1. Get the list of file changes made by the current branch in some local test262 repo.
2. Copy only those files from srcDir (the local test262 repo) to inDir.
3. Treat the inDir as a test262 checkout.
4. Copy inDir stuff to outDir/local.

@@ +415,5 @@
> +    # Reset any older directory in the output
> +    outDir = os.path.join(outDir, "local", branchName)
> +    if os.path.isdir(outDir):
> +        shutil.rmtree(outDir)
> +    os.makedirs(outDir)

I'm worried about unconditionally blowing away the local/ directory everytime this local import is done. That seems very prone to losing work.

Comment 15

[Leo Balter]

> My main concern about this workflow is I don't yet understand how it meshes with the new workflow you proposed in our previous call. IIRC, the workflow you proposed was for us to write reftests in the local/ folder directly, and then have an export functionality instead of the import functionality seen here. Would this importing workflow be superseded by the exporting workflow?

I'm trying to follow a pragmatic approach here.

This script was already done before the call we've had and I was trying to "force" someone to work directly on Test262 and then importing tests to this suite. It is a good approach for Mozilla developers who are already used to Test262 or just changing an existing test file there. It doesn't require to open a PR on GitHub to import anything.

From my own perspective, as I have much more experience with Test262, it's even easier for me to run this script if I'm working on a fix for a bug I found here. I could send the patch here and at the same time have this going to Test262.

---

The new export script we talked on the call, not here yet on the patches, allow the reverse workflow. Let's say I'm used to write tests here, not on Test262. It allows me to work here, as I'm already used, and export this to a Test262 format. It also allows me to export any of the current tests we already have in the ecma* folders.

As I can't say how much is each Mozilla (employee or contributor) developer used to the tests suite or Test262, I believe having both scripts available are good to get used with the workflow, in one way or another.

---

Going back to your original question:

> Would this importing workflow be superseded by the exporting workflow?

I believe they will be good together. The scripts are not imposing a single workflow. It can be done one way or another and it's up to the developers and/or teams to decide what is best for their learning experience. The integration wins.

I can also say this __import__ script is about to be done as soon as I apply the review feedback - soon - and the complexity remains on the export script as we never had one. This is the current work in progress.

I'll answer the review feedback in another comment. Thanks!

Comment 16

[Leo Balter]

> Why move the string checks to the right?

I just put them back in a different order while I was refactoring. I'll fix this.

> I like block comments for these things.

Sure, I'll do this.

> 1. (...)
> 2. (...)
> 3. (...)
> 4. Copy inDir stuff to outDir/local.

Those are all right, just adding the inDir stuff I'll go to `outDir/local/<branchName>`

> I'm worried about unconditionally blowing away the local/ directory everytime this local import is done. That seems very prone to losing work.

I considered this but I made an initial decision to remove the pre-existing import folder due to some specific reasons:

1. The changes should be captured by the version control, hg or git.
2. The same is done by the current Test262 import. The whole test262 folder is deleted in the process.
3. Only the `outDir/local/<branchName>` folder is going to be removed. This helps capturing all the changes from a pre-existing folder and even not creating a mess in case of renamed files.

If you still prefer not removing the folder, I can change this part, that's not a blocker.

---

Thanks for the review, Shu!

Comment 17

[Leo Balter]

Created attachment 8892100 [details] [diff] [review]
0004-Bug-1374290-Test262-export-script.-r-shu.patch

This patch introduces the export script

Comment 18

[Leo Balter]

Created attachment 8892101 [details] [diff] [review]
0005-Dont-merge.-Samples-for-use-with-the-export-script.patch

You can use the contents here to try the export script

Comment 19

[Leo Balter]

Created attachment 8892116 [details] [diff] [review]
0006-fixup-Bug-1374290-Import-diff-contents-from-a-local-.patch

Review updates for the import script

Comment 20

[Leo Balter]

Shu, this should be ready for another round of review.

The only new files are 0004 to 0006. I didn't change the previously reviewed files.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e11a443f1b
Import Pull Requests from Test262. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc035bd5371
Fix encoding on imported PR files. r=shu

Comment 22

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/b5e11a443f1b
https://hg.mozilla.org/mozilla-central/rev/7fc035bd5371

Comment 23

[Leo Balter]

Created attachment 8894576 [details] [diff] [review]
0007-fixup-Update-export-script-to-add-copyright-lines.patch

Update export script to add the copyright header lines

Comment 24

[Leo Balter]

Created attachment 8894578 [details] [diff] [review]
0008-fixup-Dont-merge.-Samples-for-use-with-the-export-sc.patch

fixup for the samples to be used with the patch 0007

Comment 25

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

Comment on attachment 8892100 [details] [diff] [review]
0004-Bug-1374290-Test262-export-script.-r-shu.patch

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

I'd like to see this again with the changes made, when I'll think more about what it's actually doing.

::: js/src/tests/test262-export.py
@@ +25,5 @@
> +    "js-test-driver-begin.js", "js-test-driver-end.js"])
> +
> +FRONTMATTER_WRAPPER_PATTERN = re.compile(
> +    r'/\*\---\n([\s]*)((?:\s|\S)*)[\n\s*]---\*/',
> +    flags=re.DOTALL|re.MULTILINE)

Why pass DOTALL if you're not going to use it? (?:\s|\S)* is equivalent to .* with DOTALL. Either use .* or remove DOTALL and simplify to [\s\S].

Same thing for MULTILINE. You don't have ^ anywhere in that expression, so it's not doing anything. Perhaps you meant both as documentation of the intent of the regex?

Why is this escaping one - but not the other 5?

I'm confused about what this expression is intended to match. The [\n\s*] is a little weird. Do you actually have asterisks before the closing '---*/'? It looks more likely that [\n\s]* was intended.

Anyway, the whole thing seem like it could be re.compile(r'/\*---\n(\s*)(.*?)\s*---\*/', flags=re.DOTALL) if I'm guessing correctly what it is intended to match and capture.

@@ +37,5 @@
> +    source = updateMeta(source)
> +
> +    return source
> +
> +def parseReportCompare(source):

The name confused me, especially given its usage in the previous function. This doesn't just parse them out and return some sort of metadata. Maybe convertReportCompare? handleReportCompare? replaceReportCompare?

@@ +62,5 @@
> +        expected = part.group(2)
> +
> +        if actual == expected:
> +            newSource = newSource.replace(part.group(), "", 1)
> +            continue;

The "continue;" is unnecessary here (and has a trailing semicolon, which just means you're switching between Python and JS a lot!).

@@ +64,5 @@
> +        if actual == expected:
> +            newSource = newSource.replace(part.group(), "", 1)
> +            continue;
> +
> +    for part in re.finditer(r'(\.|\W)?(reportCompare)\W?', newSource, re.MULTILINE):

I would prefer the (\.|\W)? as [.\W]? (and adjust the next line to part.group(1)).

@@ +67,5 @@
> +
> +    for part in re.finditer(r'(\.|\W)?(reportCompare)\W?', newSource, re.MULTILINE):
> +        newSource = newSource.replace(part.group(2), "assert.sameValue")
> +
> +    return newSource

I suspect the whole parseReportCompare function would be simpler (and faster, not that it matters) as a single re.sub call with a function passed for the replacement string, but this is fine.

@@ +74,5 @@
> +    """
> +    Collects and stores the entries from the reftest header.
> +    """
> +
> +    # TODO: fails, slow, skip, random, random-if

Please file a bug for these and reference it in this comment.

@@ +82,5 @@
> +    comments = None
> +    module = False
> +
> +    # should capture conditions to skip
> +    matchesSkip = re.search(r'skip-if\((.*)\)', reftest)

.*? please, I could imagine later parentheses

@@ +88,5 @@
> +        matches = matchesSkip.group(1).split("||")
> +        for match in matches:
> +            # captures a features list
> +            dependsOnProp = re.search(
> +                r'!this.hasOwnProperty\([\'\"](.*)[\'\"]\)', match)

.*? again, though splitting on "||" seems to save you here.

@@ +91,5 @@
> +            dependsOnProp = re.search(
> +                r'!this.hasOwnProperty\([\'\"](.*)[\'\"]\)', match)
> +            if dependsOnProp:
> +                features.append(dependsOnProp.group(1))
> +            # TODO: how do we parse the other skip conditions?

Can you log the exceptions here? Either to stdout or stderr, or if that's too messy, to a dedicated log file for format exceptions.

@@ +99,5 @@
> +    if matchesError:
> +        # issue: we can't say it's a runtime or an early error.
> +        # If it's not a SyntaxError or a ReferenceError,
> +        # assume it's a runtime error(?)
> +        error = matchesError.group(1)

I see this in the comment. I don't see it in the code. Can you describe the situation in the comment, understandable by someone totally unfamiliar with test262 and how it does error-checking stuff?

The later code seems to distinguish between these with a 'negative' flag (I haven't really read that far, just searched and failed to find any code matches for "SyntaxError" or "ReferenceError".)

@@ +102,5 @@
> +        # assume it's a runtime error(?)
> +        error = matchesError.group(1)
> +
> +    # just tells if it's a module
> +    matchesModule = re.search(r'\smodule(\s|$)', reftest)

r'\bmodule\b' please.

@@ +152,5 @@
> +    if not match:
> +        return {}
> +
> +    unindented = re.sub('^' + match.group(1), '',
> +        match.group(2), flags=re.MULTILINE)

Name the groups from FRONTMATTER_WRAPPER_PATTERN, please. It's too hard to guess what match.group(1) and (2) are.

  indent, frontmatter_lines = match.groups()

maybe?

@@ +154,5 @@
> +
> +    unindented = re.sub('^' + match.group(1), '',
> +        match.group(2), flags=re.MULTILINE)
> +
> +    return yaml.safe_load(unindented)

Why is this unindenting necessary? I wouldn't have thought YAML would care, if everything is indented equally.

@@ +166,5 @@
> +    # Extract the reftest data from the source
> +    source, reftest = parseHeader(source)
> +
> +    # Collect the frontmatter data from the source
> +    frontmatter = fetchMeta(source)

"collect", "fetch"... I'd probably go for "extract", but it's fine the way you have it.

@@ +185,5 @@
> +
> +    # Populate required tags
> +    for tag in ("description", "esid"):
> +        if tag not in meta:
> +            meta[tag] = "pending"

for tag in (...):
    meta.setdefault(tag, "pending")

@@ +201,5 @@
> +
> +    if "negative" in meta:
> +        # If the negative tag exists, phase needs to be present and set
> +        if "phase" not in meta["negative"] or \
> +            meta["negative"]["phase"] not in ("early", "runtime"):

if meta["negative"].get("phase") not in ("early", "runtime")

(I'm not just trying to make it shorter or more clever; I find the above easier to read.)

@@ +223,5 @@
> +    if "features" in reftest:
> +        if "features" in frontmatter:
> +            frontmatter["features"] += reftest["features"]
> +        else:
> +            frontmatter["features"] = reftest["features"]

frontmatter.setdefault("features", []).extend(reftest.get("features", []))

or something similar. (Having a separate test for eg ("features" in reftest) is ok, but the repetition is too much.)

@@ +230,5 @@
> +    if reftest.get("module"):
> +        if "flags" in frontmatter:
> +            frontmatter["flags"].append("module")
> +        else:
> +            frontmatter["flags"] = ["module"]

Sorry, I know setdefault is an annoying name, but

    frontmatter.setdefault("flags", []).append("module")

just reads better to me than needing to trace through the logic.

@@ +240,5 @@
> +        # Open some space in an existing info text
> +        if "info" in frontmatter:
> +            frontmatter["info"] += "\n\n  "
> +
> +        frontmatter["info"] = info

This appears to overwrite the previous value.

@@ +273,5 @@
> +            lines.append("%s: |" % key)
> +            lines.append("  " + yaml.dump(value, encoding="utf8",
> +                ).strip().replace('\n...', ''))
> +        else:
> +            lines.append(yaml.dump({key: frontmatter[key]}, encoding="utf8",

{key: value}

@@ +278,5 @@
> +                default_flow_style=False).strip())
> +
> +
> +    #lines.append(yaml.dump(frontmatter, encoding='utf8',
> +    #    default_flow_style=False).strip())

Remove dead comment

@@ +293,5 @@
> +    src = args.src
> +    outDir = args.out
> +
> +    if not os.path.isabs(src):
> +        src = os.path.join(os.getcwd(), src)

unconditional src = os.path.abspath(src)

@@ +296,5 @@
> +    if not os.path.isabs(src):
> +        src = os.path.join(os.getcwd(), src)
> +
> +    if not os.path.isabs(outDir):
> +        outDir = os.path.join(os.getcwd(), outDir)

same

@@ +308,5 @@
> +        relPath = os.path.relpath(dirPath, src)
> +
> +        # This also creates the own outDir folder
> +        if not os.path.exists(os.path.join(outDir, relPath)):
> +            os.makedirs(os.path.join(outDir, relPath))

name this with a variable, since I'm not sure what "the own outDir folder" means. relOutDir = os.path.join(outdir, relPath), maybe?

@@ +326,5 @@
> +                print("C %s" % testName)
> +                continue
> +
> +            # Read the original test source and preprocess it for Test262
> +            with io.open(filePath, "rb") as testFile:

Huh. I've never seen io.open used. It appears that this is an alias for plain 'open'. I think the latter is more customary.

Comment 26

[Leo Balter]

(In reply to Steve Fink [:sfink] [:s:] from comment #25)
> Comment on attachment 8892100 [details] [diff] [review]
> 0004-Bug-1374290-Test262-export-script.-r-shu.patch
> 
> Review of attachment 8892100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see this again with the changes made, when I'll think more about
> what it's actually doing.
> 
> ::: js/src/tests/test262-export.py
> @@ +25,5 @@
> > +    "js-test-driver-begin.js", "js-test-driver-end.js"])
> > +
> > +FRONTMATTER_WRAPPER_PATTERN = re.compile(
> > +    r'/\*\---\n([\s]*)((?:\s|\S)*)[\n\s*]---\*/',
> > +    flags=re.DOTALL|re.MULTILINE)
> 
> Why pass DOTALL if you're not going to use it? (?:\s|\S)* is equivalent to
> .* with DOTALL. Either use .* or remove DOTALL and simplify to [\s\S].
> 
> Same thing for MULTILINE. You don't have ^ anywhere in that expression, so
> it's not doing anything. Perhaps you meant both as documentation of the
> intent of the regex?
> 
> Why is this escaping one - but not the other 5?
> 
> I'm confused about what this expression is intended to match. The [\n\s*] is
> a little weird. Do you actually have asterisks before the closing '---*/'?
> It looks more likely that [\n\s]* was intended.
> 
> Anyway, the whole thing seem like it could be
> re.compile(r'/\*---\n(\s*)(.*?)\s*---\*/', flags=re.DOTALL) if I'm guessing
> correctly what it is intended to match and capture.


I am not an expert with RegExp, and for this case I got a basic copy from Test262 to capture the frontmatter:

https://github.com/tc39/test262/blob/da4f4385fdf88ff2c8acf036efaaa62f8cd6bd58/tools/generation/lib/util/parse_yaml.py#L6-L7

The pattern is used this way to not only capture simple cases of frontmatter, but also cases with trailing spaces to be preserved. I've used the current tests from Test262 to verify and now I want to write a few tests in here as well, so the issue can be better seen.

> 
> @@ +37,5 @@
> > +    source = updateMeta(source)
> > +
> > +    return source
> > +
> > +def parseReportCompare(source):
> 
> The name confused me, especially given its usage in the previous function.
> This doesn't just parse them out and return some sort of metadata. Maybe
> convertReportCompare? handleReportCompare? replaceReportCompare?

I changed it to convertReportCompare

> 
> @@ +62,5 @@
> > +        expected = part.group(2)
> > +
> > +        if actual == expected:
> > +            newSource = newSource.replace(part.group(), "", 1)
> > +            continue;
> 
> The "continue;" is unnecessary here (and has a trailing semicolon, which
> just means you're switching between Python and JS a lot!).

Done, and yes, I'm way more used to write JS in the regular basis.

> 
> @@ +64,5 @@
> > +        if actual == expected:
> > +            newSource = newSource.replace(part.group(), "", 1)
> > +            continue;
> > +
> > +    for part in re.finditer(r'(\.|\W)?(reportCompare)\W?', newSource, re.MULTILINE):
> 
> I would prefer the (\.|\W)? as [.\W]? (and adjust the next line to
> part.group(1)).

done.

> 
> @@ +67,5 @@
> > +
> > +    for part in re.finditer(r'(\.|\W)?(reportCompare)\W?', newSource, re.MULTILINE):
> > +        newSource = newSource.replace(part.group(2), "assert.sameValue")
> > +
> > +    return newSource
> 
> I suspect the whole parseReportCompare function would be simpler (and
> faster, not that it matters) as a single re.sub call with a function passed
> for the replacement string, but this is fine.

I changed it and it's much smaller now, but it still needs more improvements. I didn't want to get myself stuck on that before sending the patch with the other changes. This means I'll revisit it soon.

> 
> @@ +74,5 @@
> > +    """
> > +    Collects and stores the entries from the reftest header.
> > +    """
> > +
> > +    # TODO: fails, slow, skip, random, random-if
> 
> Please file a bug for these and reference it in this comment.

Sure.

> 
> @@ +82,5 @@
> > +    comments = None
> > +    module = False
> > +
> > +    # should capture conditions to skip
> > +    matchesSkip = re.search(r'skip-if\((.*)\)', reftest)
> 
> .*? please, I could imagine later parentheses

In this case it broke some of the expectations. I'm not sure how to properly change this.

> 
> @@ +88,5 @@
> > +        matches = matchesSkip.group(1).split("||")
> > +        for match in matches:
> > +            # captures a features list
> > +            dependsOnProp = re.search(
> > +                r'!this.hasOwnProperty\([\'\"](.*)[\'\"]\)', match)
> 
> .*? again, though splitting on "||" seems to save you here.

done.

> 
> @@ +91,5 @@
> > +            dependsOnProp = re.search(
> > +                r'!this.hasOwnProperty\([\'\"](.*)[\'\"]\)', match)
> > +            if dependsOnProp:
> > +                features.append(dependsOnProp.group(1))
> > +            # TODO: how do we parse the other skip conditions?
> 
> Can you log the exceptions here? Either to stdout or stderr, or if that's
> too messy, to a dedicated log file for format exceptions.


The code will print some information now. e.g.: "# Can't parse the following skip-if rule: outro()"

> 
> @@ +99,5 @@
> > +    if matchesError:
> > +        # issue: we can't say it's a runtime or an early error.
> > +        # If it's not a SyntaxError or a ReferenceError,
> > +        # assume it's a runtime error(?)
> > +        error = matchesError.group(1)
> 
> I see this in the comment. I don't see it in the code. Can you describe the
> situation in the comment, understandable by someone totally unfamiliar with
> test262 and how it does error-checking stuff?
> 
> The later code seems to distinguish between these with a 'negative' flag (I
> haven't really read that far, just searched and failed to find any code
> matches for "SyntaxError" or "ReferenceError".)

I improved the comment and placed it where I really define the phase. I believe it's fine to pre-assume the errors are for the early phase, but maybe a warning can help if it asks human verification for the generated code.

> 
> @@ +102,5 @@
> > +        # assume it's a runtime error(?)
> > +        error = matchesError.group(1)
> > +
> > +    # just tells if it's a module
> > +    matchesModule = re.search(r'\smodule(\s|$)', reftest)
> 
> r'\bmodule\b' please.

done. 
 
> @@ +152,5 @@
> > +    if not match:
> > +        return {}
> > +
> > +    unindented = re.sub('^' + match.group(1), '',
> > +        match.group(2), flags=re.MULTILINE)
> 
> Name the groups from FRONTMATTER_WRAPPER_PATTERN, please. It's too hard to
> guess what match.group(1) and (2) are.
> 
>   indent, frontmatter_lines = match.groups()
> 
> maybe?

done.

> 
> @@ +154,5 @@
> > +
> > +    unindented = re.sub('^' + match.group(1), '',
> > +        match.group(2), flags=re.MULTILINE)
> > +
> > +    return yaml.safe_load(unindented)
> 
> Why is this unindenting necessary? I wouldn't have thought YAML would care,
> if everything is indented equally.

this is for proper formatting, we have the same on Test262 tools, it might help handling some bad formatting and it doesn't hurt if the formatting is good, IMHO.


> 
> @@ +166,5 @@
> > +    # Extract the reftest data from the source
> > +    source, reftest = parseHeader(source)
> > +
> > +    # Collect the frontmatter data from the source
> > +    frontmatter = fetchMeta(source)
> 
> "collect", "fetch"... I'd probably go for "extract", but it's fine the way
> you have it.

I renamed it to extractMeta.

> 
> @@ +185,5 @@
> > +
> > +    # Populate required tags
> > +    for tag in ("description", "esid"):
> > +        if tag not in meta:
> > +            meta[tag] = "pending"
> 
> for tag in (...):
>     meta.setdefault(tag, "pending")

done.

> 
> @@ +201,5 @@
> > +
> > +    if "negative" in meta:
> > +        # If the negative tag exists, phase needs to be present and set
> > +        if "phase" not in meta["negative"] or \
> > +            meta["negative"]["phase"] not in ("early", "runtime"):
> 
> if meta["negative"].get("phase") not in ("early", "runtime")
> 
> (I'm not just trying to make it shorter or more clever; I find the above
> easier to read.)


done. 


> 
> @@ +223,5 @@
> > +    if "features" in reftest:
> > +        if "features" in frontmatter:
> > +            frontmatter["features"] += reftest["features"]
> > +        else:
> > +            frontmatter["features"] = reftest["features"]
> 
> frontmatter.setdefault("features", []).extend(reftest.get("features", []))
> 
> or something similar. (Having a separate test for eg ("features" in reftest)
> is ok, but the repetition is too much.)


done.

> 
> @@ +230,5 @@
> > +    if reftest.get("module"):
> > +        if "flags" in frontmatter:
> > +            frontmatter["flags"].append("module")
> > +        else:
> > +            frontmatter["flags"] = ["module"]
> 
> Sorry, I know setdefault is an annoying name, but
> 
>     frontmatter.setdefault("flags", []).append("module")
> 
> just reads better to me than needing to trace through the logic.

done.

> 
> @@ +240,5 @@
> > +        # Open some space in an existing info text
> > +        if "info" in frontmatter:
> > +            frontmatter["info"] += "\n\n  "
> > +
> > +        frontmatter["info"] = info
> 
> This appears to overwrite the previous value.

It's fixed now.

> 
> @@ +273,5 @@
> > +            lines.append("%s: |" % key)
> > +            lines.append("  " + yaml.dump(value, encoding="utf8",
> > +                ).strip().replace('\n...', ''))
> > +        else:
> > +            lines.append(yaml.dump({key: frontmatter[key]}, encoding="utf8",
> 
> {key: value}


fixed.

> 
> @@ +278,5 @@
> > +                default_flow_style=False).strip())
> > +
> > +
> > +    #lines.append(yaml.dump(frontmatter, encoding='utf8',
> > +    #    default_flow_style=False).strip())
> 
> Remove dead comment

fixed.

> 
> @@ +293,5 @@
> > +    src = args.src
> > +    outDir = args.out
> > +
> > +    if not os.path.isabs(src):
> > +        src = os.path.join(os.getcwd(), src)
> 
> unconditional src = os.path.abspath(src)


done.

> 
> @@ +296,5 @@
> > +    if not os.path.isabs(src):
> > +        src = os.path.join(os.getcwd(), src)
> > +
> > +    if not os.path.isabs(outDir):
> > +        outDir = os.path.join(os.getcwd(), outDir)
> 
> same

done.

> 
> @@ +308,5 @@
> > +        relPath = os.path.relpath(dirPath, src)
> > +
> > +        # This also creates the own outDir folder
> > +        if not os.path.exists(os.path.join(outDir, relPath)):
> > +            os.makedirs(os.path.join(outDir, relPath))
> 
> name this with a variable, since I'm not sure what "the own outDir folder"
> means. relOutDir = os.path.join(outdir, relPath), maybe?

done.

> 
> @@ +326,5 @@
> > +                print("C %s" % testName)
> > +                continue
> > +
> > +            # Read the original test source and preprocess it for Test262
> > +            with io.open(filePath, "rb") as testFile:
> 
> Huh. I've never seen io.open used. It appears that this is an alias for
> plain 'open'. I think the latter is more customary.

I was following consistency with the test262-update.py file, but I changed this to open only.

Comment 27

[Leo Balter]

Created attachment 8895059 [details] [diff] [review]
0009-fixup-Applying-changes-from-review-feedback.patch

Thanks for the comprehensive review! I applied all the changes in this patch and I can rebase and squash it anytime you request.

Comment 28

[Leo Balter]

Created attachment 8895060 [details] [diff] [review]
0010-fixup-mock-files.patch

extracted files

Comment 29

[Leo Balter]

I also created the new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1388510

Comment 30

[Aki Sasaki [:aki]]

https://hg.mozilla.org/projects/date/rev/b5e11a443f1b8cd884bcc9694722ee13af83572d
Bug 1374290 - Import Pull Requests from Test262. r=shu

https://hg.mozilla.org/projects/date/rev/7fc035bd5371bd8d9f1910dde7a178321d4a51ed
Bug 1374290 - Fix encoding on imported PR files. r=shu

Comment 31

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

Comment on attachment 8892116 [details] [diff] [review]
0006-fixup-Bug-1374290-Import-diff-contents-from-a-local-.patch

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

I guess I was probably just supposed to review the changes here, but I'd really rather you change the f.split(os.sep)[...] stuff.

::: js/src/tests/test262-update.py
@@ +330,5 @@
>          if relPath == ".":
>              continue
>  
>          # Skip creating a "prs" directory if it already exists
> +        if relPath != "prs" and relPath != "local" or not os.path.exists(os.path.join(test262OutDir, relPath)):

This expression definitely needs parentheses.

The original was if not A or B and C, which I guess is left associative so it means (not A or B) and C. The new version is B and C or not A, so (B and C) or not A, which is different. So going back to what this is doing... the comment says skip creating prs/ if it exists. The old code was messed up, if relPath was anything but prs it would try to create the directory whether it existed or not. 

So what is the intention? For most directories, it seems like you'd want to create the directory unless it already exists. Do you not want to create prs/ or local/ at all, ever? If so, then this should be

  if relPath not in ("prs", "local") and not os.path.exists(...):

@@ +374,5 @@
>      import subprocess
>  
>      # TOOD: fail if it's in the master branch? or require a branch name?
>  
> +    # Checks for unstaged or non committed files. A clean branch provices a clean status.

*provides

@@ +425,5 @@
>  
>      for f in files.splitlines():
>          # Capture the subdirectories names to recreate the file tree
>          # TODO: join the file tree with -- instead of multiple subfolders?
>          fileTree = os.path.join(inDir, *f.split(os.sep)[:-1])

It's hard to see what this is doing. Is this the same as fileTree = os.path.join(indir, os.path.dirname(f)) ?

@@ +429,5 @@
>          fileTree = os.path.join(inDir, *f.split(os.sep)[:-1])
>          if not os.path.exists(fileTree):
>              os.makedirs(fileTree)
>  
>          shutil.copyfile(os.path.join(srcDir, f), os.path.join(fileTree, f.split(os.sep)[-1]))

and this must be os.path.join(fileTree, os.path.basename(f))

Or you could do

  (fileDir, fileBase) = os.path.split(f)

and then use those instead of dirname and basename.

@@ +432,5 @@
>  
>          shutil.copyfile(os.path.join(srcDir, f), os.path.join(fileTree, f.split(os.sep)[-1]))
>  
> +    # Extras from Test262. Copy the current support folders - including the
> +    # harness - for a proper convertion process

*conversion

Comment 32

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

Comment on attachment 8894576 [details] [diff] [review]
0007-fixup-Update-export-script-to-add-copyright-lines.patch

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

::: js/src/tests/test262-export.py
@@ +271,5 @@
> +    if not re.match(r'\/\/\s+Copyright.*\. All rights reserved.', source):
> +        year = date.today().year
> +        lines.append("// Copyright (C) %s Mozilla Corporation. All rights reserved." % year)
> +        lines.append("// This code is governed by the BSD license found in the LICENSE file.")
> +        lines.append("\n")

Do you have an official word on what license to use? Because our test files are normally in the public domain:

// Any copyright is dedicated to the Public Domain.
// http://creativecommons.org/licenses/publicdomain/

but maybe we have some different policy here?

Comment 33

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

Comment on attachment 8895059 [details] [diff] [review]
0009-fixup-Applying-changes-from-review-feedback.patch

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

::: js/src/tests/test262-export.py
@@ +50,2 @@
>  
> +    def matchFn(matchobj):

Call this replaceFn instead.

@@ +50,4 @@
>  
> +    def matchFn(matchobj):
> +        actual = matchobj.group(1)
> +        expected = matchobj.group(2)

(actual, expected) = matchobj.groups()

@@ +54,3 @@
>  
> +        if actual == expected and actual in ["0", "true", "null"]:
> +            return

I'm surprised this worked; I would've expected it to demand a string instead of None. return "" to be explicit.

@@ +57,3 @@
>  
> +        return matchobj.group()
> +    

trailing whitespace

@@ +65,2 @@
>  
> +    return re.sub(r'([.\W]?)reportCompare(\W?)', r'\1assert.sameValue\2', newSource)

return re.sub(r'\breportCompare\b', 'assert.sameValue', newSource)

@@ +77,5 @@
>      comments = None
>      module = False
>  
>      # should capture conditions to skip
> +    matchesSkip = re.search(r'skip-if\((.*)?\)', reftest)

Not (.*)? -- that's redundant. I wanted a minimal (non-greedy) match.

re.search(r'skip-if\((.*?)\)', reftest)

Oh! But you don't want that; it looks like we have lots of skip-if expressions with parentheses in them. Ok, never mind then. To do this properly would require counting parens, and we certainly don't want to do that. Your original (just .* without the added ?) is best.

@@ +233,1 @@
>      

Some more trailing whitespace.

@@ +295,5 @@
>      else:
>          return "\n".join(lines) + source
>  
>  def exportTest262(args):
> +    from io import open

don't need this any more

Comment 34

[Leo Balter]

(In reply to Steve Fink [:sfink] [:s:] from comment #32)
> Comment on attachment 8894576 [details] [diff] [review]
> 0007-fixup-Update-export-script-to-add-copyright-lines.patch
> 
> Review of attachment 8894576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/test262-export.py
> @@ +271,5 @@
> > +    if not re.match(r'\/\/\s+Copyright.*\. All rights reserved.', source):
> > +        year = date.today().year
> > +        lines.append("// Copyright (C) %s Mozilla Corporation. All rights reserved." % year)
> > +        lines.append("// This code is governed by the BSD license found in the LICENSE file.")
> > +        lines.append("\n")
> 
> Do you have an official word on what license to use? Because our test files
> are normally in the public domain:
> 
> // Any copyright is dedicated to the Public Domain.
> // http://creativecommons.org/licenses/publicdomain/
> 
> but maybe we have some different policy here?

This is an annoying rule from Test262 that I'm still trying to find the legal feedback to get rid of these copyrights header in there.

The proposed header here is based on other contributions from Mozilla to Test262, it would follow consistency.

The material in the TC39 project is also offered like the specs with royalty free agreement from Ecma members.

We've had several discussions on these copyright lines but I don't have the legal degree to say anything else is valid for Test262.

Comment 35

[Leo Balter]

Created attachment 8895907 [details] [diff] [review]
0011-mocking-a-local-import.patch

just for reference testing the local import

Comment 36

[Leo Balter]

Created attachment 8895908 [details] [diff] [review]
0012-fixup-apply-review-feedback-for-test262-update.py.patch

Comment 37

[Leo Balter]

Created attachment 8895909 [details] [diff] [review]
0013-fixup-test262-export.patch

Comment 38

[Leo Balter]

(In reply to Steve Fink [:sfink] [:s:] from comment #31)
> Comment on attachment 8892116 [details] [diff] [review]
> 0006-fixup-Bug-1374290-Import-diff-contents-from-a-local-.patch
> 
> Review of attachment 8892116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess I was probably just supposed to review the changes here, but I'd
> really rather you change the f.split(os.sep)[...] stuff.

I changed them expect for old the line with

> `if "/".join(os.path.normpath(os.getcwd()).split(os.sep)[-3:]) != "js/src/tests":`

I can improve this, but I wanted to address the other parts first.

> 
> ::: js/src/tests/test262-update.py
> @@ +330,5 @@
> >          if relPath == ".":
> >              continue
> >  
> >          # Skip creating a "prs" directory if it already exists
> > +        if relPath != "prs" and relPath != "local" or not os.path.exists(os.path.join(test262OutDir, relPath)):
> 
> This expression definitely needs parentheses.
> 
> The original was if not A or B and C, which I guess is left associative so
> it means (not A or B) and C. The new version is B and C or not A, so (B and
> C) or not A, which is different. So going back to what this is doing... the
> comment says skip creating prs/ if it exists. The old code was messed up, if
> relPath was anything but prs it would try to create the directory whether it
> existed or not. 
> 
> So what is the intention? For most directories, it seems like you'd want to
> create the directory unless it already exists. Do you not want to create
> prs/ or local/ at all, ever? If so, then this should be
> 
>   if relPath not in ("prs", "local") and not os.path.exists(...):

This is much better, fixed.

> 
> @@ +374,5 @@
> >      import subprocess
> >  
> >      # TOOD: fail if it's in the master branch? or require a branch name?
> >  
> > +    # Checks for unstaged or non committed files. A clean branch provices a clean status.
> 
> *provides

fixed.

> 
> @@ +425,5 @@
> >  
> >      for f in files.splitlines():
> >          # Capture the subdirectories names to recreate the file tree
> >          # TODO: join the file tree with -- instead of multiple subfolders?
> >          fileTree = os.path.join(inDir, *f.split(os.sep)[:-1])
> 
> It's hard to see what this is doing. Is this the same as fileTree =
> os.path.join(indir, os.path.dirname(f)) ?


yes, and this is fixed now using the suggestion.

The TODO is asking if we should use `--` to avoid a deep folder tree. See the patch 0011.


> 
> @@ +429,5 @@
> >          fileTree = os.path.join(inDir, *f.split(os.sep)[:-1])
> >          if not os.path.exists(fileTree):
> >              os.makedirs(fileTree)
> >  
> >          shutil.copyfile(os.path.join(srcDir, f), os.path.join(fileTree, f.split(os.sep)[-1]))
> 
> and this must be os.path.join(fileTree, os.path.basename(f))
> 
> Or you could do
> 
>   (fileDir, fileBase) = os.path.split(f)
> 
> and then use those instead of dirname and basename.

I fixed using the first suggestion, it seems more clean.

> 
> @@ +432,5 @@
> >  
> >          shutil.copyfile(os.path.join(srcDir, f), os.path.join(fileTree, f.split(os.sep)[-1]))
> >  
> > +    # Extras from Test262. Copy the current support folders - including the
> > +    # harness - for a proper convertion process
> 
> *conversion

thanks, fixed!

Comment 39

[Leo Balter]

(In reply to Steve Fink [:sfink] [:s:] from comment #33)
> Comment on attachment 8895059 [details] [diff] [review]
> 0009-fixup-Applying-changes-from-review-feedback.patch
> 
> Review of attachment 8895059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/test262-export.py
> @@ +50,2 @@
> >  
> > +    def matchFn(matchobj):
> 
> Call this replaceFn instead.

done.


> 
> @@ +50,4 @@
> >  
> > +    def matchFn(matchobj):
> > +        actual = matchobj.group(1)
> > +        expected = matchobj.group(2)
> 
> (actual, expected) = matchobj.groups()

This change was causing a `ValueError: too many values to unpack`.

> 
> @@ +54,3 @@
> >  
> > +        if actual == expected and actual in ["0", "true", "null"]:
> > +            return
> 
> I'm surprised this worked; I would've expected it to demand a string instead
> of None. return "" to be explicit.

I was using an explicit `return ""` before, but when I noticed None was working as well I tried to make it clean.

This is fixed now.

> 
> @@ +57,3 @@
> >  
> > +        return matchobj.group()
> > +    
> 
> trailing whitespace

fixed.

> 
> @@ +65,2 @@
> >  
> > +    return re.sub(r'([.\W]?)reportCompare(\W?)', r'\1assert.sameValue\2', newSource)
> 
> return re.sub(r'\breportCompare\b', 'assert.sameValue', newSource)

fixed.

> 
> @@ +77,5 @@
> >      comments = None
> >      module = False
> >  
> >      # should capture conditions to skip
> > +    matchesSkip = re.search(r'skip-if\((.*)?\)', reftest)
> 
> Not (.*)? -- that's redundant. I wanted a minimal (non-greedy) match.
> 
> re.search(r'skip-if\((.*?)\)', reftest)
> 
> Oh! But you don't want that; it looks like we have lots of skip-if
> expressions with parentheses in them. Ok, never mind then. To do this
> properly would require counting parens, and we certainly don't want to do
> that. Your original (just .* without the added ?) is best.

I reverted this to the original.

> 
> @@ +233,1 @@
> >      
> 
> Some more trailing whitespace.

fixed.

> 
> @@ +295,5 @@
> >      else:
> >          return "\n".join(lines) + source
> >  
> >  def exportTest262(args):
> > +    from io import open
> 
> don't need this any more

done.

---

Thanks for the review!

Comment 40

[Leo Balter]

Created attachment 8896416 [details] [diff] [review]
0014-fixup-Tests-for-the-export-script.patch

This patch adds basic tests for the test262-export script and open a path to add other tests in the same folder.

Comment 41

[Leo Balter]

Before I commit more time into it, I'd like to ask you about testing the import script and check if you're interested in pursuing this feature.

To test the import script from a local branch, the script would need to:

1. Clone test262 to a local temporary folder
2. Create a new branch
3. Copy the fixture files into it.
4. Commit them.

Now I would be able to run the import script and verify I imported the files properly, with the expected conversion.

Some similar structure would be necessary for testing the other import scripts, it would need to verify changes from a remote fork.

This process might be complex but reflects the best opportunity to test these scripts as well.

Comment 42

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

Comment on attachment 8894576 [details] [diff] [review]
0007-fixup-Update-export-script-to-add-copyright-lines.patch

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

Sounds like you've looked into the question of what copyright to use, so it sounds fine to me.

Comment 43

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

Comment on attachment 8896416 [details] [diff] [review]
0014-fixup-Tests-for-the-export-script.patch

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

::: js/src/tests/test/run.py
@@ +21,5 @@
> +
> +    def getFiles(self, path):
> +        names = []
> +        for root, _, fileNames in os.walk(path):
> +            for fileName in filter(lambda x: x[0] != '.', fileNames):

Hm, I can envision this failing due to temporary files, etc. Instead of donig a lambda here, can you define something like

  def isTestFile(filename):
    if filename.startswith('.'):
      return false
    if filename.startswith('#'):
      return false
    if filename.endswith('~'):
      return false
    return true

and then pass that in place of the lambda?

Comment 44

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

(In reply to Leo Balter from comment #41)
> Before I commit more time into it, I'd like to ask you about testing the
> import script and check if you're interested in pursuing this feature.

Yes, I think it would be good to have a test for this procedure.

> 
> To test the import script from a local branch, the script would need to:
> 
> 1. Clone test262 to a local temporary folder
> 2. Create a new branch
> 3. Copy the fixture files into it.
> 4. Commit them.
> 
> Now I would be able to run the import script and verify I imported the files
> properly, with the expected conversion.
> 
> Some similar structure would be necessary for testing the other import
> scripts, it would need to verify changes from a remote fork.
> 
> This process might be complex but reflects the best opportunity to test
> these scripts as well.

Yes, sounds good.

Comment 45

[Leo Balter]

Created attachment 8898403 [details] [diff] [review]
0015-fixup-fixup-Tests-for-the-export-script.patch

apply review feedback on the tests for the export script

Comment 46

[Leo Balter]

Created attachment 8898404 [details] [diff] [review]
0016-Bug-1374290-Test-the-local-import-script.-r-sfink.patch

Comment 47

[Leo Balter]

I can do a rebase and squash all the fixup commits, just keep me posted when it's ok to do it (I don't want to mess with all the previously reviewed parts)

Comment 48

[Leo Balter]

Created attachment 8898839 [details] [diff] [review]
0016-Bug-1374290-Test-the-local-import-script.-r-sfink.patch

Comment 49

[Leo Balter]

Created attachment 8898859 [details] [diff] [review]
0017-Bug-1374290-Fix-case-sensitive-file-name.-r-sfink.patch

Comment 50

[Leo Balter]

Created attachment 8899014 [details] [diff] [review]
0001-Bug-1374290-Import-diff-contents-from-a-local-Test26.patch

Comment 51

[Leo Balter]

Created attachment 8899015 [details] [diff] [review]
0002-Bug-1374290-Test262-export-script.-r-shu.patch

Comment 52

[Leo Balter]

Created attachment 8899016 [details] [diff] [review]
0003-Bug-1374290-Test-the-local-import-script.-r-sfink.patch

Comment 53

[Leo Balter]

Created attachment 8899018 [details] [diff] [review]
0004-Bug-1374290-Update-Test262.-r-sfink.patch

Comment 54

[Leo Balter]

Created attachment 8899019 [details] [diff] [review]
0005-Bug-1374290-Update-the-skip-list.-r-sfink.patch

Comment 55

[Leo Balter]

to finish this work I submitted 5 patches with:

- The tests for the import script from a local branch (previously reviewed commits, squashed)
- The export script with tests (previously reviewed commits, squashed) + the changes on https://bugzilla.mozilla.org/attachment.cgi?id=8898403&action=edit
- The tests for the import script (not reviewed yet)
- Update Test262 tests
- Update the skip list

I rebased it with the master branch, so a few commits are not present anymore, like https://bugzilla.mozilla.org/attachment.cgi?id=8898859&action=edit (fixed elsewhere) and the first 2 active attachments already merged.

Comment 56

[Leo Balter]

I'll verify the mozilla-try results on Monday for the updated Test262 files.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55041a96aabee03c96a0159940dd1d4dc53ae6dd

Comment 57

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

Comment on attachment 8899016 [details] [diff] [review]
0003-Bug-1374290-Test-the-local-import-script.-r-sfink.patch

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

::: js/src/tests/test/run.py
@@ +60,5 @@
> +            subprocess.check_call(['git', '-C', cloneDir, 'add', '.'])
> +            subprocess.check_call(['git', '-C', cloneDir, 'commit', '-m', '"local foo"'])
> +
> +            # Run import script
> +            print("%s --local %s --out %s" % (importExec, cloneDir, OUT_DIR))            

whitespace at end of line

@@ +108,5 @@
> +        with open(filePath, "rb") as file:
> +            expected = file.read()
> +        
> +        expected = expected.replace('{{folder}}', folder)   
> +        self.assertMultiLineEqual(output, expected)

more whitespace

Comment 58

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

Comment on attachment 8899018 [details] [diff] [review]
0004-Bug-1374290-Update-Test262.-r-sfink.patch

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

I'm not sure what to do with this one other than blindly mark r+.

Comment 59

[Leo Balter]

> I'm not sure what to do with this one other than blindly mark r+.

That's fine, those are just files imported from Test262, nothing else.

Comment 60

[Tom Schuster [:evilpie]]

What's the status here? Are we going to update soon?

Comment 61

[Leo Balter]

Created attachment 8907267 [details] [diff] [review]
0003-Bug-1374290-Test-the-local-import-script.-r-sfink.patch

fixed without trailing spaces

Comment 62

[Leo Balter]

I missed a small fixup but this is done now.

Comment 63

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

Comment on attachment 8907267 [details] [diff] [review]
0003-Bug-1374290-Test-the-local-import-script.-r-sfink.patch

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

Works for me.

Comment 64

[André Bargull [:anba]]

NI Leo for Tom's question in #comment 60.

Comment 65

[André Bargull [:anba]]

Test262 was updated in bug 1407584.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec879327cd7c
Import diff contents from a local Test262 folder. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6f7d9bc6f0
Test262 export script. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/5306302faf01
Test the local import script. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9789b0ad7982
Update the skip list. r=sfink

Comment 67

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

Ok, I think I managed to work out what should land and what shouldn't, of the batch of patches attached to this bug.

It required some fixups -- eg, there was a test of a file named '#ignore.js', but the reftest parser treats # preceded by whitespace as a comment; there is no way to escape it. So I removed that file. Some other rebasing was required, and I did not do a test262 update since bug 1407584 just did one.

Comment 68

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

Filed bug 1408583 for a nuisance issue noticed while doing a trial update.

Comment 69

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ec879327cd7c
https://hg.mozilla.org/mozilla-central/rev/0a6f7d9bc6f0
https://hg.mozilla.org/mozilla-central/rev/5306302faf01
https://hg.mozilla.org/mozilla-central/rev/9789b0ad7982