This feels like a *lot* of complexity. Dynamically selected callables that
return callables that return iterables? At the very least, I'd like some
more
descriptive naming, to make it easier to understand what's going on, see
comments below.
Wouldn't the following design simplify things:
- let each test suite provide a default list of named variants (just the
names,
not the corresponding flag lists)
- let the command-line args filter those default variants as needed
- let each test case, by means of the status file, further filter the
variants
for that test as needed
- once the list of variant names per test has been computed, have a
translation
step that translates them to a list of lists of flags. This translation
could
again be implemented per-testsuite to keep custom variant definitions local.
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py
File tools/testrunner/local/testsuite.py (right):
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py#newcode55
tools/testrunner/local/testsuite.py:55: def FilterVariants(variants,
variant_filter):
nit: VariantsFilterBuilder would be a more appropriate name, no?
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py#newcode65
tools/testrunner/local/testsuite.py:65: def DefaultVariant(self,
testcase):
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.)
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py#newcode79
tools/testrunner/local/testsuite.py:79: def __call__(self, testcase):
nit: instead of messing with Python object internals, can you just call
this function Build(self, testcase)?
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py#newcode83
tools/testrunner/local/testsuite.py:83: Returns: An iterable over
variant tuples (name, list of flags).
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.
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py#newcode137
tools/testrunner/local/testsuite.py:137: def VariantFlagsBuilder(self):
nit: I don't like clashing names of unrelated things. Please either
rename this to GetVariantFlagsBuilder, or the class above to
DefaultVariantFlagsBuilder, or similar.
https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/testsuite.py#newcode141
tools/testrunner/local/testsuite.py:141: def VariantFlags(self,
variant_filter):
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?
https://codereview.chromium.org/1245623005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
