tests: better combine_logs.py behavior#14683
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
ACK. very useful! |
|
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 Maybe add a check that the directory is owned by the current user as well? |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK e2d9b2fec809971de4cfc67048995e33e7e4749c, though I agree it would be good to check ownership.
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
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! |
jamesob
force-pushed
the
2018-11-better-cons-log
branch
from
e2d9b2f to
ca9698e
Compare
|
Thanks for the good feedback, all. I've incorporated everyone's suggestions - though I'm testing for tmp directory readability instead of ownership. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK ca9698e9075fb1b85afc01e890cef97e13d25405. Just minor tweaks since last review: changing test prefix, choosing last readble directory, changing help formatting and forbidding unknown options
|
utACK ca9698e9075fb1b85afc01e890cef97e13d25405 |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Changes mostly look good, but please remove the dependency on test_framework
jamesob
force-pushed
the
2018-11-better-cons-log
branch
from
ca9698e to
4aabadb
Compare
|
ACK 4aabadb. Thanks for removing the dependency! |
|
re-utACK 4aabadb |
Summary: Have combine_logs.py default to the most recent test directory if no argument is provided. Backport of Core [[bitcoin/bitcoin#14683 | PR14683]] Test Plan: ``` ninja ../test/functional/feature_bip68_sequence.py --configfile=./test/config.ini ../test/functional/combine_logs.py -c ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7795
10 participants
Have
combine_logs.pydefault 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