test/test262: update testcfg.py for new test262

test/test262/testcfg.py

 # Copyright 2012 the V8 project authors. All rights reserved.
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 #       notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 #       copyright notice, this list of conditions and the following
 #       disclaimer in the documentation and/or other materials provided
@@ skipping 18 matching lines @@
 import hashlib
 import os
 import shutil
 import sys
 import tarfile
 
 from testrunner.local import testsuite
 from testrunner.local import utils
 from testrunner.objects import testcase
 
+TEST_262_ARCHIVE_REVISION = "595a36b"  # This is the 2014-Aug-10 revision.
+TEST_262_ARCHIVE_MD5 = "89dfc8458f474cdb7e8e178f3215dd06"
+TEST_262_URL = "https://github.com/tc39/test262/tarball/%s"
+TEST_262_HARNESS = ["sta.js"]
 
-TEST_262_ARCHIVE_REVISION = "fbba29f"  # This is the r365 revision.
-TEST_262_ARCHIVE_MD5 = "e1ff0db438cc12de8fb6da80621b4ef6"
-TEST_262_URL = "https://github.com/tc39/test262/tarball/%s"
-TEST_262_HARNESS = ["sta.js", "testBuiltInObject.js", "testIntl.js"]
+TEST_262_ROOT = ["data", "test"]
+TEST_262_SUITE_ROOT = TEST_262_ROOT + ["suite"]
+TEST_262_HARNESS_ROOT = TEST_262_ROOT + ["harness"]
+TEST_262_PYTHON_ROOT = ["data", "tools", "packaging"]
 
+# import parseTestRecord

[Jakob Kummerow, 2014/08/18 11:55:47]

Nit: comments should have proper capitalization and punctuation such as a
trailing full stop. (Again several times below.)

[smikes, 2014/08/18 16:04:26]

Removed needless comment.

+sys.path.append(os.path.abspath(os.path.join('test', 'test262', *TEST_262_PYTHON_ROOT)))

[Jakob Kummerow, 2014/08/18 11:55:47]

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

[smikes, 2014/08/18 16:04:27]

Fixed both.  Is there a more idiomatic way of importing a file
from a directory not in sys.path?

[Jakob Kummerow, 2014/08/18 16:14:10]

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?

+try:
+  from parseTestRecord import parseTestRecord

[Jakob Kummerow, 2014/08/18 11:55:47]

nit: "... as ParseTestRecord" to adhere to CapitalizedFunctionNameStyle

[smikes, 2014/08/18 16:04:26]

Done.

+except ImportError:
+  print "Need to --download-data for test262"
 
 class Test262TestSuite(testsuite.TestSuite):
 
   def __init__(self, name, root):
     super(Test262TestSuite, self).__init__(name, root)
-    self.testroot = os.path.join(root, "data", "test", "suite")
-    self.harness = [os.path.join(self.root, "data", "test", "harness", f)
-                    for f in TEST_262_HARNESS]
-    self.harness += [os.path.join(self.root, "harness-adapt.js")]
+    # set up paths
+    self.testroot = os.path.join(self.root, *TEST_262_SUITE_ROOT)
+    self.harnesspath = os.path.join(self.root, *TEST_262_HARNESS_ROOT)
+
+    # basic test harness consists of TEST_262_HARNESS files
+    # and our harness adapter
+    self.harness = [os.path.join(self.harnesspath, f)
+                    for f in TEST_262_HARNESS] + [

[Jakob Kummerow, 2014/08/18 11:55:47]

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).

[smikes, 2014/08/18 16:04:26]

Done.

+                    os.path.join(self.root, "harness-adapt.js")]
 
   def CommonTestName(self, testcase):
     return testcase.path.split(os.path.sep)[-1]
 
   def ListTests(self, context):
     tests = []
     for dirname, dirs, files in os.walk(self.testroot):
       for dotted in [x for x in dirs if x.startswith(".")]:
         dirs.remove(dotted)
       if context.noi18n and "intl402" in dirs:
         dirs.remove("intl402")
       dirs.sort()
       files.sort()
       for filename in files:
         if filename.endswith(".js"):
           testname = os.path.join(dirname[len(self.testroot) + 1:],
                                   filename[:-3])
           case = testcase.TestCase(self, testname)
           tests.append(case)
     return tests
 
   def GetFlagsForTestCase(self, testcase, context):
     return (testcase.flags + context.mode_flags + self.harness +
+            self.GetIncludesForTest(testcase) +
             [os.path.join(self.testroot, testcase.path + ".js")])
 
+  def GetIncludesForTest(self, testcase):
+    try:

[Jakob Kummerow, 2014/08/18 11:55:47]

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 []

[smikes, 2014/08/18 16:04:27]

Done.

+      testRecord = parseTestRecord(self.GetSourceForTest(testcase), testcase.path)

[Jakob Kummerow, 2014/08/18 11:55:47]

nit: 80col

+      return [os.path.join(self.harnesspath, f)
+              for f in testRecord['includes']]
+    except KeyError:
+      return []
+
   def GetSourceForTest(self, testcase):
-    filename = os.path.join(self.testroot, testcase.path + ".js")
-    with open(filename) as f:
-      return f.read()
+    try:
+      return testcase.source

[Jakob Kummerow, 2014/08/18 11:55:47]

No need to cache this; the framework is careful not to call GetSourceForTest
repeatedly (as it assumes that this operation might be expensive).

[smikes, 2014/08/18 16:04:26]

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)

+    except AttributeError:
+      filename = os.path.join(self.testroot, testcase.path + ".js")
+      with open(filename) as f:
+        testcase.source = f.read()
+      return testcase.source
 
   def IsNegativeTest(self, testcase):
-    return "@negative" in self.GetSourceForTest(testcase)
+    testRecord = parseTestRecord(self.GetSourceForTest(testcase), testcase.path)

[Jakob Kummerow, 2014/08/18 11:55:47]

nit: s/testRecord/test_record/

[smikes, 2014/08/18 16:04:27]

Done.

+    try:

[Jakob Kummerow, 2014/08/18 11:55:47]

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.)

[smikes, 2014/08/18 16:04:27]

Done.

+      return testRecord['negative']
+    except KeyError:
+      return False
 
   def IsFailureOutput(self, output, testpath):
     if output.exit_code != 0:
       return True
     return "FAILED!" in output.stdout
 
   def DownloadData(self):
     revision = TEST_262_ARCHIVE_REVISION
     archive_url = TEST_262_URL % revision
     archive_name = os.path.join(self.root, "tc39-test262-%s.tar.gz" % revision)
@@ skipping 20 matching lines @@
         # Magic incantation to allow longer path names on Windows.
         archive.extractall(u"\\\\?\\%s" % self.root)
       else:
         archive.extractall(self.root)
       os.rename(os.path.join(self.root, "tc39-test262-%s" % revision),
                 directory_name)
 
 
 def GetSuite(name, root):
   return Test262TestSuite(name, root)