QA: stop nodes after RPC tests, even with –nocleanup #6032

1. 
   dexX7 commented at 10:20 am on April 20, 2015: contributor

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

2. laanwj added the label Tests on Apr 20, 2015
3. 
   laanwj commented at 10:49 am on April 20, 2015: member

   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)
4. QA: stop nodes after RPC tests, even with --nocleanup

   `--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.

   2eadeb27ed
5. 
   dexX7 commented at 11:57 am on April 20, 2015: contributor

   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. :)

6. 
   laanwj commented at 7:48 pm on April 20, 2015: member

   You don’t have to explicitly know the ports, you can use bitcoin-cli -regtest -datadir=/tmp/<tempdir>/node0 to communicate with the instances.
7. QA: add --noshutdown option to prevent stopping nodes

   With `--noshutdown`, the nodes are not stopped explicitly. `--noshutdown` implies `--nocleanup`, to prevent removing datadirs, which are still in use.

   688da79e4a
8. in qa/rpc-tests/test_framework.py: in 7d1bf7c598 outdated

   132-            print("Cleaning up")
   133+        try:
   134             stop_nodes(self.nodes)
   135             wait_bitcoinds()
   136+        except:
   137+            pass
   

   ---

   ---

   

   laanwj commented at 11:38 am on April 23, 2015:

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

   ---

   

   dexX7 commented at 11:48 am on April 23, 2015:

   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. :)

   ---

   

   dexX7 commented at 12:27 pm on April 23, 2015:

   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.
9. dexX7 force-pushed on Apr 23, 2015
10. 
    laanwj commented at 3:33 pm on April 24, 2015: member

    utACK
11. 
    gavinandresen commented at 10:19 pm on April 26, 2015: contributor

    ACK
12. 
    sipa commented at 9:06 am on April 27, 2015: member

    Concept ACK. I didn’t even know –nocleanup existed.
13. laanwj merged this on Apr 29, 2015
14. laanwj closed this on Apr 29, 2015

    ---

15. laanwj referenced this in commit e08886d8a3 on Apr 29, 2015
16. 
    dgenr8 commented at 1:23 am on April 30, 2015: contributor

    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.
17. MarcoFalke locked this on Sep 8, 2021

Contributors
dexX7 laanwj gavinandresen sipa dgenr8

Labels
Tests