[test] Key variant flags by variant name everywhere.

tools/testrunner/local/testsuite.py

Index: tools/testrunner/local/testsuite.py
diff --git a/tools/testrunner/local/testsuite.py b/tools/testrunner/local/testsuite.py
index 133c74eefba74c13c95b4043c5f05cd4022d1522..4bf7dcd9989c04dfd6102d580a59997873d0f0d0 100644
--- a/tools/testrunner/local/testsuite.py
+++ b/tools/testrunner/local/testsuite.py
@@ -35,16 +35,61 @@ 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"]}
-
-FAST_VARIANT_FLAGS = [
-    f for v, f in VARIANT_FLAGS.iteritems() if v in ["default", "turbofan"]
+ALL_VARIANTS = [
+  ("default", []),
+  ("stress", ["--stress-opt", "--always-opt"]),
+  ("turbofan", ["--turbo", "--always-opt"]),
+  ("nocrankshaft", ["--nocrankshaft"]),
 ]
 
+# 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
@@ -89,15 +134,20 @@ class TestSuite(object):
   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
@@ -285,8 +335,8 @@ class GoogleTestSuite(TestSuite):
             ["--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