test: add functional tests for -deprecatedrpc=permissive_bool#7246
test: add functional tests for -deprecatedrpc=permissive_bool#7246knst wants to merge 2 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
knst
force-pushed
the
test-permissive-bool
branch
from
079a5f2 to
4838f65
Compare
knst
force-pushed
the
test-permissive-bool
branch
from
4838f65 to
00d18f8
Compare
WalkthroughThe test file was updated to validate the Sequence Diagram(s)mermaid Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/rpc_deprecated.py (1)
16-27:⚠️ Potential issue | 🟡 MinorUpdate stale log message after adding a real deprecated RPC test.
Line 27 currently logs
"No tested deprecated RPC methods", but Line 16 now runstest_permissive_bool(). This makes test output misleading.Suggested fix
- self.log.info("No tested deprecated RPC methods") + self.log.info("Tested deprecated RPC parsing with -deprecatedrpc=permissive_bool")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_deprecated.py` around lines 16 - 27, The log message "No tested deprecated RPC methods" is now misleading because test_permissive_bool() is executed; update the log in run_test (the self.log.info call) to reflect the actual test being run (e.g., log that permissive boolean/deprecated-RPC behavior is being tested) or remove the message entirely; locate the self.log.info invocation near test_permissive_bool() and change its text to accurately describe the test (referencing test_permissive_bool or the deprecated-RPC check) so test output is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/functional/rpc_deprecated.py`:
- Around line 16-27: The log message "No tested deprecated RPC methods" is now
misleading because test_permissive_bool() is executed; update the log in
run_test (the self.log.info call) to reflect the actual test being run (e.g.,
log that permissive boolean/deprecated-RPC behavior is being tested) or remove
the message entirely; locate the self.log.info invocation near
test_permissive_bool() and change its text to accurately describe the test
(referencing test_permissive_bool or the deprecated-RPC check) so test output is
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 83abcfe8-d5e8-4e01-939f-2d4f1e435a5b
📒 Files selected for processing (1)
test/functional/rpc_deprecated.py
|
pls see eb8d292 |
Add full coverage for the permissive_bool legacy input surface:
- Test falsy values (False, 0, "0") in both strict and permissive modes
- Test string aliases ("yes", "no") in both modes
- Test invalid values (2, "notabool") are still rejected in permissive mode
- Add log.info calls for test sections
- Comment out stale "No tested deprecated RPC methods" message
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/rpc_deprecated.py (1)
36-51: Consider table-driven assertions to reduce duplication and future misses.Lines 36-51 repeat many near-identical assertions. A small loop-based structure would keep intent clearer and make it easier to extend token coverage safely.
♻️ Refactor sketch
- assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", 1) - assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "1") - assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", 0) - assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "0") - assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "yes") - assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "no") + strict_legacy_values = [1, "1", 0, "0", "yes", "no", "true", "false"] + strict_err = "detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing." + for value in strict_legacy_values: + assert_raises_rpc_error(-8, strict_err, self.nodes[0].protx, "list", "valid", value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_deprecated.py` around lines 36 - 51, Replace the repeated near-identical assertions with two small table-driven loops: define a list of test_values = [True, 1, "1", False, 0, "0", "yes", "no"] (or separate legacy_values for node0 that should raise), then for node0 iterate over the values and call assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", val) and for node1 iterate and call assert_equal([], self.nodes[1].protx("list", "valid", val)); this consolidates the repeated assertions around the existing helpers assert_raises_rpc_error, self.nodes[0].protx, self.nodes[1].protx and assert_equal into concise loops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/functional/rpc_deprecated.py`:
- Around line 43-55: Add assertions to cover legacy string booleans "true" and
"false" accepted by ParseBoolV in permissive mode: update the protx tests that
currently assert acceptance for 1/0, "1"/"0", "yes"/"no" by adding
assert_equal([], self.nodes[1].protx("list", "valid", "true")) and
assert_equal([], self.nodes[1].protx("list", "valid", "false")) so the
permissive-path for ParseBoolV and the protx("list","valid",...) calls are fully
exercised.
---
Nitpick comments:
In `@test/functional/rpc_deprecated.py`:
- Around line 36-51: Replace the repeated near-identical assertions with two
small table-driven loops: define a list of test_values = [True, 1, "1", False,
0, "0", "yes", "no"] (or separate legacy_values for node0 that should raise),
then for node0 iterate over the values and call assert_raises_rpc_error(-8,
'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow
legacy boolean parsing.', self.nodes[0].protx, "list", "valid", val) and for
node1 iterate and call assert_equal([], self.nodes[1].protx("list", "valid",
val)); this consolidates the repeated assertions around the existing helpers
assert_raises_rpc_error, self.nodes[0].protx, self.nodes[1].protx and
assert_equal into concise loops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bcc1dc2-4f34-4203-b0d8-847ce48ace31
📒 Files selected for processing (1)
test/functional/rpc_deprecated.py
| self.log.info("Node 1 (permissive): JSON boolean and legacy types accepted") | ||
| assert_equal([], self.nodes[1].protx("list", "valid", True)) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", 1)) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", "1")) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", False)) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", 0)) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", "0")) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", "yes")) | ||
| assert_equal([], self.nodes[1].protx("list", "valid", "no")) | ||
|
|
||
| self.log.info("Node 1 (permissive): invalid values still rejected") | ||
| assert_raises_rpc_error(-8, "detailed must be true, false, yes, no, 1 or 0 (not '2')", self.nodes[1].protx, "list", "valid", 2) | ||
| assert_raises_rpc_error(-8, "detailed must be true, false, yes, no, 1 or 0 (not 'notabool')", self.nodes[1].protx, "list", "valid", "notabool") |
There was a problem hiding this comment.
Add missing legacy string coverage for "true"/"false" in permissive mode.
At Line 43-55, permissive-mode assertions cover 1/0, "1"/"0", "yes"/"no" but miss "true" and "false", which are explicitly accepted by ParseBoolV. This leaves part of the legacy input surface untested.
💡 Suggested patch
assert_equal([], self.nodes[1].protx("list", "valid", "yes"))
assert_equal([], self.nodes[1].protx("list", "valid", "no"))
+ assert_equal([], self.nodes[1].protx("list", "valid", "true"))
+ assert_equal([], self.nodes[1].protx("list", "valid", "false"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/functional/rpc_deprecated.py` around lines 43 - 55, Add assertions to
cover legacy string booleans "true" and "false" accepted by ParseBoolV in
permissive mode: update the protx tests that currently assert acceptance for
1/0, "1"/"0", "yes"/"no" by adding assert_equal([], self.nodes[1].protx("list",
"valid", "true")) and assert_equal([], self.nodes[1].protx("list", "valid",
"false")) so the permissive-path for ParseBoolV and the
protx("list","valid",...) calls are fully exercised.
There was a problem hiding this comment.
Good catch! Should be added to non-permissive mode too.
diff --git a/test/functional/rpc_deprecated.py b/test/functional/rpc_deprecated.py
index e7cb4adb98..0780b85158 100755
--- a/test/functional/rpc_deprecated.py
+++ b/test/functional/rpc_deprecated.py
@@ -37,6 +37,8 @@ class DeprecatedRpcTest(BitcoinTestFramework):
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "1")
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", 0)
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "0")
+ assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "true")
+ assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "false")
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "yes")
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "no")
@@ -47,6 +49,8 @@ class DeprecatedRpcTest(BitcoinTestFramework):
assert_equal([], self.nodes[1].protx("list", "valid", False))
assert_equal([], self.nodes[1].protx("list", "valid", 0))
assert_equal([], self.nodes[1].protx("list", "valid", "0"))
+ assert_equal([], self.nodes[1].protx("list", "valid", "true"))
+ assert_equal([], self.nodes[1].protx("list", "valid", "false"))
assert_equal([], self.nodes[1].protx("list", "valid", "yes"))
assert_equal([], self.nodes[1].protx("list", "valid", "no"))
There was a problem hiding this comment.
@UdjinM6, great point! The "true" and "false" string forms should indeed also be verified as rejected on node 0 (strict mode), since they are part of the same ParseBoolV legacy surface. The combined patch covering both nodes is:
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "1")
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", 0)
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "0")
+ assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "true")
+ assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "false")
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "yes")
assert_raises_rpc_error(-8, 'detailed must be a JSON boolean. Pass -deprecatedrpc=permissive_bool to allow legacy boolean parsing.', self.nodes[0].protx, "list", "valid", "no")
...
assert_equal([], self.nodes[1].protx("list", "valid", False))
assert_equal([], self.nodes[1].protx("list", "valid", 0))
assert_equal([], self.nodes[1].protx("list", "valid", "0"))
+ assert_equal([], self.nodes[1].protx("list", "valid", "true"))
+ assert_equal([], self.nodes[1].protx("list", "valid", "false"))
assert_equal([], self.nodes[1].protx("list", "valid", "yes"))
assert_equal([], self.nodes[1].protx("list", "valid", "no"))This ensures the full ParseBoolV legacy input surface — including the "true"/"false" string aliases — is exercised symmetrically across both strict and permissive modes.
2 participants
Issue being fixed or feature implemented
Follow-up for #7099
What was done?
Added functional tests for #7099
How Has This Been Tested?
See new functional tests inside
rpc_deprecated.pyBreaking Changes
N/A
Checklist: