From 234f27b5ae876668fc0931376fe088b2551dcfe1 Mon Sep 17 00:00:00 2001 From: Tamer Tas Date: Fri, 11 Jan 2019 12:29:15 +0100 Subject: [PATCH] [testrunner] fix leaky abstraction in TestSuite loading process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Michael Achenbach Reviewed-by: Sergiy Belozorov Cr-Commit-Position: refs/heads/master@{#58744} --- tools/testrunner/base_runner.py | 40 ++++++++----------- .../fake_testsuite/fake_testsuite.status | 5 +++ .../local/fake_testsuite/testcfg.py | 3 +- tools/testrunner/local/testsuite.py | 31 +++++++------- tools/testrunner/local/testsuite_unittest.py | 21 ++++++---- tools/testrunner/num_fuzzer.py | 7 +--- 6 files changed, 57 insertions(+), 50 deletions(-) create mode 100644 tools/testrunner/local/fake_testsuite/fake_testsuite.status diff --git a/tools/testrunner/base_runner.py b/tools/testrunner/base_runner.py index df1a5c49ad..3907253d01 100644 --- a/tools/testrunner/base_runner.py +++ b/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 = ( diff --git a/tools/testrunner/local/fake_testsuite/fake_testsuite.status b/tools/testrunner/local/fake_testsuite/fake_testsuite.status new file mode 100644 index 0000000000..b5ebc84474 --- /dev/null +++ b/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. + +[] diff --git a/tools/testrunner/local/fake_testsuite/testcfg.py b/tools/testrunner/local/fake_testsuite/testcfg.py index 7639c34942..ec7c51e33d 100644 --- a/tools/testrunner/local/fake_testsuite/testcfg.py +++ b/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) diff --git a/tools/testrunner/local/testsuite.py b/tools/testrunner/local/testsuite.py index 3954cc7493..a6d741af00 100644 --- a/tools/testrunner/local/testsuite.py +++ b/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, diff --git a/tools/testrunner/local/testsuite_unittest.py b/tools/testrunner/local/testsuite_unittest.py index 05d611feae..8e393c7155 100755 --- a/tools/testrunner/local/testsuite_unittest.py +++ b/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__': diff --git a/tools/testrunner/num_fuzzer.py b/tools/testrunner/num_fuzzer.py index a313377363..bf5bf07472 100755 --- a/tools/testrunner/num_fuzzer.py +++ b/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: