Skip to content

tests: better combine_logs.py behavior #14683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Nov 30, 2018
Merged

tests: better combine_logs.py behavior #14683

maflcko merged 1 commit into from
Nov 30, 2018

Conversation

@jamesob
Copy link
Contributor

@jamesob jamesob commented Nov 7, 2018 •
edited
Loading

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 7, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@fanquake fanquake added the Tests label Nov 7, 2018
@lucash-dev
Copy link
Contributor

lucash-dev commented Nov 9, 2018

ACK. very useful!

practicalswift
practicalswift reviewed Nov 9, 2018
View reviewed changes
@laanwj
Copy link
Member

laanwj commented Nov 12, 2018

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?

ryanofsky
ryanofsky approved these changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK e2d9b2fec809971de4cfc67048995e33e7e4749c, though I agree it would be good to check ownership.

jnewbery
jnewbery reviewed Nov 21, 2018
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big concept ACK. Very cool.

A few nits inline. You should also address comments by laanwj, ryanofsky and practicalswift.

@practicalswift
Copy link
Contributor

practicalswift commented Nov 22, 2018

Concept ACK

Nice developer ergonomics improvement!

@conscott
Copy link
Contributor

conscott commented Nov 22, 2018

Concept ACK woot!

maflcko
maflcko reviewed Nov 22, 2018
View reviewed changes
@jamesob jamesob force-pushed the 2018-11-better-cons-log branch from e2d9b2f to ca9698e Compare November 27, 2018 16:05
@jamesob
Copy link
Contributor Author

jamesob commented Nov 27, 2018

Thanks for the good feedback, all. I've incorporated everyone's suggestions - though I'm testing for tmp directory readability instead of ownership.

ryanofsky
ryanofsky approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK ca9698e9075fb1b85afc01e890cef97e13d25405. Just minor tweaks since last review: changing test prefix, choosing last readble directory, changing help formatting and forbidding unknown options

@maflcko
Copy link
Member

maflcko commented Nov 27, 2018

utACK ca9698e9075fb1b85afc01e890cef97e13d25405

@maflcko maflcko added this to the 0.18.0 milestone Nov 27, 2018
jnewbery
jnewbery reviewed Nov 29, 2018
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes mostly look good, but please remove the dependency on test_framework

@jamesob jamesob force-pushed the 2018-11-better-cons-log branch from ca9698e to 4aabadb Compare November 29, 2018 22:41
@jnewbery
Copy link
Contributor

jnewbery commented Nov 29, 2018

ACK 4aabadb. Thanks for removing the dependency!

@maflcko maflcko merged commit 4aabadb into bitcoin:master Nov 30, 2018
@maflcko
Copy link
Member

maflcko commented Nov 30, 2018

re-utACK 4aabadb

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 7, 2020
@jamesob @PiRK
tests: have combine_logs default to most recent test dir
c60eca0 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

+3 more reviewers

@jnewbery jnewbery jnewbery left review comments

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

10 participants

@jamesob @DrahtBot @lucash-dev @laanwj @practicalswift @conscott @maflcko @jnewbery @ryanofsky @fanquake