Closed Bug 1374290 Opened 8 years ago Closed 6 years ago

test262-update.py imports Pull Requests

Categories

(Core :: JavaScript Engine, defect, P3)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox57 --- wontfix

People

(Reporter: leonardo.balter, Assigned: leonardo.balter)

References

(Blocks 2 open bugs)

Details

Attachments

(8 files, 18 obsolete files)

0001-Bug-1374290-Import-Pull-Requests-from-Test262.-r-shu.patch
8.55 KB, patch
shu
: review+
Details | Diff | Splinter Review
0002-Bug-1374290-Fix-encoding-on-imported-PR-files.-r-shu.patch
7.35 KB, patch
shu
: review+
Details | Diff | Splinter Review
0011-mocking-a-local-import.patch
20.99 KB, patch
Details | Diff | Splinter Review
0001-Bug-1374290-Import-diff-contents-from-a-local-Test26.patch
13.53 KB, patch
sfink
: review+
Details | Diff | Splinter Review
0002-Bug-1374290-Test262-export-script.-r-shu.patch
24.10 KB, patch
sfink
: review+
Details | Diff | Splinter Review
0004-Bug-1374290-Update-Test262.-r-sfink.patch
9.10 MB, patch
sfink
: review+
Details | Diff | Splinter Review
0005-Bug-1374290-Update-the-skip-list.-r-sfink.patch
31.96 KB, patch
sfink
: review+
Details | Diff | Splinter Review
0003-Bug-1374290-Test-the-local-import-script.-r-sfink.patch
28.12 KB, patch
sfink
: review+
Details | Diff | Splinter Review
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 ```
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.
Flags: needinfo?(shu)
Attachment #8879147 - Flags: review?(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.
Attachment #8879147 - Flags: review?(shu)
Flags: needinfo?(shu)
> 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
the updated patch after the review feedback
Attachment #8879147 - Attachment is obsolete: true
Flags: needinfo?(shu)
Attachment #8881494 - Flags: review?(shu)
Flags: needinfo?(shu)
Flags: needinfo?(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
Attachment #8881494 - Flags: review?(shu) → review+
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.
Flags: needinfo?(shu)
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) ```
Attachment #8881494 - Attachment is obsolete: true
Attachment #8881941 - Flags: review?(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!
Attachment #8881941 - Flags: review?(shu) → review+
Thank you!
this fixes an encoding issue found writing files with non-ascii chars.
Attachment #8884088 - Flags: review?(shu)
this patch adds an import script for local branches. It's working but it might need some discussion before landing. Reviews are pretty welcome!
Attachment #8884089 - Flags: review?(shu)
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 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.
Attachment #8884088 - Flags: review?(shu) → review+
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.
Attachment #8884089 - Flags: review?(shu)
> 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!
> 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!
This patch introduces the export script
Attachment #8892100 - Flags: review?(shu)
You can use the contents here to try the export script
Review updates for the import script
Attachment #8892116 - Flags: review?(shu)
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.
Keywords: leave-open
Pushed by shu@rfrn.org: 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
Attachment #8892100 - Flags: review?(shu) → review?(sphink)
Attachment #8892116 - Flags: review?(shu) → review?(sphink)
Update export script to add the copyright header lines
Attachment #8894576 - Flags: review?(sphink)
fixup for the samples to be used with the patch 0007
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.
Attachment #8892100 - Flags: review?(sphink)
(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.
Thanks for the comprehensive review! I applied all the changes in this patch and I can rebase and squash it anytime you request.
Attachment #8895059 - Flags: review?(sphink)
Attached patch 0010-fixup-mock-files.patch (obsolete) — — Splinter Review
extracted files
Blocks: 1388510
I also created the new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1388510
No longer blocks: 1388510
Blocks: 1388510
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
Attachment #8892116 - Flags: review?(sphink) → review+
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 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
Attachment #8895059 - Flags: review?(sphink) → review+
(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.
just for reference testing the local import
Attachment #8895908 - Flags: review?(sphink)
Attached patch 0013-fixup-test262-export.patch (obsolete) — — Splinter Review
Attachment #8895909 - Flags: review?(sphink)
(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!
(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!
This patch adds basic tests for the test262-export script and open a path to add other tests in the same folder.
Attachment #8896416 - Flags: review?(sphink)
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.
Flags: needinfo?(sphink)
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.
Attachment #8894576 - Flags: review?(sphink) → review+
Attachment #8895908 - Flags: review?(sphink) → review+
Attachment #8895909 - Flags: review?(sphink) → review+
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?
Attachment #8896416 - Flags: review?(sphink) → review+
(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.
Flags: needinfo?(sphink)
apply review feedback on the tests for the export script
Attachment #8898403 - Flags: review?(sphink)
Attachment #8898404 - Flags: review?(sphink)
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)
Attachment #8898404 - Attachment is obsolete: true
Attachment #8898404 - Flags: review?(sphink)
Attachment #8898839 - Flags: review?(sphink)
Attachment #8898859 - Flags: review?(sphink)
Attachment #8884089 - Attachment is obsolete: true
Attachment #8892116 - Attachment is obsolete: true
Attachment #8895908 - Attachment is obsolete: true
Attachment #8899014 - Flags: review?(sphink)
Attachment #8892100 - Attachment is obsolete: true
Attachment #8892101 - Attachment is obsolete: true
Attachment #8894576 - Attachment is obsolete: true
Attachment #8894578 - Attachment is obsolete: true
Attachment #8895059 - Attachment is obsolete: true
Attachment #8895060 - Attachment is obsolete: true
Attachment #8895909 - Attachment is obsolete: true
Attachment #8896416 - Attachment is obsolete: true
Attachment #8898403 - Attachment is obsolete: true
Attachment #8898403 - Flags: review?(sphink)
Attachment #8899015 - Flags: review?(sphink)
Attachment #8898839 - Attachment is obsolete: true
Attachment #8898859 - Attachment is obsolete: true
Attachment #8898839 - Flags: review?(sphink)
Attachment #8898859 - Flags: review?(sphink)
Attachment #8899016 - Flags: review?(sphink)
Attachment #8899018 - Flags: review?(sphink)
Attachment #8899019 - Flags: review?(sphink)
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.
I'll verify the mozilla-try results on Monday for the updated Test262 files. https://treeherder.mozilla.org/#/jobs?repo=try&revision=55041a96aabee03c96a0159940dd1d4dc53ae6dd
Attachment #8899014 - Flags: review?(sphink) → review+
Attachment #8899015 - Flags: review?(sphink) → review+
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
Attachment #8899016 - Flags: review?(sphink) → review+
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+.
Attachment #8899018 - Flags: review?(sphink) → review+
Attachment #8899019 - Flags: review?(sphink) → review+
> 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.
What's the status here? Are we going to update soon?
fixed without trailing spaces
Attachment #8899016 - Attachment is obsolete: true
Attachment #8907267 - Flags: review?(sphink)
I missed a small fixup but this is done now.
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.
Attachment #8907267 - Flags: review?(sphink) → review+
Blocks: 1361526
Assignee: nobody → leonardo.balter
NI Leo for Tom's question in #comment 60.
Flags: needinfo?(leonardo.balter)
status-firefox57: --- → wontfix
Priority: -- → P3
Test262 was updated in bug 1407584.
Flags: needinfo?(leonardo.balter)
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.
Filed bug 1408583 for a nuisance issue noticed while doing a trial update.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

Steve, do you think we can close this bug now? Not sure if you are the right one to ask, but I thought I would start with you.

Flags: needinfo?(sdetar) → needinfo?(sphink)

yes, this is done

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(sphink)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: