Skip to content

Conversation

@dexX7
Copy link
Contributor

@dexX7 dexX7 commented Apr 20, 2015

--nocleanup should provide a way to preserve test data, but should not have an impact on whether nodes are to be stopped after the test execution.

In particular, when currently running RPC tests with --nocleanup, then it may result in several active bitcoind, which are not stopped properly.

Shutdown errors are ignored, given that a failure likely relates to already terminated processes, caused during the actual testing.

Edit:
Further, with --noshutdown, the nodes are not stopped explicitly. --noshutdown implies --nocleanup, to prevent removing datadirs, which are still in use.

@laanwj laanwj added the Tests label Apr 20, 2015
@laanwj
Copy link
Member

laanwj commented Apr 20, 2015

I was also surprised by the bitcoinds still running when specifying --nocleanup, it is not very intuitive. Though I'm sure that there can be reasons to want the nodes to remain running, for example to investigate their state (which is more difficult postmortem)

`--nocleanup` should provide a way to preserve test data, but should not have an impact on whether nodes are to be stopped after the test execution.

In particular, when currently running RPC tests with `--nocleanup`, then it may result in several active `bitcoind` processes, which are not terminated properly.
@dexX7
Copy link
Contributor Author

dexX7 commented Apr 20, 2015

Though I'm sure that there can be reasons to want the nodes to remain running ... which is more difficult postmortem

There could be a --noshutdown option to explicitly prevent shutting down the nodes (which should imply --nocleanup to avoid removing datadirs that are still in use), but I think there would probably be not much use for it, due to the seemingly random selection of RPC ports.

Assuming there were such an option, additional reporting of the ports, or alternatively providing options to specify ports, or maybe even an option to run the QT client instead of bitcoind, would be helpful.

I'm not sure how other people use, or want to use, the tests, so it would be interesting to learn more about potential use cases. :)

@laanwj
Copy link
Member

laanwj commented Apr 20, 2015

You don't have to explicitly know the ports, you can use bitcoin-cli -regtest -datadir=/tmp/<tempdir>/node0 to communicate with the instances.

Copy link
Member

Choose a reason for hiding this comment

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

Please report the exception w/ stack trace here (traceback.print_exc()). Ignoring exceptions, especially in tests, can cause useful diagnostic information to et lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking: if the client crashed earlier, then stopping the nodes is going to fail, too, and this was the part I wanted to hide. I'm going to take a look at it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be wrong, but it looks like there doesn't seem to be an exception raised to begin with. I removed the catch completely in the updated version.

With `--noshutdown`, the nodes are not stopped explicitly. `--noshutdown` implies `--nocleanup`, to prevent removing datadirs, which are still in use.
@dexX7 dexX7 force-pushed the qa-rpc-no-cleanup-node-stop branch from 7d1bf7c to 688da79 Compare April 23, 2015 12:25
@laanwj
Copy link
Member

laanwj commented Apr 24, 2015

utACK

@gavinandresen
Copy link
Contributor

ACK

@sipa
Copy link
Member

sipa commented Apr 27, 2015

Concept ACK. I didn't even know --nocleanup existed.

@laanwj laanwj merged commit 688da79 into bitcoin:master Apr 29, 2015
laanwj added a commit that referenced this pull request Apr 29, 2015
688da79 QA: add --noshutdown option to prevent stopping nodes (dexX7)
2eadeb2 QA: stop nodes after RPC tests, even with --nocleanup (dexX7)
@dgenr8
Copy link
Contributor

dgenr8 commented Apr 30, 2015

After-the-fact ACK. --nocleanup is indispensable when something actually breaks, which is often when developing a test, but the zombie bitcoind's were a nuisance.

@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