[test] Key variant flags by variant name everywhere.

tools/testrunner/local/testsuite.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 17 matching lines @@
 
 import imp
 import os
 
 from . import commands
 from . import statusfile
 from . import utils
 from ..objects import testcase
 
 # Use this to run several variants of the tests.
-VARIANT_FLAGS = {
-    "default": [],
-    "stress": ["--stress-opt", "--always-opt"],
-    "turbofan": ["--turbo", "--always-opt"],
-    "nocrankshaft": ["--nocrankshaft"]}
+ALL_VARIANTS = [
+  ("default", []),
+  ("stress", ["--stress-opt", "--always-opt"]),
+  ("turbofan", ["--turbo", "--always-opt"]),
+  ("nocrankshaft", ["--nocrankshaft"]),
+]
 
-FAST_VARIANT_FLAGS = [
-    f for v, f in VARIANT_FLAGS.iteritems() if v in ["default", "turbofan"]
+# FAST_VARIANTS implies no --always-opt.
+FAST_VARIANTS = [
+  ("default", []),
+  ("turbofan", ["--turbo"]),
 ]
 
+DEFAULT_VARIANT = [
+  ("default", []),
+]
+
+def FilterVariants(variants, variant_filter):

[Jakob Kummerow, 2015/07/27 14:36:53]

nit: VariantsFilterBuilder would be a more appropriate name, no?

+  return filter(lambda (v, f): v in variant_filter, variants)
+
+class VariantFlagsBuilder(object):
+  def __init__(self, suite, variant_filter):
+    self.suite = suite
+    self.all_variants = FilterVariants(ALL_VARIANTS, variant_filter)
+    self.fast_variants = FilterVariants(FAST_VARIANTS, variant_filter)
+    self.default_variant = FilterVariants(DEFAULT_VARIANT, variant_filter)
+
+  def DefaultVariant(self, testcase):

[Jakob Kummerow, 2015/07/27 14:36:53]

nit: this method and its friends below are private to this class, right? Please
let their names begin with an underscore. (Don't forget to update the overrides
too.)

+    """The variants to be run if the test case is marked with NO_VARIANTS."""
+    return self.default_variant
+
+  def FastVariants(self, testcase):
+    """The variants to be run if the test case is marked with FAST_VARIANTS."""
+    return self.fast_variants
+
+  def AllVariants(self, testcase):
+    """The variants to be run if the test case is not marked with a variants
+    modifier.
+    """
+    return self.all_variants
+
+  def __call__(self, testcase):

[Jakob Kummerow, 2015/07/27 14:36:53]

nit: instead of messing with Python object internals, can you just call this
function Build(self, testcase)?

+    """Calculate the testing variants and flags for a test case of the
+    corresponding suite.
+
+    Returns: An iterable over variant tuples (name, list of flags).

[Jakob Kummerow, 2015/07/27 14:36:53]

nit: IIUC this comment is a lie. It returns a callable that returns an iterable.
Which kind of proves that this design is overly complex.

+    """
+    if testcase.outcomes and statusfile.OnlyStandardVariant(
+        testcase.outcomes):
+      return self.DefaultVariant(testcase)
+    if testcase.outcomes and statusfile.OnlyFastVariants(testcase.outcomes):
+      return self.FastVariants(testcase)
+    return self.AllVariants(testcase)
+
+
 class TestSuite(object):
 
   @staticmethod
   def LoadTestSuite(root):
     name = root.split(os.path.sep)[-1]
     f = None
     try:
       (f, pathname, description) = imp.find_module("testcfg", [root])
       module = imp.load_module("testcfg", f, pathname, description)
       return module.GetSuite(name, root)
@@ skipping 24 matching lines @@
   # Used in the status file and for stdout printing.
   def CommonTestName(self, testcase):
     if utils.IsWindows():
       return testcase.path.replace("\\", "/")
     else:
       return testcase.path
 
   def ListTests(self, context):
     raise NotImplementedError
 
-  def VariantFlags(self, testcase, default_flags):
-    if testcase.outcomes and statusfile.OnlyStandardVariant(testcase.outcomes):
-      return [[]]
-    if testcase.outcomes and statusfile.OnlyFastVariants(testcase.outcomes):
-      # FAST_VARIANTS implies no --always-opt.
-      return [ filter(lambda flag: flag != "--always-opt", f)
-               for f in filter(lambda flags: flags in FAST_VARIANT_FLAGS,
-                               default_flags) ]
-    return default_flags
+  def VariantFlagsBuilder(self):

[Jakob Kummerow, 2015/07/27 14:36:53]

nit: I don't like clashing names of unrelated things. Please either rename this
to GetVariantFlagsBuilder, or the class above to DefaultVariantFlagsBuilder, or
similar.

+    """The variant builder class to be used."""
+    return VariantFlagsBuilder
+
+  def VariantFlags(self, variant_filter):

[Jakob Kummerow, 2015/07/27 14:36:53]

nit: this doesn't return flags any more, but a function producing flags. Please
rename accordingly. Maybe VariantFlagsGenerator or VariantFlagsFactory or
something to that effect?

+    """Calculate the testing variants of this suite.
+
+    Args:
+      variant_filter: List of variant names to be run as specified by the test
+                      runner.
+    Returns: A callable that returns an iterable over variant tuples (name,
+             list of flags).
+    """
+    return self.VariantFlagsBuilder()(self, variant_filter)
 
   def DownloadData(self):
     pass
 
   def ReadStatusFile(self, variables):
     (self.rules, self.wildcards) = \
         statusfile.ReadStatusFile(self.status_file(), variables)
 
   def ReadTestCases(self, context):
     self.tests = self.ListTests(context)
@@ skipping 167 matching lines @@
         tests.append(test)
     tests.sort()
     return tests
 
   def GetFlagsForTestCase(self, testcase, context):
     return (testcase.flags + ["--gtest_filter=" + testcase.path] +
             ["--gtest_random_seed=%s" % context.random_seed] +
             ["--gtest_print_time=0"] +
             context.mode_flags)
 
-  def VariantFlags(self, testcase, default_flags):
-    return [[]]
+  def VariantFlags(self, variant_filter):
+    return lambda _: DEFAULT_VARIANT
 
   def shell(self):
     return self.name