test: add test for specifying custom pidfile via -pid #30724

1. 
   theStack commented at 12:44 pm on August 27, 2024: contributor

   This small PR adds test coverage for the -pid command line option, which allows to overrule the pid filename (bitcoind.pid by default). One can specify either a relative path (within the datadir) or an absolute one; the latter is tested using self.options.tmpdir. Note that the functional test file feature_init.py so far only contained a stress test; with this new sub-test added, both the description and the test name are adapted to be more generic.
2. test: add test for specifying custom pidfile via `-pid` db3edefce4
3. 
   DrahtBot commented at 12:45 pm on August 27, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	tdb3, naiyoma

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

4. DrahtBot added the label Tests on Aug 27, 2024
5. in test/functional/feature_init.py:16 in db3edefce4

   11@@ -12,7 +12,11 @@
   12 from test_framework.util import assert_equal
   13 
   14 
   15-class InitStressTest(BitcoinTestFramework):
   16+BITCOIN_PID_FILENAME_DEFAULT = "bitcoind.pid"
   17+BITCOIN_PID_FILENAME_CUSTOM = "my_fancy_bitcoin_pid_file.foobar"
   

   ---

   ---

   

   furszy commented at 1:35 pm on August 27, 2024:

   nit: why not declaring this custom param within the test function?
6. 
   tdb3 commented at 12:07 pm on August 28, 2024: contributor

   Concept ACK Good addition. CR and ran tests locally.

   "bitcoind.pid" is also mentioned in feature_filelock.py. Could add a commit that deduplicates this.

   Created an example commit here: https://github.com/tdb3/bitcoin/commit/b622390b33cf3813b4f80d13377b248d6e898a4f

   Thought about maybe making something like a

   0[@property](/bitcoin-bitcoin/contributor/property/)
   1def pid_file_path(self) -> Path:
   2    return self.chain_path / "bitcoind.pid"
   

   but with bitcoind -pid=someotherfile being an option, might be confusing/unexpected to have pid_file_path mismatch the pid file actually used by the node.

7. 
   naiyoma commented at 7:08 pm on September 7, 2024: contributor

   Concept ACK +1 on the suggestion above to move BITCOIN_PID_FILENAME_DEFAULT and import it in both files.

   This is out of scope for this PR, so feel free to ignore. it would be good to add an edge case to test starting a node when there’s already a pre-existing PID file.

Contributors
theStack DrahtBot furszy tdb3 naiyoma

Labels
Tests