test: replace feature_masternode_params.py with C++ unit coverage#7386
test: replace feature_masternode_params.py with C++ unit coverage#7386thepastaclaw wants to merge 2 commits into
Conversation
Cover the InitParameterInteraction behavior triggered by -masternodeblsprivkey directly: -disablewallet, -peerblockfilters and -blockfilterindex=basic are soft-set, while explicit user values win. This replaces the much slower node-spinning checks in feature_masternode_params.py, removed in a follow-up commit.
The functional test only verified -masternodeblsprivkey parameter interactions (auto -disablewallet, -peerblockfilters, -blockfilterindex, plus user override semantics). All of these are now covered directly in masternode_params_tests, which is faster and exercises the soft-set logic without spinning up nodes. Local timings (Apple M-series, debug build): - feature_masternode_params.py: real 5.70s - test_dash --run_test=masternode_params_tests: real 0.35s
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
WalkthroughThe functional test Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit b900880) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only PR replacing a slow functional test with a focused unit test for the -masternodeblsprivkey parameter interaction. Both agents converged on a single valid suggestion: the new Dash-specific test file should be registered in non-backported.txt for Dash cppcheck lint coverage. No correctness or scope issues.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:60-68: Register the new Dash-specific test file for Dash lint coverage
The new `src/test/masternode_params_tests.cpp` is Dash-specific (it exercises the `-masternodeblsprivkey` parameter interaction added in Dash's `InitParameterInteraction`, which has no upstream counterpart), but no pattern in `non-backported.txt` matches it. `test/lint/lint-cppcheck-dash.py` derives its cppcheck target list from this file via `git ls-files -- <patterns>`, so without an entry the new test source will be excluded from the Dash-specific cppcheck pass. Other Dash-specific test files in `src/test/` are explicitly registered (e.g. `evo*.cpp`, `llmq*.cpp`, `governance*.cpp`, `block_reward_reallocation_tests.cpp`). Add a matching entry alongside the existing `src/test/...` lines.
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT while preserving the verified review result.
1 participant
Issue being fixed or feature implemented
test/functional/feature_masternode_params.pyexercised the-masternodeblsprivkeyparameter interaction by spinning up two fullnodes and restarting them three times. Every assertion the test made is
really about argument-state after
InitParameterInteractionruns —which is plain C++ logic and is much cheaper to cover as a unit test.
What was done?
src/test/masternode_params_tests.cppwith three cases againstInitParameterInteraction:-masternodeblsprivkey→ defaults untouched-masternodeblsprivkey→-disablewallet,-peerblockfiltersand-blockfilterindex=basicare soft-set-masternodeblsprivkey+ explicit-disablewallet=0/-peerblockfilters=0/-blockfilterindex=0→ user values win(SoftSet semantics)
src/Makefile.test.include.test/functional/feature_masternode_params.pyand its entryin
test/functional/test_runner.py.The unit tests are strictly stronger than the functional test: the
functional test had to weaken its first masternode assertion to just
"node is running" because filter advertisement timing is asynchronous,
while the unit test asserts the actual contract directly on
ArgsManager.The downstream effect of
-peerblockfilters=1/-blockfilterindex=basic(advertising
NODE_COMPACT_FILTERS, instantiating the basic filterindex) is not Dash-specific and is already covered upstream by
blockfilter_index_testsand the rest of init validation, so droppingthe functional check there is not a coverage regression.
How Has This Been Tested?
Local timings on Apple M-series, debug build
(
--enable-debug --disable-hardening):/usr/bin/time -p test/functional/feature_masternode_params.py/usr/bin/time -p ./src/test/test_dash --run_test=masternode_params_testsNew suite output:
CI savings: every PR build runs the functional suite, so removing a
~6 s node-spinning test in favor of a 0.35 s unit test is a small but
consistent win, and removes one process-spawn/restart cycle from
parallel functional shards.
Breaking Changes
None. Test-only change.
Checklist: