test: Remove –noshutdown flag, Tidy startup failures #31620

1. 
   maflcko commented at 10:21 am on January 8, 2025: member

   The --noshutdown flag is brittle, confusing, and redundant:

   • Someone wanting to inspect the state after a test failure will likely also want to debug the state on the python side, so the option is redundant with --pdbonfailure. If there was a use case to replicate --pdbonfailure without starting pdb, a dedicated flag could be added for that use case.
   • It is brittle to use the flag for a passing test, because it will disable checks in the test. For example, on shutdown LSan will perform a leak check, and the test framework will check that the node did not crash, and it will check that the node did not print errors to stderr.

   Fix all issues by removing it.

   Also, tidy up startup error messages to be less confusing as a result.

2. test: Treat leftover process as error

   Printing to stderr instead of stdout makes the test_runner.py fail on
   leftover processes. This is desired and fine, because a leftover process
   should only happen on a test failure anyway.

   fad441fba0
3. test: Remove --noshutdown flag fa0dc09b90
4. test: Avoid redundant stop and error spam on startup failure

   Trying to immediately shut down a node after a startup failure without
   waiting for the RPC to be fully up will in most cases just fail and lead
   to an RPC error.
   
   Also, it is confusing to sidestep the existing fallback to kill any
   leftover nodes on a test failure.
   
   So just rely on the fallback.

   fae3bf6b87
5. 
   DrahtBot commented at 10:21 am on January 8, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31620.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	pablomartin4btc

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30660 (qa: Shut down framework cleanly on RPC connection failure by hodlinator)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

6. DrahtBot renamed this:
   test: Remove --noshutdown flag, Tidy startup failures
   test: Remove --noshutdown flag, Tidy startup failures
   on Jan 8, 2025
7. DrahtBot added the label Tests on Jan 8, 2025
8. 
   maflcko commented at 10:40 am on January 8, 2025: member

   For testing the third commit different ways are possible. For example:

   0$ rm -rf ./releases
   1$ ./test/get_previous_releases.py -b
   2$ rm -rf ./releases/v28.0/
   3$ ./build/test/functional/wallet_migration.py
   

   Then, observing a shorter error message.

9. 
   pablomartin4btc commented at 8:22 pm on January 8, 2025: member

   cr ACK and tACK fae3bf6b870eb0f9cddd1adac82ba72890806ae3 (coming from #31462).

   Adding sys.stderr was necessary for the CI checks (1st commit - fad441fba07877ea78ed6020fde11828307273a6). Redundant error messages were eliminated (e.g. in the proposed PR’s test: “Error: no RPC connection” is shown only once instead of twice as in master).

   Just for reference, --noshutdown was added in #6032 and --pdbonfailure in #11023.

Contributors
maflcko DrahtBot pablomartin4btc

Labels
Tests