-pid
#30724
-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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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"
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.
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.
"bitcoind.pid"is also mentioned infeature_filelock.py. Could add a commit that deduplicates this.Created an example commit here: tdb3@b622390
Thanks! I’ve cherry-picked your commit, but reordered it to appear before the test introduction commit and changed the commit subject accordingly (introduce constant, instead of deduplicating). Let me know if that’s okay for you. @naiyoma:
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.
Good idea. I decided to leave that for a follow-up, happy to review if someone opens a PR.
Thanks! I’ve cherry-picked your commit, but reordered it to appear before the test introduction commit and changed the commit subject accordingly (introduce constant, instead of deduplicating). Let me know if that’s okay for you.
Looks great, thanks.
