[PATCH] [LIT] Add SuffixDispatchTest for use in libc++ and libc++abi tests.
Dan Albert
danalbert at google.com
Wed Nov 12 21:25:03 PST 2014
A bunch of nits, a few design improvements, and a couple questions, but I think this is definitely something that will save us time in the long run. Thanks!
================
Comment at: utils/lit/lit/formats/__init__.py:3
@@ -2,1 +2,3 @@
from lit.formats.base import TestFormat, FileBasedTest, OneCommandPerFileTest
+from lit.formats.suffixdispatchtest import SuffixDispatchTest,\
+ CompileAndExecuteTestRunner
----------------
I much prefer
from foo import (bar,
baz)
Trailing backslashes are gross.
================
Comment at: utils/lit/lit/formats/base.py:25
@@ -24,5 +24,3 @@
if not os.path.isdir(filepath):
- base,ext = os.path.splitext(filename)
- if ext in localConfig.suffixes:
- yield lit.Test.Test(testSuite, path_in_suite + (filename,),
- localConfig)
+ for ext in localConfig.suffixes:
+ if filepath.endswith(ext):
----------------
EricWF wrote:
> This allows suffixes to have multiple extensions ex: `.pass.cpp`
This looks odd to me, but phab isn't giving me any context...
There's an outer loop here somewhere (judging by the yield below). I assume it's looping over each test?
You then have `if filepath.endswith(ext)` within the for loop, making all but one iteration a no-op. Am I missing something here?
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:1
@@ +1,2 @@
+from __future__ import absolute_import
+import os
----------------
I've always loved that Python has an import for a time machine.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:4
@@ +3,3 @@
+import sys
+import errno
+import tempfile
----------------
Sort these.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:15
@@ +14,3 @@
+ SuffixDispatchTest dispatches tests based on their suffix to different
+ test runners. test runners must have function signature of
+ # __call__(self, test, lit_config).
----------------
't'.upper()
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:19
@@ +18,3 @@
+ def __init__(self, suffixes=[]):
+ super(FileBasedTest, self).__init__()
+ self.suffix_tests = list(suffixes)
----------------
The single worst thing about Python is its syntax for `super`.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:22
@@ +21,3 @@
+
+ def add_suffix_test(self, suffix, test_runner):
+ self.suffix_tests += [tuple((suffix, test_runner))]
----------------
Is there ever a case where these aren't known at initialization time of the object? I'd vote for just removing these two methods (especially delete).
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:84
@@ +83,3 @@
+ unsupported_features = [f for f in unsupported
+ if f in test.config.available_features
+ or f in test.config.target_triple
----------------
Alignment.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:95
@@ +94,3 @@
+ missing_required_xfail_features = [f for f in requires_xfail
+ if f not in test.config.available_features
+ and f not in test.config.target_triple]
----------------
Alignment.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:98
@@ +97,3 @@
+ unsupported_xfail_features = [f for f in unsupported_xfail
+ if f in test.config.available_features
+ or f in test.config.target_triple
----------------
Alignment.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:106
@@ +105,3 @@
+ test_runner = None
+ for suffix_test in self.suffix_tests:
+ if name.endswith(suffix_test[0]):
----------------
EricWF wrote:
> jroelofs wrote:
> > I think you can do this in python....
> >
> > for (suffix, test) in self.suffix_tests:
> > if name.endswith(suffix):
> > test_runner = test
> > break
> Cool! That's so much cleaner. Thanks.
You wanted a dict.
test_runners = {
".pass": PassRunner(),
".fail": FailRunner()
}
This block would become `test_runner = test_runners[suffix]`.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:113
@@ +112,3 @@
+
+ status,report,msg = test_runner(test, lit_config)
+ # If the test is REQUIRES-XFAIL on UNSUPPORTED-XFAIL translate the
----------------
Spaces after commas
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:129
@@ +128,3 @@
+ report += msg
+ return status,report
+
----------------
Space after comma
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:147
@@ +146,3 @@
+ try:
+ compile_cmd = self.cxx.compileLinkCmd(['-o', exec_path, source_path])
+ cmd = compile_cmd
----------------
Why do we have to pass `-o` for this? Isn't it pretty obvious we want the link command to generate an output?
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:149
@@ +148,3 @@
+ cmd = compile_cmd
+ out,err,exitCode,report = lit.util.executeCommandAndReport(cmd)
+ if exitCode != 0:
----------------
Spaces after commas
`exit_code` to fit with the rest of the file
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:151
@@ +150,3 @@
+ if exitCode != 0:
+ return lit.Test.FAIL,report,"Compilation failed unexpectedly!"
+
----------------
sac
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:159
@@ +158,3 @@
+ cmd.append(exec_path)
+ if lit_config.useValgrind:
+ cmd = lit_config.valgrindArgs + cmd
----------------
This looks like an implementation detail that leaked (though I appreciate the irony).
I'd like to see if we can factor this out some how... but I'm not coming up with anything right now.
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:163
@@ +162,3 @@
+ cwd=source_dir)
+ report = """Compiled With: %s\n""" % \
+ ' '.join(["'%s'" % a for a in compile_cmd]) + report
----------------
Why a docstring?
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:166
@@ +165,3 @@
+ if exitCode != 0:
+ return lit.Test.FAIL,report,"Compiled test failed unexpectedly!"
+ finally:
----------------
sac
================
Comment at: utils/lit/lit/formats/suffixdispatchtest.py:172
@@ +171,3 @@
+ pass
+ return lit.Test.PASS,report,""
+##
----------------
sac
================
Comment at: utils/lit/lit/util.py:9
@@ -8,1 +8,3 @@
import sys
+import pty
+import select
----------------
sort these
================
Comment at: utils/lit/lit/util.py:149
@@ +148,3 @@
+
+def _to_string(str_bytes):
+ if isinstance(str_bytes, str):
----------------
jroelofs wrote:
> Some comments on these two functions would be nice here to explain the "why" behind them. At the moment I don't understand why these are needed.
Yeah, I find it really difficult to believe that a python library needs its own `to_string`.
================
Comment at: utils/lit/lit/util.py:159
@@ -158,3 +172,1 @@
- def to_string(bytes):
- if isinstance(bytes, str):
----------------
Oh, I see it wasn't you that added it. Still confused that we needed it...
================
Comment at: utils/lit/lit/util.py:189
@@ -165,1 +188,3 @@
+
+def executeCommandPTY(command, cwd=None, env=None):
try:
----------------
Why do we need this? What's wrong with `subprocess.Popen(..., stdout=subprocess.PIPE, stderr=subprocess.PIPE)`?
================
Comment at: utils/lit/lit/util.py:194
@@ +193,3 @@
+ kwargs = {
+ 'stdin' :subprocess.PIPE,
+ 'stdout':out_pty[1],
----------------
sac (and not before)
================
Comment at: utils/lit/lit/util.py:206
@@ +205,3 @@
+ while True:
+ exitCode = p.poll()
+ if exitCode is not None:
----------------
`exit_code`
================
Comment at: utils/lit/lit/util.py:234
@@ +233,3 @@
+ report = ''
+ report += """Command: %s\n""" % \
+ ' '.join(["'%s'" % a for a in command])
----------------
Docstrings?
First line added is unconditional, no sense in `report = ''` before
================
Comment at: utils/lit/lit/util.py:240
@@ +239,3 @@
+ report += """Environment: %s\n""" % \
+ ' '.join(["'%s'='%s'" % (k,env[k]) for k in env])
+ report += """Exit Code: %d\n""" % exitCode
----------------
sac
================
Comment at: utils/lit/lit/util.py:247
@@ +246,3 @@
+ report += '\n'
+ return out,err,exitCode,report
+
----------------
sac
================
Comment at: utils/lit/lit/util.py:276
@@ +275,3 @@
+
+class Compiler(object):
+ def __init__(self, path, compile_flags=[], link_flags=[]):
----------------
`lit.util` doesn't seem like the right place for this (but I haven't had a look around to know what the right place would be).
================
Comment at: utils/lit/lit/util.py:279
@@ +278,3 @@
+ self.path = str(path)
+ self.compile_flags = list(compile_flags)
+ self.link_flags = list(link_flags)
----------------
The copy probably isn't necessary.
================
Comment at: utils/lit/lit/util.py:282
@@ +281,3 @@
+
+ def clone(self):
+ return Compiler(self.path, self.compile_flags, self.link_flags)
----------------
Isn't this implicit?
================
Comment at: utils/lit/lit/util.py:285
@@ +284,3 @@
+
+ def compileCmd(self, compile_flags=[]):
+ return [self.path, '-c'] + self.compile_flags + compile_flags
----------------
For the way this is used, I think `def compilerCmd(self, in, out)` would make more sense.
Same goes for the rest of these.
http://reviews.llvm.org/D6206
More information about the llvm-commits
mailing list