-
Notifications
You must be signed in to change notification settings - Fork 38.7k
[qa] assert_start_raises_init_error #9832
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
[qa] assert_start_raises_init_error #9832
Conversation
c4ddc2d to
5737be0
Compare
|
Makes sense. |
|
Thanks. |
qa/rpc-tests/test_framework/util.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this if/else construction is needed. subprocess.Popen's stderr argument defaults to None, so you can just call bitcoind_processes[i] = subprocess.Popen(args, stderr=stderr)
qa/rpc-tests/test_framework/util.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yuck! raising an exception in expectation of catching it in the very next line.
I think try/except/else is a much cleaner construction, as in:
try:
thing_we_expect_to_fail()
except ExceptionType as e:
do_some_asserts_on_the_exception_raised()
else:
# oops, we were expecting an exception
assert False, "we were expecting an exception!"
qa/rpc-tests/test_framework/util.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider opening the file using the with log_stderr as... form. That means the file will be closed automatically (even if an exception is raised on the way), and you won't have to close it yourself with the finally branch at the bottom.
qa/rpc-tests/test_framework/util.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit: assert is a statement, not a function. This should be:
assert 'bitcoind exited' in str(e)
4839562 to
25df293
Compare
|
nits of @jnewbery addressed |
|
Please squash.
|
25df293 to
836cd17
Compare
836cd17 to
025dec0
Compare
|
squashed @MarcoFalke |
025dec0 [qa] assert_start_raises_init_error (NicolasDorier) Tree-SHA512: 0fe3ecbd47625b181aed92f15445ac26993e1a8b9843bbc1088c4adcea774e503b870912a18e13dca3f255c22a9964c1c0ca92c758907538143f316c5272ea4a
| with tempfile.SpooledTemporaryFile(max_size=2**16) as log_stderr: | ||
| try: | ||
| node = start_node(i, dirname, extra_args, stderr=log_stderr) | ||
| stop_node(node, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: No need to stop the node here. This is done by the test framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does need to be stopped, or this function would not be side effect free.
EDIT: Ah no it does not, as if it passes here, the test is finished, which would close the node anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right this line is only executed in exceptional circumstances, in which case the whole frameworks shuts down.
|
Post merge utACK 025dec0 |
025dec0 [qa] assert_start_raises_init_error (NicolasDorier) Tree-SHA512: 0fe3ecbd47625b181aed92f15445ac26993e1a8b9843bbc1088c4adcea774e503b870912a18e13dca3f255c22a9964c1c0ca92c758907538143f316c5272ea4a
Backport RPC test harness PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6548 - bitcoin/bitcoin#6804 - Just the coverage backend, not the flag to enable it. - bitcoin/bitcoin#7744 - bitcoin/bitcoin#9832 - Excludes `wallet-hd.py` change (missing bitcoin/bitcoin#8309). Part of #2074.
Backport RPC test harness PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6548 - bitcoin/bitcoin#6804 - Just the coverage backend, not the flag to enable it for all RPC tests. - bitcoin/bitcoin#7744 - bitcoin/bitcoin#9832 - Excludes `wallet-hd.py` change (missing bitcoin/bitcoin#8309). Part of #2074.
assert_start_raises_init_error make it possible to test expected error condition when bitcoind is starting.
ping @MarcoFalke and @jonasschnelli mentioned the need on #9662 and #9728