Have combine_logs.py default to the most recent test directory if no argument is provided. This allows you to avoid an annoying copy-paste when iterating on a failing test, since you can do something like
alias testlogs='./test/functional/combine_logs.py -c | less'
./test/functional/some_test.py # fails
testlogs
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
ACK. very useful!
74 | + testdir_paths = [ 75 | + join_tmp(name) for name in os.listdir(tmpdir) if 76 | + os.path.isdir(join_tmp(name)) and is_test_dir_name(name) 77 | + ] 78 | + 79 | + testdir_paths.sort(key=lambda x: os.path.getmtime(x), reverse=True)
Isn't the lambda redundant here? Could use os.path.getmtime directly?
Doesn't hugely matter, but there's no need to build and sort a list if you just want the newest file, e.g.:
newest = max((os.path.getmtime(f), f) for f in os.listdir("."))[1]
Concept ACK—I think this functionality is very useful
Though I'm not sure about security implications of opening everything that looks like a test directory in /tmp, say, on a multi-user system.
Maybe add a check that the directory is owned by the current user as well?
utACK e2d9b2fec809971de4cfc67048995e33e7e4749c, though I agree it would be good to check ownership.
21 | @@ -19,22 +22,29 @@ 22 | 23 | def main(): 24 | """Main function. Parses args, reads the log files and renders them as text or html.""" 25 | - 26 | - parser = argparse.ArgumentParser(usage='%(prog)s [options] <test temporary directory>', description=__doc__) 27 | + parser = argparse.ArgumentParser(description=__doc__)
nit: can you add formatter_class=argparse.RawTextHelpFormatter here. Otherwise the newlines from the docstring get squashed in the help text.
30 | + help=('temporary test directory to combine logs from. ' 31 | + 'Defaults to the most recent')) 32 | parser.add_argument('-c', '--color', dest='color', action='store_true', help='outputs the combined log with events colored by source (requires posix terminal colors. Use less -r for viewing)') 33 | parser.add_argument('--html', dest='html', action='store_true', help='outputs the combined log as html. Requires jinja2. pip install jinja2') 34 | - args, unknown_args = parser.parse_known_args() 35 | + args, _ = parser.parse_known_args()
I think you can just change this to parser.parse_args() and drop the unused variable.
47 | + print("No test directories found") 48 | sys.exit(1) 49 | 50 | - log_events = read_logs(unknown_args[0]) 51 | + if not args.testdir: 52 | + print("Opening latest test directory: {}".format(testdir))
Can you drop this print, or print to stderr.
(printing this is problematic when using the --html output option for example)
Big concept ACK. Very cool.
A few nits inline. You should also address comments by laanwj, ryanofsky and practicalswift.
Concept ACK
Nice developer ergonomics improvement!
Concept ACK woot!
81 | + return testdir_paths[0] if testdir_paths else None 82 | + 83 | + 84 | +def is_test_dir_name(path): 85 | + """Returns True if the filename indicates a test directory.""" 86 | + return re.match(r'test\S{8}$', os.path.basename(path))
Instead of matching just on "test*", which is used by other programs (such as our unit test runner, IIRC), couldn't you instead explicitly set a unique (or approximately unique) prefix for the temp folder name in the test framework?
maybe match the silly unicode characters as well, they're very unlikely to arise anywhere else :)
Oh, the unicode characters are only present when run through the test runner, in which case the tests are run in parallel, so this feature wouldn't turn out useful.
If the tests are run one-each, they use a different prefix, so I suggest to change that prefix.
I've updated the test framework to share a TMPDIR_PREFIX constant which has a value of "bitcoin_func_test_".
Thanks for the good feedback, all. I've incorporated everyone's suggestions - though I'm testing for tmp directory readability instead of ownership.
utACK ca9698e9075fb1b85afc01e890cef97e13d25405. Just minor tweaks since last review: changing test prefix, choosing last readble directory, changing help formatting and forbidding unknown options
utACK ca9698e9075fb1b85afc01e890cef97e13d25405
12 | @@ -11,6 +13,9 @@ 13 | import os 14 | import re 15 | import sys 16 | +import tempfile 17 | + 18 | +from test_framework.test_framework import TMPDIR_PREFIX
Can we remove this dependency? I have a combine_logs.py installed in a separate bin directory and it currently works as a standalone util. Adding this dependency breaks that.
Perhaps just directly define TMPDIR_PREFIX as "bitcoin_func_test_" here. If you want to go wild, you could add a unit test that checks that TMPDIR_PREFIX is defined the same in both files.
Perhaps also add a comment to warn people against importing from test_framework in here (or I can do it in a separate PR).
Fixed
Changes mostly look good, but please remove the dependency on test_framework
ACK 4aabadbf44ad8f14cfa3f8921caf3135515b9ecf. Thanks for removing the dependency!
re-utACK 4aabadb
Milestone
0.18.0