test: improve trigger checks in feature_governance_cl.py
#6926
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR refactors the governance cleanup verification in feature_governance_cl.py by replacing a brittle 1-second sleep with explicit log assertions. The test now uses assert_debug_log to deterministically confirm that UpdateCachesAndClean scheduler events execute, then verifies cleanup completion via RPC calls. This approach eliminates race conditions where cleanup might not complete within the fixed sleep window. The change aligns with Dash Core's governance subsystem architecture where scheduled cleanup occurs in two phases: marking objects for deletion, then removing them after GOVERNANCE_DELETION_DELAY. The test exercises this flow by advancing mock time and triggering the scheduler explicitly.
PR Description Notes:
- The PR description is completely empty - all fields (issue being fixed, what was done, how tested, breaking changes) are blank.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| test/functional/feature_governance_cl.py | 2/5 | Replaced time.sleep with deterministic log assertions but iterates over all nodes including isolated node 5, causing potential RPC failures; contains redundant mocktime assignments |
Confidence score: 2/5
- This PR has significant issues that could cause test failures and requires careful review before merging.
- Score lowered because: (1) the code iterates over
self.nodesincluding node 5 which remains isolated from the network, likely causing RPC failures whenmockscheduler()is called on the disconnected node; (2) redundantnode.mocktimeassignments on lines145 and 153 have no effect sincesetmocktime()already updates internal time state; (3) the empty PR description provides no context for reviewers to understand the motivation or validate correctness. - Pay close attention to lines 143-155 where the node iteration occurs - verify that node 5's isolation doesn't break the
mockscheduler()calls or consider excluding it from the loop.
Sequence Diagram
sequenceDiagram
participant User
participant Node0
participant Nodes1_4 as "Nodes 1-4"
participant Node5
participant Blockchain
participant GovernanceSystem as "Governance System"
participant ChainLock as "ChainLock System"
User->>Node0: "Enable DKG Spork"
Node0->>Nodes1_4: "Sync spork update"
User->>Node0: "Mine cycle quorum"
Node0->>Blockchain: "Generate blocks"
Blockchain->>ChainLock: "Activate ChainLocks"
User->>Node0: "Enable Superblocks Spork"
Node0->>Nodes1_4: "Sync spork update"
User->>Node0: "Move to superblock cycle start"
Node0->>Blockchain: "Generate blocks to cycle boundary"
User->>Node0: "Prepare Proposal_0 and Proposal_1"
Node0->>GovernanceSystem: "gobject prepare (P0 & P1)"
GovernanceSystem-->>Node0: "Return collateral hashes"
User->>Node0: "Wait 10 minutes (bump mocktime)"
User->>Node0: "Generate 6 confirmation blocks"
Node0->>Blockchain: "Mine 6 blocks"
User->>Node0: "Submit proposals"
Node0->>GovernanceSystem: "gobject submit (P0 & P1)"
GovernanceSystem-->>Node0: "Store proposals"
User->>Node5: "Isolate node 5"
Node5--xNodes1_4: "Disconnect from network"
User->>Node0: "Vote for proposals"
Node0->>GovernanceSystem: "gobject vote-many (P0 & P1)"
GovernanceSystem->>Nodes1_4: "Sync votes"
Note over Node5: Node5 misses votes
User->>Node0: "Move into superblock maturity window"
Node0->>Blockchain: "Generate blocks"
GovernanceSystem->>Nodes1_4: "Create trigger for superblock"
Note over Node5: Node5 misses trigger
User->>Node0: "Mine to superblock height"
Node0->>Blockchain: "Generate remaining blocks"
Node0->>Blockchain: "Mine superblock with payouts"
Blockchain->>ChainLock: "Request ChainLock"
ChainLock-->>Blockchain: "ChainLock superblock"
User->>Node0: "Mine superblock_cycle + 1 blocks"
loop sb_cycle + 1 times
Node0->>Blockchain: "Generate block"
end
User->>Nodes1_4: "Bump time and trigger scheduler cleanup"
Nodes1_4->>GovernanceSystem: "mockscheduler (5 min delta)"
GovernanceSystem-->>Nodes1_4: "Mark old triggers for deletion"
User->>Nodes1_4: "Bump time again and trigger cleanup"
Nodes1_4->>GovernanceSystem: "mockscheduler (10 min delta)"
GovernanceSystem->>GovernanceSystem: "Delete old triggers (UpdateCachesAndClean)"
GovernanceSystem-->>Nodes1_4: "Triggers removed"
User->>Node5: "Reconnect isolated node"
Node5->>Node0: "Reconnect to network"
Note over Node5: mnsync status: not synced
User->>Node0: "Generate 1 block"
Node0->>Blockchain: "Mine block"
ChainLock->>Blockchain: "Create ChainLock"
Blockchain->>Node5: "Sync via ChainLock"
Note over Node5: Skips governance checks<br/>due to ChainLock coverage
Context used:
- Context from
dashboard- CLAUDE.md (source)
1 file reviewed, 3 comments
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe functional test test/functional/feature_governance_cl.py was refactored to remove real-time Sequence Diagram(s)sequenceDiagram
participant Test
participant NodeA as Node A (daemon)
participant NodeB as Node B (daemon)
participant MockScheduler
participant RPC as Governance RPC
Note over Test,NodeA: Setup governance objects and nodes
Test->>NodeA: create trigger/object
Test->>NodeB: create trigger/object
Note over Test,MockScheduler: Advance ~5 minutes to trigger cache cleaning
Test->>NodeA: setmocktime(+5m)
Test->>NodeB: setmocktime(+5m)
Test->>MockScheduler: run scheduler on NodeA and NodeB
MockScheduler->>NodeA: execute scheduled cleanup
MockScheduler->>NodeB: execute scheduled cleanup
NodeA-->>Test: debug-log "UpdateCachesAndClean" (assert)
NodeB-->>Test: debug-log "UpdateCachesAndClean" (assert)
Note over Test,MockScheduler: Advance additional ~10 minutes for deletion
Test->>NodeA: setmocktime(+10m)
Test->>NodeB: setmocktime(+10m)
Test->>MockScheduler: run scheduler again on NodeA and NodeB
MockScheduler->>NodeA: execute deletion tasks
MockScheduler->>NodeB: execute deletion tasks
Test->>RPC: getgovernanceinfo / list triggers
RPC-->>Test: returns empty triggers list (assert)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)test/functional/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-28T08:54:00.392ZApplied to files:
🧬 Code graph analysis (1)test/functional/feature_governance_cl.py (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/feature_governance_cl.py(1 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_governance_cl.py
🧬 Code graph analysis (1)
test/functional/feature_governance_cl.py (2)
test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)test/functional/test_framework/util.py (1)
assert_equal(69-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/functional/feature_governance_cl.py (2)
141-147: DRY the two scheduler phases with a small parametrized loop.Removes duplication while preserving behavior and assertions.
Apply this diff:
- delta = 5 * 60 - self.mocktime += delta - for node in self.nodes: - with node.assert_debug_log(expected_msgs=['UpdateCachesAndClean']): - node.setmocktime(self.mocktime) - node.mockscheduler(delta) - # Move forward to satisfy GOVERNANCE_DELETION_DELAY, should actually remove old triggers now - delta = 10 * 60 - self.mocktime += delta - for node in self.nodes: - with node.assert_debug_log(expected_msgs=['UpdateCachesAndClean -- Governance Objects: 0']): - node.setmocktime(self.mocktime) - node.mockscheduler(delta) + for delta, expected in ( + (5 * 60, ['UpdateCachesAndClean']), # mark old triggers for deletion + (10 * 60, ['UpdateCachesAndClean -- Governance Objects: 0']), # deletion after delay + ): + self.mocktime += delta + for node in self.nodes: + with node.assert_debug_log(expected_msgs=expected): + node.setmocktime(self.mocktime) + node.mockscheduler(delta)
148-153: Log match may be brittle; RPC already asserts 0.Optional: relax the second log check to reduce coupling to exact formatting.
Apply this minimal change:
- with node.assert_debug_log(expected_msgs=['UpdateCachesAndClean -- Governance Objects: 0']): + with node.assert_debug_log(expected_msgs=['UpdateCachesAndClean -- Governance Objects:']):Rationale: RPC below enforces zero objects; the log assertion then only checks that the cleanup path ran, avoiding failures on minor message-format changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/feature_governance_cl.py(1 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_governance_cl.py
🧠 Learnings (1)
📚 Learning: 2025-10-28T08:54:00.383Z
Learnt from: UdjinM6
PR: dashpay/dash#6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.383Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
test/functional/feature_governance_cl.py
🧬 Code graph analysis (1)
test/functional/feature_governance_cl.py (2)
test/functional/test_framework/test_node.py (1)
assert_debug_log(444-472)test/functional/test_framework/util.py (1)
assert_equal(69-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (1)
test/functional/feature_governance_cl.py (1)
155-156: Including the isolated node here is correct.Running cleanup and verifying RPC state on all nodes (including the isolated one) matches the scheduler’s node-local behavior and the intent captured in this PR. Looks good.
Based on learnings
UdjinM6
force-pushed
the
fix_gov_cl_test_sleep
branch
from
da330e0 to
a7fe1eb
Compare
UdjinM6
force-pushed
the
fix_gov_cl_test_sleep
branch
from
a7fe1eb to
562bb5a
Compare
UdjinM6
changed the title
feature_governance_cl.pyfeature_governance_cl.py
1 participant
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist: