[testrunner] fix leaky abstraction in TestSuite loading process

TestSuite has a static method LoadTestSuite that should properly configure the
TestSuite instance (i.e. loaded status files and tests), however the method
leaves some configuration logic to the caller.

The leaky abstraction causes the caller to do a bunch of loading operations (see
the removed methods in base_runner.py).

This CL isolates the TestSuite loading logic to the static method only.

This is a refactoring only change without any intended logical changes.

R=machenbach@chromium.org
CC=​​​yangguo@chromium.org,sergiyb@chromium.org

Bug: v8:8174
Change-Id: I105059c9c9e050f03bb584174e2bd7ceeae2b228
Reviewed-on: https://chromium-review.googlesource.com/c/1396417
Commit-Queue: Tamer Tas <tmrts@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Sergiy Belozorov <sergiyb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58744}

tools/testrunner/base_runner.py

@@ -260,11 +260,8 @@ class BaseTestRunner(object):
         raise
 
       args = self._parse_test_args(args)
-      suites = self._get_suites(args, options)
-      self._prepare_suites(suites, options)
-
+      suites = self._load_testsuites(args, options)
       self._setup_env()
-
       print(">>> Running tests for %s.%s" % (self.build_config.arch,
                                             self.mode_name))
       tests = [t for s in suites for t in s.tests]
@@ -596,10 +593,6 @@ class BaseTestRunner(object):
 
     return reduce(list.__add__, map(expand_test_group, args), [])
 
-  def _get_suites(self, args, options):
-    names = self._args_to_suite_names(args, options.test_root)
-    return self._load_suites(names, options)
-
   def _args_to_suite_names(self, args, test_root):
     # Use default tests if no test configuration was provided at the cmd line.
     all_names = set(utils.GetSuitePaths(test_root))
@@ -609,26 +602,27 @@ class BaseTestRunner(object):
   def _get_default_suite_names(self):
     return []
 
-  def _load_suites(self, names, options):
+  def _load_testsuites(self, args, options):
+    names = self._args_to_suite_names(args, options.test_root)
     test_config = self._create_test_config(options)
-    def load_suite(name):
+    variables = self._get_statusfile_variables(options)
+    suites = []
+    for name in names:
       if options.verbose:
         print '>>> Loading test suite: %s' % name
-      return testsuite.TestSuite.LoadTestSuite(
-          os.path.join(options.test_root, name),
-          test_config)
-    return map(load_suite, names)
+      suite = testsuite.TestSuite.Load(
+          os.path.join(options.test_root, name), test_config)
 
-  def _prepare_suites(self, suites, options):
-    self._load_status_files(suites, options)
-    for s in suites:
-      s.ReadTestCases()
+      if self._is_testsuite_supported(suite, options):
+        suite.load_tests_from_disk(variables)
+        suites.append(suite)
 
