Skip to content

Conversation

@NicolasDorier
Copy link
Contributor

@NicolasDorier NicolasDorier commented Feb 23, 2017

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

@NicolasDorier NicolasDorier force-pushed the assert_start_raises_init_error branch 2 times, most recently from c4ddc2d to 5737be0 Compare February 23, 2017 08:43
@laanwj laanwj added the Tests label Feb 23, 2017
@laanwj
Copy link
Member

laanwj commented Mar 3, 2017

Makes sense.
utACK 5737be0

@jonasschnelli
Copy link
Contributor

Thanks.
utACK 5737be099f83dfb367a342c36f7f0ae5ea31bc29.
This is also required for #9662.

Copy link
Contributor

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)

Copy link
Contributor

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!"

Copy link
Contributor

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.

Copy link
Contributor

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)

@NicolasDorier NicolasDorier force-pushed the assert_start_raises_init_error branch 2 times, most recently from 4839562 to 25df293 Compare March 5, 2017 16:48
@NicolasDorier
Copy link
Contributor Author

nits of @jnewbery addressed

@maflcko
Copy link
Member

maflcko commented Mar 5, 2017 via email

@NicolasDorier NicolasDorier force-pushed the assert_start_raises_init_error branch from 25df293 to 836cd17 Compare March 6, 2017 08:20
@NicolasDorier NicolasDorier force-pushed the assert_start_raises_init_error branch from 836cd17 to 025dec0 Compare March 6, 2017 08:21
@NicolasDorier
Copy link
Contributor Author

squashed @MarcoFalke

@laanwj laanwj merged commit 025dec0 into bitcoin:master Mar 6, 2017
laanwj added a commit that referenced this pull request Mar 6, 2017
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)
Copy link
Member

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.

Copy link
Contributor Author

@NicolasDorier NicolasDorier Mar 6, 2017

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.

Copy link
Member

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.

@maflcko
Copy link
Member

maflcko commented Mar 6, 2017

Post merge utACK 025dec0

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 1, 2019
025dec0 [qa] assert_start_raises_init_error (NicolasDorier)

Tree-SHA512: 0fe3ecbd47625b181aed92f15445ac26993e1a8b9843bbc1088c4adcea774e503b870912a18e13dca3f255c22a9964c1c0ca92c758907538143f316c5272ea4a
zkbot added a commit to zcash/zcash that referenced this pull request Mar 24, 2020
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.
zkbot added a commit to zcash/zcash that referenced this pull request Apr 3, 2020
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.
@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

Development

Successfully merging this pull request may close these issues.

5 participants