Skip to content

[tests] Functional test warnings#10197

Merged
maflcko merged 2 commits intobitcoin:masterfrom
jnewbery:functional_test_warnings
Apr 17, 2017
Merged

[tests] Functional test warnings#10197
maflcko merged 2 commits intobitcoin:masterfrom
jnewbery:functional_test_warnings

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Apr 12, 2017

Two changes:

  1. Issue warnings when running test_runner:
  • if there are python files in the directory that are not listed in test_runner (this is downgraded from an error)
  • if there is already a bitcoind process running on the system (uses pidof - only works for linux)
  • if there is a cache directory
  1. delete cache directory every time test_runner is started. Adds a parameter --keepcache to override the default behaviour and keep the cache directory from the previous run.

Rebuilding the cache takes about 21 seconds on my pc. This will be reduced to just under 2 seconds once #10082 has been fully merged.

@jnewbery jnewbery changed the title Functional test warnings [tests] Functional test warnings Apr 12, 2017
@fanquake fanquake added the Tests label Apr 12, 2017
@jnewbery jnewbery force-pushed the functional_test_warnings branch from d85ae2a to 3fd09cb Compare April 13, 2017 12:31
maflcko
maflcko reviewed Apr 14, 2017
View reviewed changes
test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Apr 14, 2017

Choose a reason for hiding this comment

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

I'd prefer to keep this an error, so that a failing travis will quickly tell the author of a new script that it is not being run.

Copy link
Member

@maflcko maflcko Apr 17, 2017

Choose a reason for hiding this comment

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

Maybe make this warning an error on travis?

if os.getenv('TRAVIS')=='true':
  # On travis this warning is an error to prevent merging incomplete commits into master
  sys.exit(1)

@maflcko
Copy link
Member

maflcko commented Apr 14, 2017

utACK 3fd09cb beside the comment

@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 17, 2017

@MarcoFalke I've found the error-and-exit behaviour annoying and intrusive since we introduced it. I'm often working on test cases outside a git repository and either want to add a new test case or modify an existing test case and keep a backup in the same directory (my exact setup is that I'm using rsync to copy my git repo into a directory on a VM, and old files don't automatically get deleted when I checkout a new branch and rsync it to the VM). The fact that test_runner fails to run when it detects a new file in the directory is annoying.

Remember that the point of this check is so new test cases don't get added to the repo without being added to test_runner. Only one person needs to notice that the warning is being raised either locally or on Travis for it to have done its job. For that, a warning should be enough.

@maflcko
Copy link
Member

maflcko commented Apr 17, 2017

@jnewbery Sounds reasonable.

maflcko
maflcko reviewed Apr 17, 2017
View reviewed changes
test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Apr 17, 2017

Choose a reason for hiding this comment

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

except (OSError, subprocess.SubprocessError):

should be enough?

test/functional/test_runner.py Outdated
Copy link
Member

@maflcko maflcko Apr 17, 2017

Choose a reason for hiding this comment

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

Maybe make this warning an error on travis?

if os.getenv('TRAVIS')=='true':
  # On travis this warning is an error to prevent merging incomplete commits into master
  sys.exit(1)

@jnewbery jnewbery force-pushed the functional_test_warnings branch from 3fd09cb to 08e51c1 Compare April 17, 2017 14:32
@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 17, 2017

Addressed @MarcoFalke's nits

@maflcko maflcko merged commit 08e51c1 into bitcoin:master Apr 17, 2017
maflcko pushed a commit that referenced this pull request Apr 17, 2017
Merge #10197: [tests] Functional test warnings
d86bb07 
08e51c1 [tests] Remove cache directory by default when running test_runner (John Newbery)
c85b080 [test] add warnings to test_runner (John Newbery)

Tree-SHA512: 537a8a258e410102708d1e02893f3f45abe7a3a3290536249381a7dc55d74ca78322804bf34178dec1461ec1c29d8f8358c5901ddd1633f8b301b95bcbb6ce6d
@jnewbery jnewbery deleted the functional_test_warnings branch April 17, 2017 20:25
@laanwj laanwj mentioned this pull request Aug 24, 2017
Closed
@Sjors
Copy link
Member

Sjors commented Jan 2, 2018

If a MacOS user installs pidof via Homebrew, subprocess.check_output(["pidof", "bitcoind"]) will return b'' which isn't None. This then causes the "There is already a bitcoind process running on this system" to always appear. Not sure if that's worth fixing.

@jnewbery
Copy link
Contributor Author

jnewbery commented Jan 2, 2018

Not sure if that's worth fixing.

Certainly! PRs welcome 🙂

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 14, 2019
@PastaPastaPasta
Merge bitcoin#10197: [tests] Functional test warnings
b76b91d 
08e51c1 [tests] Remove cache directory by default when running test_runner (John Newbery)
c85b080 [test] add warnings to test_runner (John Newbery)

Tree-SHA512: 537a8a258e410102708d1e02893f3f45abe7a3a3290536249381a7dc55d74ca78322804bf34178dec1461ec1c29d8f8358c5901ddd1633f8b301b95bcbb6ce6d
@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

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jnewbery @maflcko @Sjors @fanquake