-  def _load_status_files(self, suites, options):
-    # simd_mips is true if SIMD is fully supported on MIPS
-    variables = self._get_statusfile_variables(options)
-    for s in suites:
-      s.ReadStatusFile(variables)
+    return suites
+
+  def _is_testsuite_supported(self, suite, options):
+    """A predicate that can be overridden to filter out unsupported TestSuite
+    instances (see NumFuzzer for usage)."""
+    return True
 
   def _get_statusfile_variables(self, options):
     simd_mips = (

tools/testrunner/local/fake_testsuite/fake_testsuite.status

@@ -0,0 +1,5 @@
+# Copyright 2019 the V8 project authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+[]

tools/testrunner/local/fake_testsuite/testcfg.py

@@ -9,7 +9,8 @@ from testrunner.local import testsuite
 
 
 class TestSuite(testsuite.TestSuite):
-  pass
+  def ListTests(self):
+    return []
 
 def GetSuite(*args, **kwargs):
   return TestSuite(*args, **kwargs)

tools/testrunner/local/testsuite.py

@@ -29,6 +29,7 @@
 import fnmatch
 import imp
 import os
+from contextlib import contextmanager
 
 from . import command
 from . import statusfile
@@ -77,19 +78,22 @@ class TestCombiner(object):
   def _combined_test_class(self):
     raise NotImplementedError()
 
+@contextmanager
+def _load_testsuite_module(name, root):
+  f = None
+  try:
+    (f, pathname, description) = imp.find_module("testcfg", [root])
+    yield imp.load_module(name + "_testcfg", f, pathname, description)
+  finally:
+    if f:
+      f.close()
 
 class TestSuite(object):
   @staticmethod
-  def LoadTestSuite(root, test_config):
+  def Load(root, test_config):
     name = root.split(os.path.sep)[-1]
-    f = None
-    try:
-      (f, pathname, description) = imp.find_module("testcfg", [root])
-      module = imp.load_module(name + "_testcfg", f, pathname, description)
+    with _load_testsuite_module(name, root) as module:
       return module.GetSuite(name, root, test_config)
-    finally:
-      if f:
-        f.close()
 
   def __init__(self, name, root, test_config):
     self.name = name  # string
@@ -104,6 +108,11 @@ class TestSuite(object):
   def ListTests(self):
     raise NotImplementedError
 
+  def load_tests_from_disk(self, statusfile_variables):
+    self.statusfile = statusfile.StatusFile(
+      self.status_file(), statusfile_variables)
+    self.tests = self.ListTests()
+
   def get_variants_gen(self, variants):
     return self._variants_gen_class()(variants)
 
@@ -125,12 +134,6 @@ class TestSuite(object):
     """
     return None
 
-  def ReadStatusFile(self, variables):
-    self.statusfile = statusfile.StatusFile(self.status_file(), variables)
-
-  def ReadTestCases(self):
-    self.tests = self.ListTests()
-
   def _create_test(self, path, **kwargs):
     test_class = self._test_class()
     return test_class(self, path, self._path_to_name(path), self.test_config,

tools/testrunner/local/testsuite_unittest.py

@@ -36,15 +36,22 @@ class TestSuiteTest(unittest.TestCase):
         verbose=False,
     )
 
-  def testLoadingTestSuite(self):
-    suite = TestSuite.LoadTestSuite(self.test_root, self.test_config)
+    self.suite = TestSuite.Load(self.test_root, self.test_config)
 
-    self.assertEquals(suite.name, "fake_testsuite")
-    self.assertEquals(suite.test_config, self.test_config)
+  def testLoadingTestSuites(self):
+    self.assertEquals(self.suite.name, "fake_testsuite")
+    self.assertEquals(self.suite.test_config, self.test_config)
 
-    # Verify that no tests are loaded yet.
-    self.assertIsNone(suite.tests)
-    self.assertIsNone(suite.statusfile)
+    # Verify that the components of the TestSuite aren't loaded yet.
+    self.assertIsNone(self.suite.tests)
+    self.assertIsNone(self.suite.statusfile)
+
+  def testLoadingTestsFromDisk(self):
+    self.suite.load_tests_from_disk(statusfile_variables={})
+
+    # Verify that the components of the TestSuite are loaded.
+    self.assertIsNotNone(self.suite.tests)
+    self.assertIsNotNone(self.suite.statusfile)
 
 
 if __name__ == '__main__':

tools/testrunner/num_fuzzer.py

@@ -173,11 +173,8 @@ class NumFuzzer(base_runner.BaseTestRunner):
     # Indicate if a SIGINT or SIGTERM happened.
     return sigproc.exit_code
 
-  def _load_suites(self, names, options):
-    suites = super(NumFuzzer, self)._load_suites(names, options)
-    if options.combine_tests:
-      suites = [s for s in suites if s.test_combiner_available()]
-    return suites
+  def _is_testsuite_supported(self, suite, options):
+    return not options.combine_tests or suite.test_combiner_available()
 
   def _create_combiner(self, rng, options):
     if not options.combine_tests: