Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
478163002:
test/test262: update testcfg.py for new test262 (Closed)
|
Created:
6 years, 4 months ago by smikes Modified:
6 years, 4 months ago Reviewers:
Jakob Kummerow CC:
v8-dev, Dmitry Lomov (no reviews), Michael Starzinger, Yang, arv (Not doing code reviews), Michael Achenbach Base URL:
git@github.com:smikes/v8.git@master Project:
v8 Visibility:
Public. |
Descriptiontest/test262: update testcfg.py for new test262
testcfg.py:
- update revision and MD5
- remove non-mandatory harness files
- use test parser distributed with test262
- new attribute `suite.harnesspath`
- new method GetIncludesForTest
- GetSourceForTest: cache source on testcase
- IsNegativeTest: use parseTestRecord
- use 7-char sha hash [1]
- DRY setting up paths to test262 suite, harness, etc
- clean up helper fns
harness-adapter.js:
- add $DONE function to adapter [2]
1: github tar file has 7-char sha embedded in dir name
script cannot find directory to rename if they don't match exactly
2: test262 uses a `$DONE` function for async tests with
semantics like those of mocha's `done`.
Briefly: done(arg) => if (arg) { /* failure */ }
Implemented a version of this for v8, using v8-specific
api (`print`, `quit`)
BUG=v8:3513
LOG=N
R=jkummerow@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=23313
Patch Set 1 #
Total comments: 22
Patch Set 2 : Incorporate code review comments from Jakob #
Total comments: 8
Patch Set 3 : Use 'imp' for importing; additional cleanup #
Total comments: 1
Patch Set 4 : Remove vestige of caching test source #Patch Set 5 : Remove vestige of source caching #
Total comments: 6
Patch Set 6 : Rebased with final (?) changes #
Messages
Total messages: 13 (0 generated)
IIUC, not all tests in this test262 revision currently pass with V8. Tests that don't pass must be annotated accordingly in test/test262/test262.status, so that running the suite finishes without errors. https://codereview.chromium.org/478163002/diff/1/test/test262/harness-adapt.js File test/test262/harness-adapt.js (right): https://codereview.chromium.org/478163002/diff/1/test/test262/harness-adapt.j... test/test262/harness-adapt.js:85: $DONE = function __v8_$DONE__(arg){ why not just: function $DONE(arg) { https://codereview.chromium.org/478163002/diff/1/test/test262/harness-adapt.j... test/test262/harness-adapt.js:87: print('FAILED! Error: ' + arguments[0]); why not s/arguments[0]/arg/ ? https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:49: # import parseTestRecord Nit: comments should have proper capitalization and punctuation such as a trailing full stop. (Again several times below.) https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:50: sys.path.append(os.path.abspath(os.path.join('test', 'test262', *TEST_262_PYTHON_ROOT))) nit: 80col please. Also, if we need to muck with sys.path at all, please restore it when done, i.e.: original_sys_path = sys.path sys.path.append(...) import ... sys.path = original_sys_path https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:52: from parseTestRecord import parseTestRecord nit: "... as ParseTestRecord" to adhere to CapitalizedFunctionNameStyle https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:67: for f in TEST_262_HARNESS] + [ Please move the last '[' to the next line for better readability (or simply stick with the way the previous code split lines using a second += assignment). https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:96: try: The V8 project as a whole is not too fond of exceptions, especially for regularly occuring non-error conditions. In this case the try..except seems easily avoidable: test_record = ParseTestRecord(self.GetSourceForTest(testcase), testcase.path) if "includes" in test_record: return [os.path.join(self.harnesspath, f) for f in test_record["includes"])] else: return [] https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:97: testRecord = parseTestRecord(self.GetSourceForTest(testcase), testcase.path) nit: 80col https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:105: return testcase.source No need to cache this; the framework is careful not to call GetSourceForTest repeatedly (as it assumes that this operation might be expensive). https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:113: testRecord = parseTestRecord(self.GetSourceForTest(testcase), testcase.path) nit: s/testRecord/test_record/ https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:114: try: Again, I'd prefer 'if "negative" in test_record:' over try..except. Also, if you want to cache something, cache this, as IsNegativeTest() is actually called twice for each test case. (Use "if hasattr(...)" rather than "except AttributeError", please.)
In every case "why not <x>?" the answer is "because I am not familiar enough with python". I will incorporate your suggestions and update the patch, thanks.
Thanks for your comments. I will upload a new patch soon. https://codereview.chromium.org/478163002/diff/1/test/test262/harness-adapt.js File test/test262/harness-adapt.js (right): https://codereview.chromium.org/478163002/diff/1/test/test262/harness-adapt.j... test/test262/harness-adapt.js:85: $DONE = function __v8_$DONE__(arg){ On 2014/08/18 11:55:46, Jakob wrote: > why not just: > function $DONE(arg) { Done. https://codereview.chromium.org/478163002/diff/1/test/test262/harness-adapt.j... test/test262/harness-adapt.js:87: print('FAILED! Error: ' + arguments[0]); On 2014/08/18 11:55:47, Jakob wrote: > why not s/arguments[0]/arg/ ? Done. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:49: # import parseTestRecord On 2014/08/18 11:55:47, Jakob wrote: > Nit: comments should have proper capitalization and punctuation such as a > trailing full stop. (Again several times below.) Removed needless comment. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:50: sys.path.append(os.path.abspath(os.path.join('test', 'test262', *TEST_262_PYTHON_ROOT))) On 2014/08/18 11:55:47, Jakob wrote: > nit: 80col please. > > Also, if we need to muck with sys.path at all, please restore it when done, > i.e.: > > original_sys_path = sys.path > sys.path.append(...) > import ... > sys.path = original_sys_path Fixed both. Is there a more idiomatic way of importing a file from a directory not in sys.path? https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:52: from parseTestRecord import parseTestRecord On 2014/08/18 11:55:47, Jakob wrote: > nit: "... as ParseTestRecord" to adhere to CapitalizedFunctionNameStyle Done. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:67: for f in TEST_262_HARNESS] + [ On 2014/08/18 11:55:47, Jakob wrote: > Please move the last '[' to the next line for better readability (or simply > stick with the way the previous code split lines using a second += assignment). Done. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:96: try: On 2014/08/18 11:55:47, Jakob wrote: > The V8 project as a whole is not too fond of exceptions, especially for > regularly occuring non-error conditions. In this case the try..except seems > easily avoidable: > > test_record = ParseTestRecord(self.GetSourceForTest(testcase), > testcase.path) > if "includes" in test_record: > return [os.path.join(self.harnesspath, f) > for f in test_record["includes"])] > else: > return [] Done. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:105: return testcase.source On 2014/08/18 11:55:47, Jakob wrote: > No need to cache this; the framework is careful not to call GetSourceForTest > repeatedly (as it assumes that this operation might be expensive). Yes, but I was calling GetSourceForTest repeatedly (including from IsNegativeTest). New function GetTestRecord calls ParseTestRecord once and stores test_record attribute on testcase (see updated patch, soon) https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:113: testRecord = parseTestRecord(self.GetSourceForTest(testcase), testcase.path) On 2014/08/18 11:55:47, Jakob wrote: > nit: s/testRecord/test_record/ Done. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:114: try: On 2014/08/18 11:55:47, Jakob wrote: > Again, I'd prefer 'if "negative" in test_record:' over try..except. > > Also, if you want to cache something, cache this, as IsNegativeTest() is > actually called twice for each test case. (Use "if hasattr(...)" rather than > "except AttributeError", please.) Done.
I forgot to mention that you'll have to sign the CLA before we can accept your patch, see https://code.google.com/p/v8/wiki/Contributing. https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/1/test/test262/testcfg.py#newc... test/test262/testcfg.py:50: sys.path.append(os.path.abspath(os.path.join('test', 'test262', *TEST_262_PYTHON_ROOT))) On 2014/08/18 16:04:27, smikes wrote: > On 2014/08/18 11:55:47, Jakob wrote: > > nit: 80col please. > > > > Also, if we need to muck with sys.path at all, please restore it when done, > > i.e.: > > > > original_sys_path = sys.path > > sys.path.append(...) > > import ... > > sys.path = original_sys_path > > Fixed both. Is there a more idiomatic way of importing a file > from a directory not in sys.path? There is imp, https://docs.python.org/2/library/imp.html, which we use in https://code.google.com/p/v8/source/browse/branches/bleeding_edge/tools/testr.... It's more verbose but less hacky, does that count as idiomatic?
Thanks. I e-signed the CLA. I am currently re-running the full test262 suite to make sure I caught all the expected failures. Will investigate imp and add at least one more known failure (https://code.google.com/p/v8/issues/detail?id=705) in about 2 hours.
Looking pretty good now; I have a few more comments. https://codereview.chromium.org/478163002/diff/20001/test/test262/test262.status File test/test262/test262.status (right): https://codereview.chromium.org/478163002/diff/20001/test/test262/test262.sta... test/test262/test262.status:47: 'S22.1.3.6_T1': [FAIL], Can you alpha-sort these please (within each section)? https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py#... test/test262/testcfg.py:47: TEST_262_PYTHON_ROOT = ["data", "tools", "packaging"] Since this is now only used in the line right below, I'd inline it there. But come to think of it, there's more cleanup we should do: - os.path.abspath("test/test262") is essentially what's passed in to GetSuite as "root". We should re-use that rather than hard-code and duplicate it. Which means the import trickery must go into GetSuite (which shouldn't hurt, AFAICT). - It seems a bit arbitrary which variables are pulled out and which aren't. - Having four paths named <x>_ROOT is a bit confusing. Putting it all together gives us something like: TEST_262_TESTS_PATH = ["data", "test", "suite"] TEST_262_HARNESS_PATH = ["data", "test", "harness"] TEST_262_TOOLS_PATH = ["data", "tools", "packaging"] ... def GetSuite(name, root): ... sys.path.append(os.path.join(root, *TEST_262_TOOLS_PATH)) # or the 'imp' version you're coming up with ... return Test262TestSuite(name, root) https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py#... test/test262/testcfg.py:94: if hasattr(testcase, "test_record"): nit: one return statement is more readable, and in this case easy to achieve: if not hasattr(testcase, "test_record"): testcase.test_record = ... return testcase.test_record https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py#... test/test262/testcfg.py:116: return test_record['negative'] one more nit: the rest of this file uses double quotes around strings, let's be consistent.
Implemented the changes you suggested. I just noticed that I didn't rename _PYTHON_PATH to _TOOLS_PATH, but if you still think I should, I will. https://codereview.chromium.org/478163002/diff/20001/test/test262/test262.status File test/test262/test262.status (right): https://codereview.chromium.org/478163002/diff/20001/test/test262/test262.sta... test/test262/test262.status:47: 'S22.1.3.6_T1': [FAIL], On 2014/08/18 17:18:39, Jakob wrote: > Can you alpha-sort these please (within each section)? Done. https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py#... test/test262/testcfg.py:47: TEST_262_PYTHON_ROOT = ["data", "tools", "packaging"] On 2014/08/18 17:18:39, Jakob wrote: > We should re-use that rather than hard-code and duplicate it. Which > means the import trickery must go into GetSuite (which shouldn't hurt, AFAICT). I think it can't go into GetSuite because GetSuite has to succeed in order for DownloadData to run, and we would like to be able to --download-data and run in the same step. I pulled it into its own function LoadParseTestRecord > - It seems a bit arbitrary which variables are pulled out and which aren't. > - Having four paths named <x>_ROOT is a bit confusing. > > Putting it all together gives us something like: > > TEST_262_TESTS_PATH = ["data", "test", "suite"] > TEST_262_HARNESS_PATH = ["data", "test", "harness"] > TEST_262_TOOLS_PATH = ["data", "tools", "packaging"] > > ... > > def GetSuite(name, root): > ... > sys.path.append(os.path.join(root, *TEST_262_TOOLS_PATH)) > # or the 'imp' version you're coming up with > ... > return Test262TestSuite(name, root) https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py#... test/test262/testcfg.py:94: if hasattr(testcase, "test_record"): On 2014/08/18 17:18:39, Jakob wrote: > nit: one return statement is more readable, and in this case easy to achieve: > > if not hasattr(testcase, "test_record"): > testcase.test_record = ... > return testcase.test_record Done. https://codereview.chromium.org/478163002/diff/20001/test/test262/testcfg.py#... test/test262/testcfg.py:116: return test_record['negative'] On 2014/08/18 17:18:39, Jakob wrote: > one more nit: the rest of this file uses double quotes around strings, let's be > consistent. Done.
https://codereview.chromium.org/478163002/diff/40001/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/40001/test/test262/testcfg.py#... test/test262/testcfg.py:119: testcase.source = f.read() this is the worse of both worlds; stores a copy of the test source, but doesn't re-use it. I will remove this.
LGTM with some last comments. https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py#... test/test262/testcfg.py:97: raise Error("Cannot load parseTestRecord; you may need to --download-data for test262") The only way this can happen is when there was an exception when trying to import the module, right? So you can move it to the right place. And please respect the 80 column limit. Don't be afraid of line breaks! They're really easy in Python: try: ... except: raise Error("Cannot load parseTestRecord; you may need to " "--download-data for test262") finally: ... https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py#... test/test262/testcfg.py:101: ParseTestRecord = self.LoadParseTestRecord() Let's move this into the if-block where it's needed. https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py#... test/test262/testcfg.py:111: for f in test_record["includes"]] nit: please adjust indentation here
Thanks for the comments. Also: - renamed _PYTHON_PATH to _TOOLS_PATH per earlier comment. - changed 'Error' to 'ImportError' in raise -- 'Error' does not exist, the generic form is 'Exception' and 'ImportError' seemed more useful. https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py#... test/test262/testcfg.py:97: raise Error("Cannot load parseTestRecord; you may need to --download-data for test262") On 2014/08/19 11:32:55, Jakob wrote: > The only way this can happen is when there was an exception when trying to > import the module, right? So you can move it to the right place. And please > respect the 80 column limit. Don't be afraid of line breaks! They're really easy > in Python: > > try: > ... > except: > raise Error("Cannot load parseTestRecord; you may need to " > "--download-data for test262") > finally: > ... Done. https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py#... test/test262/testcfg.py:101: ParseTestRecord = self.LoadParseTestRecord() On 2014/08/19 11:32:55, Jakob wrote: > Let's move this into the if-block where it's needed. Done. https://codereview.chromium.org/478163002/diff/80001/test/test262/testcfg.py#... test/test262/testcfg.py:111: for f in test_record["includes"]] On 2014/08/19 11:32:55, Jakob wrote: > nit: please adjust indentation here Done.
Message was sent while issue was closed.
Committed patchset #6 manually as 23313 (presubmit successful).
Message was sent while issue was closed.
On 2014/08/22 13:00:17, rossberg wrote: > Committed patchset #6 manually as 23313 (presubmit successful). Thanks a lot for doing this, I just landed the patch. However, I decided to move it to a separate test/test262-es6 directory for the time being. This way, we can still track ES5 conformance until ES6 actually is the official standard. Michael A will set up new test262-es6 runners for our waterfall. I also adjusted the README to mention the same revision as the cfg file.
Message was sent while issue was closed.
On 2014/08/22 13:13:24, rossberg wrote: > On 2014/08/22 13:00:17, rossberg wrote: > > Committed patchset #6 manually as 23313 (presubmit successful). > > Thanks a lot for doing this, I just landed the patch. However, I decided to move > it to a separate test/test262-es6 directory for the time being. This way, we can > still track ES5 conformance until ES6 actually is the official standard. Michael > A will set up new test262-es6 runners for our waterfall. > > I also adjusted the README to mention the same revision as the cfg file. CC'ing relevant V8 folks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
