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.js#newcode85
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.js#newcode87
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#newcode49
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#newcode50
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#newcode52
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#newcode67
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#newcode96
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#newcode97
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#newcode105
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#newcode113
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#newcode114
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.)
https://codereview.chromium.org/478163002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
