test: add missing stop_node calls to feature_coinstatsindex and feature_prune #25034

1. 
   mzumsande commented at 7:48 pm on April 29, 2022: member

   In #24789, I forgot to stop the node before using assert_start_raises_init_error in feature_coinstatsindex. This resulted in a bitcoind process that is not being terminated after the test finishes. feature_prune has the same problem and also creates a zombie bitcoind process.

   Also adds an assert to assert_start_raises_init_error to make sure the node isn’t already running to prevent this sort of mistake in the future.

2. mzumsande force-pushed on Apr 29, 2022
3. DrahtBot added the label Tests on Apr 29, 2022
4. test: stop node before calling assert_start_raises_init_error

   ...in feature_coinstatsindex and feature_pruning.
   Also add an assert to assert_start_raises_init_error that the node is
   not already running.

   a3cd7dbfd8
5. mzumsande force-pushed on Apr 29, 2022
6. mzumsande renamed this:
   test: add missing stop_node call to feature_coinstatsindex
   test: add missing stop_node calls to feature_coinstatsindex and feature_prune
   on Apr 29, 2022
7. 
   mzumsande commented at 9:06 pm on April 29, 2022: member

   An alternative fix would be to have assert_start_raises_init_error stop the node before starting it again (instead of asserting).
8. 
   brunoerg commented at 9:46 pm on April 29, 2022: member

   I noticed there are other tests which don’t stop the node before assert_start_raises_init_error, should we update all of them?
9. 
   mzumsande commented at 10:17 pm on April 29, 2022: member

   I noticed there are other tests which don’t stop the node before assert_start_raises_init_error, should we update all of them?

   Which ones? Since I added the assert to assert_start_raises_init_error, I would expect these tests to assert now. I’m not sure - on the one hand it might be easier to let assert_start_raises_init_error just stop the node, on the other hand I think it would be clearer if it’s visible in the actual test when a node is stopped.

   Oh, and thanks @willcl-ark I for finding this bug!

10. 
    brunoerg commented at 10:44 pm on April 29, 2022: member

    Which ones? Sorry, my bad! With these changes, I think all the tests are right. I thought it was missing intest_invalid_command_line_options (feature_config_args) but in the end of the previous function it stops the node.

11. in test/functional/feature_coinstatsindex.py:240 in a3cd7dbfd8

    235         self.nodes[1].assert_start_raises_init_error(
    236             expected_msg='Error: -reindex-chainstate option is not compatible with -coinstatsindex. '
    237             'Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.',
    238             extra_args=['-coinstatsindex', '-reindex-chainstate'],
    239         )
    240+        self.restart_node(1, extra_args=["-coinstatsindex"])
    

    ---

    ---

    

    MarcoFalke commented at 6:35 am on April 30, 2022:

    I think the default extra args are persisted in node, so no need to pass them here again.
12. MarcoFalke approved
13. 
    MarcoFalke commented at 7:25 am on April 30, 2022: member

    LGTM
14. MarcoFalke merged this on Apr 30, 2022
15. MarcoFalke closed this on Apr 30, 2022

    ---

16. sidhujag referenced this in commit eacf5a4216 on Apr 30, 2022
17. MarcoFalke deleted a comment on May 2, 2022
18. mzumsande deleted the branch on May 19, 2022
19. DrahtBot locked this on May 19, 2023

Contributors
mzumsande brunoerg MarcoFalke

Labels
Tests