Skip to content
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.

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

tests: better combine_logs.py behavior #14683

Merged
merged 1 commit into from Nov 30, 2018

Conversation

@jamesob
Copy link
Member

@jamesob jamesob commented Nov 7, 2018

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 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 lucash-dev commented Nov 9, 2018

ACK. very useful!

@laanwj
Copy link
Member

@laanwj 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?

Copy link
Contributor

@ryanofsky ryanofsky left a comment

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

test/functional/combine_logs.py Outdated Show resolved Hide resolved
Copy link
Member

@jnewbery jnewbery left a comment

Big concept ACK. Very cool.

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

test/functional/combine_logs.py Outdated Show resolved Hide resolved
test/functional/combine_logs.py Outdated Show resolved Hide resolved
test/functional/combine_logs.py Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Nov 22, 2018

Concept ACK

Nice developer ergonomics improvement!

@conscott
Copy link
Contributor

@conscott conscott commented Nov 22, 2018

Concept ACK woot!

test/functional/combine_logs.py Outdated Show resolved Hide resolved
@jamesob jamesob force-pushed the 2018-11-better-cons-log branch from e2d9b2f to ca9698e Nov 27, 2018
@jamesob
Copy link
Member Author

@jamesob 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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Nov 27, 2018

utACK ca9698e9075fb1b85afc01e890cef97e13d25405

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Nov 27, 2018
Copy link
Member

@jnewbery jnewbery left a comment

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

test/functional/combine_logs.py Outdated Show resolved Hide resolved
@jamesob jamesob force-pushed the 2018-11-better-cons-log branch from ca9698e to 4aabadb Nov 29, 2018
@jnewbery
Copy link
Member

@jnewbery jnewbery commented Nov 29, 2018

ACK 4aabadb. Thanks for removing the dependency!

pull bot pushed a commit to ken2812221/bitcoin that referenced this issue Nov 30, 2018
4aabadb tests: have combine_logs default to most recent test dir (James O'Beirne)

Pull request description:

  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
  ```sh
  alias testlogs='./test/functional/combine_logs.py -c | less'

  ./test/functional/some_test.py  # fails
  testlogs
  ```

Tree-SHA512: 919642ab09c314888a23c9491963b35b9da87e60deb740d1d5e816444aa9bdda5e519dc8ca131669f2d563167ef5f5abb14e22f20f47bf8362915ed578181846
@MarcoFalke MarcoFalke merged commit 4aabadb into bitcoin:master Nov 30, 2018
2 checks passed
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Nov 30, 2018

re-utACK 4aabadb

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Oct 7, 2020
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.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants