test: various improvements for functional tests feature_llmq_{evo,rotation,chainlocks.py} #6832
Conversation
…pay#6430 Reduced amount of mn from 4 to just 3 as no needs for rotation quorum anymore Removed IS spork changes
…nly 4 evo nodes, less blocks generated
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/functional/feature_llmq_chainlocks.py (1)
26-34: Tighten helper and fix flake8 E265; reduce log spam.
- Replace per-node log lines with a single log.
- Remove/comment-space the “confirmations” commented assert to satisfy E265 and avoid confusion (not required and may be false).
- Allow adjustable delay.
Apply:
-def sleep_and_assert_no_cl(self, block_hash): - time.sleep(5) - - for node in self.nodes: - self.log.info(f"Expecting no ChainLock for {block_hash}") - block = node.getblock(block_hash) - #assert_equal(block["confirmations"], 0) - assert not block["chainlock"] +def sleep_and_assert_no_cl(self, block_hash, delay=5): + time.sleep(delay) + self.log.info(f"Expecting no ChainLock for {block_hash} on all nodes") + for node in self.nodes: + block = node.getblock(block_hash) + # assert_equal(block["confirmations"], 0) # not required; CL status alone is checked + assert not block["chainlock"]test/functional/feature_llmq_evo.py (1)
176-191: Robust Evo presence checks in both lists — good.
- Validates type and membership in masternodelist() and masternodelist(mode="evo").
- Minor nit: consider early-break once found to avoid N^2 scan, albeit tiny here.
- for _, mn in mn_list.items(): + for _, mn in mn_list.items(): if mn['proTxHash'] == evo_protx: found_in_mns = True assert_equal(mn['type'], "Evo") - ... + breaktest/functional/test_framework/test_framework.py (2)
2312-2322: Gate extra rotation debug via env var to avoid code edits.Keeps logs off by default but easy to enable in CI/dev.
-extra_debug_rotation_info = False -if extra_debug_rotation_info: +extra_debug_rotation_info = bool(int(os.getenv("EXTRA_DEBUG_ROTATION_INFO", "0"))) +if extra_debug_rotation_info:
1956-1959: Confirm removal ofexpectedparameter support and optionally provide a no-chainlock helperThe
wait_for_chainlocked_block_all_nodeshelper no longer acceptsexpected=Falseand all tests have been updated to avoid negative waits. To simplify future tests and prevent per-test sleeps, consider adding anassert_no_chainlock_all_nodes(block_hash, timeout=…)helper that polls each node and raises if a chainlock appears.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
test/functional/feature_asset_locks.py(1 hunks)test/functional/feature_llmq_chainlocks.py(2 hunks)test/functional/feature_llmq_evo.py(5 hunks)test/functional/feature_llmq_rotation.py(4 hunks)test/functional/test_framework/test_framework.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_asset_locks.pytest/functional/feature_llmq_chainlocks.pytest/functional/feature_llmq_evo.pytest/functional/test_framework/test_framework.pytest/functional/feature_llmq_rotation.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/feature_llmq_evo.py
🧬 Code graph analysis (3)
test/functional/feature_asset_locks.py (1)
test/functional/test_framework/test_framework.py (1)
activate_mn_rr(1581-1582)
test/functional/feature_llmq_evo.py (2)
test/functional/test_framework/test_framework.py (1)
set_dash_test_params(1508-1530)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/feature_llmq_rotation.py (1)
test/functional/test_framework/test_framework.py (4)
set_dash_test_params(1508-1530)set_dash_llmq_test_params(1584-1590)delay_v20_and_mn_rr(1533-1535)activate_v20(1578-1579)
🪛 Flake8 (7.2.0)
test/functional/feature_llmq_chainlocks.py
[error] 32-32: block comment should start with '# '
(E265)
🔇 Additional comments (12)
test/functional/feature_asset_locks.py (1)
627-627: No remainingactivate_mn_rrcalls with arguments—approved.
Grep only matched the method definition, confirming no legacy usages remain.test/functional/feature_llmq_chainlocks.py (2)
19-19: Add time import — OK.Used by the new helper; no issues.
97-97: Runtime improvement preserved while matching new framework API — good.Single 5s wait for all nodes replaces 5s-per-node polling.
test/functional/feature_llmq_rotation.py (4)
54-57: Per-node vbparams and earlier activation — good.
- testdummy vbparams set consistently on all 9 nodes.
- v20/mn_rr delayed to height 300 via helper for clarity.
109-110: Use mn_rr_height consistently for v20 activation — good.Avoids hard-coding; aligns tests with framework knobs.
184-185: Second rotation cycle expectations updated — good.Matches removal of 102-type quorums.
163-165: Approve: No lingering LLMQ type-102 references
Refined search found no instances ofllmq_type = 102,QuorumId(102), orq_102intest/functional—dropping type-102 expectations is safe.test/functional/feature_llmq_evo.py (4)
52-55: Lean topology and explicit mn_rr_height — good.3 regular nodes + 4 Evo nodes keeps quorum minimal; explicit 320 height is clear.
70-70: Accurate initial counts.expected_mns_count=2, expected_evo_count=0 reflects the new topology.
81-81: Progressive Evo count checks — good.Validates growth per iteration.
96-98: MN RR activation flow/logging — good.Switch to framework’s parameterless activate_mn_rr and concise f-string log.
test/functional/test_framework/test_framework.py (1)
1581-1583: approve code changes
No callers of activate_mn_rr pass an argument—removing the parameter is safe.
Co-authored-by: PastaPastaPasta <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/feature_llmq_chainlocks.py (1)
26-34: Reduce log noise and clarify scope of the check.Log once before the loop and downgrade per-node messages to debug.
Apply this diff:
- for node in self.nodes: - self.log.info(f"Expecting no ChainLock for {block_hash}") + self.log.info(f"Expecting no ChainLock for {block_hash} on all nodes") + for idx, node in enumerate(self.nodes): + self.log.debug(f"Checking absence of ChainLock for {block_hash} on node[{idx}]")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/feature_llmq_chainlocks.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_llmq_chainlocks.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
🪛 Flake8 (7.2.0)
test/functional/feature_llmq_chainlocks.py
[error] 32-32: block comment should start with '# '
(E265)
🔇 Additional comments (2)
test/functional/feature_llmq_chainlocks.py (2)
19-19: LGTM on adding time import.Needed by the new helper; no issues.
97-97: Good swap: single sleep+assert avoids O(n) per-node waits.This should cut runtime while preserving correctness in the negative-CL scenario.
| def sleep_and_assert_no_cl(self, block_hash, sleep_time=5): | ||
| time.sleep(sleep_time) | ||
|
|
||
| for node in self.nodes: | ||
| self.log.info(f"Expecting no ChainLock for {block_hash}") | ||
| block = node.getblock(block_hash) | ||
| #assert_equal(block["confirmations"], 0) | ||
| assert not block["chainlock"] | ||
|
|
There was a problem hiding this comment.
Fix Flake8 E265 and remove stale commented assertion.
The commented line lacks a space after '#', tripping E265. It’s also misleading (confirmations can be >0). Remove it.
Apply this diff:
- #assert_equal(block["confirmations"], 0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def sleep_and_assert_no_cl(self, block_hash, sleep_time=5): | |
| time.sleep(sleep_time) | |
| for node in self.nodes: | |
| self.log.info(f"Expecting no ChainLock for {block_hash}") | |
| block = node.getblock(block_hash) | |
| #assert_equal(block["confirmations"], 0) | |
| assert not block["chainlock"] | |
| def sleep_and_assert_no_cl(self, block_hash, sleep_time=5): | |
| time.sleep(sleep_time) | |
| for node in self.nodes: | |
| self.log.info(f"Expecting no ChainLock for {block_hash}") | |
| block = node.getblock(block_hash) | |
| assert not block["chainlock"] |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 32-32: block comment should start with '# '
(E265)
🤖 Prompt for AI Agents
In test/functional/feature_llmq_chainlocks.py around lines 26 to 34, remove the
stale commented assertion line "#assert_equal(block["confirmations"], 0)" to fix
Flake8 E265 (missing space after '#') and to avoid misleading code; leave the
time.sleep, logging, getblock call, and the assert not block["chainlock"] check
intact.
Issue being fixed or feature implemented
These functional tests are one of the slowest to run with tsan on CI:
What was done?
feature_llmq_chainlocks.py
It waits only once for all nodes to check missing chainlock; instead of waiting 5 second for each node. Used to be: wait->check->wait->check... Become: wait->check->check->check...
feature_llmq_evo.py
masternodelistby checking results ofmode=evofeature_llmq_rotation.py
test_framework
Minor improvements and refactorings:
activate_mn_rrwait_for_chainlocked_block_all_nodesby removing flagexpectedHow Has This Been Tested?
Run functional tests locally multiple times. Median time is improved:
feature_llmq_chainlocks.py: 120s -> 75s (localhost), 301s -> 105s (CI)feature_llmq_evo.py: 108s -> 74s (localhost), 198s -> 118s (CI)feature_llmq_rotation.py: 154s -> 93s (localhost), 315s -> 141s (CI)Breaking Changes
N/A
Checklist: