test: migrate governance inv cache coverage to unit tests

[thepastaclaw]

Issue being fixed or feature implemented

p2p_governance_invs.py covered one Dash-specific governance invariant by spinning up a node: governance object and vote INVs are remembered, duplicates are suppressed while the request cache is live, and the same INV is accepted again after cleanup expires the cache entry.

That behavior is owned by governance/P2P manager code and can be tested more directly in the unit suite. Moving it there keeps the coverage but avoids a node-spinning functional test.

Timing from this branch:

• Old functional test: /usr/bin/time -p test/functional/p2p_governance_invs.py -> real 2.20
• New unit test: /usr/bin/time -p ./src/test/test_dash --run_test=governance_inv_tests -> real 0.42 on the latest local run

That saves about 1.8 seconds each time the Dash-specific functional suite runs, and the coverage now lives in the faster source-test path.

What was done?

• Added governance_inv_tests to the C++ unit suite.
• Covered governance object and governance vote requested-hash cache expiry directly through CGovernanceManager::ConfirmInventoryRequest() and CheckAndRemove().
• Added an in-process PeerManager::ProcessMessage(INV) test so the replacement still proves governance INVs route through the runtime P2P handler registration path into the governance request cache.
• Added a scheduler test for NetGovernance::Schedule() so periodic cleanup remains covered without node.mockscheduler.
• Removed test/functional/p2p_governance_invs.py from the functional runner.

The only production-code hook is RequestedHashCacheSizeForTesting(), a narrow test-facing accessor needed because ConfirmInventoryRequest() returns true for both fresh and duplicate requests.

How Has This Been Tested?

Environment: macOS arm64, local Dash Core depends/build tree.

• make -C src test/test_dash
• /usr/bin/time -p ./src/test/test_dash --run_test=governance_inv_tests
  • real 0.42
• Baseline before removing the functional test:
  • /usr/bin/time -p test/functional/p2p_governance_invs.py
  • real 2.20
• git diff --check upstream/develop..HEAD
• code-review dashpay/dash upstream/develop test/governance-inv-cache-unit "convert governance inv request expiration functional coverage to unit tests"
  • Recommendation: ship

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

✅ Review complete (commit 9ba733e)

[coderabbitai]

Walkthrough

The functional test test/functional/p2p_governance_invs.py is removed along with its entry in test_runner.py. In its place, a new C++ Boost unit test file src/test/governance_inv_tests.cpp is added and registered in src/Makefile.test.include. To support test introspection, a test-only method RequestedHashCacheSizeForTesting() is declared in governance.h and implemented in governance.cpp, returning the size of m_requested_hash_time under cs_store. The new unit tests cover INV expiration cycles, deduplication, peer-message routing through PeerManager, and scheduler-driven cache cleanup via NetGovernance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes migrating governance INV cache coverage from functional tests to unit tests.
Description check	✅ Passed	The description matches the code changes and explains the test migration, new unit coverage, and removal of the functional test.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

Code Review

Clean, well-scoped migration from a functional test to focused C++ unit tests covering governance inv-cache expiration, scheduler-driven cleanup, and the full PeerManager::ProcessMessage→AlreadyHave→NetGovernance routing path. The only production change is a narrow, properly-annotated test accessor. No correctness, safety, or consensus concerns. A few non-blocking suggestions about commit hygiene and one inaccurate fixture comment.

🟡 2 suggestion(s) | 💬 1 nitpick(s)

Findings not posted inline (1)

These findings could not be anchored to the current diff, but they are still part of this review.

• [SUGGESTION] <commit-history>:1: Squash iterative review-fix commits before merge — The stack contains two commits whose own messages identify them as in-PR review responses: b58185e ("Addresses the code-review pass that flagged the previous version of this suite...") patches fe638ec, and 22ac033 explicitly replaces the AlreadyHave fixture test added by b58185e with...

🤖 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 `src/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:67-69: Manual NetGovernance registration loses init-path coverage
  The deleted functional test implicitly covered init.cpp registering NetGovernance with PeerManager and scheduling its cleanup task. The replacement test installs the handler manually via AddExtraHandler, and the scheduler test constructs its own NetGovernance instance, so a regression in init.cpp's registration/scheduling path would no longer be caught even though governance INV handling would be broken at runtime. The functional-to-unit migration is the right direction; consider whether a minimal complementary check on the production init path is worth adding, otherwise document the deliberate trade-off.

In `<commit-history>`:
- [SUGGESTION] <commit-history>:1: Squash iterative review-fix commits before merge
  The stack contains two commits whose own messages identify them as in-PR review responses: b58185e801 ("Addresses the code-review pass that flagged the previous version of this suite...") patches fe638ec80c, and 22ac033996 explicitly replaces the AlreadyHave fixture test added by b58185e801 with a fuller PeerManager::ProcessMessage-driven test. Landing all three on develop preserves a dead-end intermediate (the AlreadyHave fixture test from b58less58e801 that 22ac033996 removes) with no bisect/blame benefit. Squash fe638ec80c + b58185e801 + 22ac033996 into a single coherent commit such as "test: add governance_inv_tests covering inv request cache via PeerManager::ProcessMessage". The Schedule-path test in 48f4985845, the accessor commit, and the functional-test removal (72e8a5444f) can remain as standalone commits.

Note: GitHub does not allow me to submit APPROVE on my own PR, so this is posted as a COMMENT while preserving the verified non-blocking findings.

[thepastaclaw]

🟡 Suggestion: Manual NetGovernance registration loses init-path coverage

The deleted functional test implicitly covered init.cpp registering NetGovernance with PeerManager and scheduling its cleanup task. The replacement test installs the handler manually via AddExtraHandler, and the scheduler test constructs its own NetGovernance instance, so a regression in init.cpp's registration/scheduling path would no longer be caught even though governance INV handling would be broken at runtime. The functional-to-unit migration is the right direction; consider whether a minimal complementary check on the production init path is worth adding, otherwise document the deliberate trade-off.

_{source: ['codex']}

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/test/governance_inv_tests.cpp (1)

31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid copying the production timeout into the test.

This local mirror can silently weaken coverage if the runtime value changes. In particular, a shorter production timeout would still pass this suite because every expiry assertion only checks > 60s. Please source the value from production code (or expose a test-visible constant) so the migrated unit test keeps pinning the real behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/governance_inv_tests.cpp` around lines 31 - 33, The test is
hard-coding a local copy of RELIABLE_PROPAGATION_TIME, which can drift from the
production behavior in governance::CheckAndRemove. Update the
governance_inv_tests.cpp fixture to read the timeout from the production source
or a test-visible constant instead of defining its own static constexpr, and
make the expiry assertions reference that shared symbol so the test keeps
validating the real runtime value.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/test/governance_inv_tests.cpp`:
- Around line 209-221: The test in governance_inv_tests.cpp is still relying on
a wall-clock sleep/poll loop after scheduler.MockForward(), which can flake
before the worker thread actually runs NetGovernance::Schedule()’s cleanup path.
Replace the loop with a deterministic synchronization point from the scheduled
work itself, such as a promise or condition_variable signaled by the cleanup
callback, and then stop the scheduler and join the worker only after that signal
is received.

---

Nitpick comments:
In `@src/test/governance_inv_tests.cpp`:
- Around line 31-33: The test is hard-coding a local copy of
RELIABLE_PROPAGATION_TIME, which can drift from the production behavior in
governance::CheckAndRemove. Update the governance_inv_tests.cpp fixture to read
the timeout from the production source or a test-visible constant instead of
defining its own static constexpr, and make the expiry assertions reference that
shared symbol so the test keeps validating the real runtime value.

🪄 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: f6e3209a-b3e4-4505-a8f6-c241b796562a

📥 Commits

Reviewing files that changed from the base of the PR and between 22ac033 and d6942bf.

📒 Files selected for processing (6)

• src/Makefile.test.include
• src/governance/governance.cpp
• src/governance/governance.h
• src/test/governance_inv_tests.cpp
• test/functional/p2p_governance_invs.py
• test/functional/test_runner.py

💤 Files with no reviewable changes (2)

• test/functional/test_runner.py
• test/functional/p2p_governance_invs.py

🚧 Files skipped from review as they are similar to previous changes (3)

• src/governance/governance.h
• src/Makefile.test.include
• src/governance/governance.cpp

[thepastaclaw]

Code Review

The current PR head keeps the governance INV behavior covered by focused unit tests, and I found no new in-scope correctness issues in the latest push. Prior finding prior-2 remains a valid non-blocking coverage trade-off; prior-1 and prior-3 are fixed by the rewritten comment and cleaned-up four-commit stack.

🟡 1 suggestion(s)

Carried-forward findings already raised (1)

These findings were not re-posted as new inline comments because an existing review thread already covers them.

• [SUGGESTION] (deduped existing open thread) src/test/governance_inv_tests.cpp:62-69: Manual NetGovernance registration loses init-path coverage — The deleted functional test started a real node, so it implicitly covered init.cpp installing NetGovernance into PeerManager and the production scheduler setup. The replacement fixture manually installs the handler with AddExtraHandler, and the scheduler test constructs a local NetGovernance inst...

🤖 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 `src/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:62-69: Manual NetGovernance registration loses init-path coverage
  The deleted functional test started a real node, so it implicitly covered init.cpp installing NetGovernance into PeerManager and the production scheduler setup. The replacement fixture manually installs the handler with AddExtraHandler, and the scheduler test constructs a local NetGovernance instance, so a future regression in init.cpp registration or scheduling would no longer be caught even though governance INV handling would fail at runtime. This is a reasonable unit-test boundary, but it is still a coverage reduction from the removed functional test and should be an intentional trade-off.

[thepastaclaw]

Investigated the earlier linux64_multiprocess-test / Test source failure from run 28376157116 / job 84073624710. That run was on old head d6942bfc99 and failed in feature_llmq_singlenode.py while waiting for recovered signatures (check_sigs timed out after 60s). The migrated governance INV unit tests were not on the failing path, and the touched governance test files are unrelated to that LLMQ functional-test timeout.

I am not pushing, rebasing, force-pushing, creating an empty commit, or rerunning CI for this tracker item. Current head is fc2aedea51, and its fresh CI run is already in progress from the later push.

[thepastaclaw]

Code Review

Carried-forward prior findings: prior-2 is STILL VALID because the replacement unit tests still bypass init.cpp's production NetGovernance registration and handler scheduling path. New findings in the latest delta: fc2aede is a patch-over-patch follow-up over earlier PR test/accessor commits and should be folded before merge. CodeRabbit had no actionable inline findings to validate.

Note: posted as a COMMENT review because GitHub does not allow approving my own PR.

🟡 2 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 `src/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:63-65: Manual NetGovernance registration loses init-path coverage
  The deleted functional test started a real node, so it implicitly covered init.cpp installing NetGovernance into PeerManager and the production ScheduleHandlers path invoking handler scheduling. The replacement fixture manually installs NetGovernance with AddExtraHandler here, and the scheduler test constructs a local NetGovernance instance directly, so a regression that removes or skips the production init.cpp registration/scheduling path would no longer be caught even though governance INV handling would fail at runtime. This may be an acceptable unit-test boundary, but it is still a coverage reduction from the removed functional test and should be an intentional trade-off.

In `<commit:fc2aedea>`:
- [SUGGESTION] <commit:fc2aedea>:1: Fold the follow-up test tightening into the earlier commits
  Commit fc2aedea511dca00f9ce0ec86295a4332b705c86 edits code introduced earlier in this same PR: it exposes RELIABLE_PROPAGATION_TIME for the new unit tests and rewrites the scheduler test wait/stop logic added by 6bd09cb12b. Since these changes have not shipped independently, keeping them as a terminal cleanup commit adds patch-over-patch noise to a project that expects one logical change per commit. Fold the constant exposure into the accessor/unit-test commit and the scheduler cleanup into the scheduler coverage commit before merge.

[github-actions]

This pull request has conflicts, please rebase.

[thepastaclaw]

Code Review

Both prior findings remain STILL VALID at head 9ba733e. Carried forward: (1) the test fixture at src/test/governance_inv_tests.cpp:63-65 still manually installs NetGovernance via AddExtraHandler rather than exercising init.cpp's production registration path, and (2) the terminal 'tighten' commit 9ba733e is a bodyless patch-over-patch follow-up over earlier unshipped PR commits. No new latest-delta findings. CodeRabbit had no actionable inline findings.

🟡 2 suggestion(s)

Carried-forward prior findings (2)

Both prior findings were re-validated against 9ba733e9; both remain STILL VALID. No new findings were found in the fc2aedea..9ba733e9 delta.

• [SUGGESTION] src/test/governance_inv_tests.cpp:63: Manual NetGovernance registration loses init-path coverage — STILL VALID (carried forward from prior-2). The deleted functional test booted a real node, implicitly covering init.cpp installing NetGovernance into PeerManager and ScheduleHandlers invoking the periodic task. The replacement fixture manually calls m_node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(...)) here, and net_governance_schedule_drives_check_and_remove constructs a fresh local NetGovernance for its own scheduler. A regression that removes or reorders init.cpp's production hookup (so governance INVs stop routing or periodic cleanup stops being scheduled at runtime) would still pass this suite. This is a reasonable unit-test boundary but is a real coverage reduction from the removed functional test and should be an acknowledged trade-off rather than treated as an equivalent replacement.
• [SUGGESTION] <commit:9ba733e9>: Fold the terminal 'tighten' commit into the earlier commits it patches — STILL VALID (carried forward from prior-2, previously against fc2aede, now recreated as 9ba733e after rebase). This terminal commit has an empty body and only edits code introduced earlier in this same PR: it (a) hoists RELIABLE_PROPAGATION_TIME from an anonymous namespace in src/governance/governance.cpp into a public governance::RELIABLE_PROPAGATION_TIME in governance.h, deleting the local mirror that c1ec85b had just added to the test file, and (b) rewrites the scheduler test wait/stop logic that 2ba9ef8 had just added into a scheduleFromNow stop-marker pattern. Because dashpay/dash merges without squashing, this fixup lands in permanent develop history alongside the code it patches, splitting a single logical change across two commits and reducing bisect/blame precision. Fold the constant-relocation hunks into c1ec85b (or a small standalone refactor commit ahead of it) and the scheduler-test rewrite into 2ba9ef8 before merge, per the project's one-logical-change-per-commit convention.

🤖 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 `src/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:63: Manual NetGovernance registration loses init-path coverage
  STILL VALID (carried forward from prior-2). The deleted functional test booted a real node, implicitly covering init.cpp installing NetGovernance into PeerManager and ScheduleHandlers invoking the periodic task. The replacement fixture manually calls `m_node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(...))` here, and net_governance_schedule_drives_check_and_remove constructs a fresh local NetGovernance for its own scheduler. A regression that removes or reorders init.cpp's production hookup (so governance INVs stop routing or periodic cleanup stops being scheduled at runtime) would still pass this suite. This is a reasonable unit-test boundary but is a real coverage reduction from the removed functional test and should be an acknowledged trade-off rather than treated as an equivalent replacement.

In `<commit:9ba733e9>`:
- [SUGGESTION] <commit:9ba733e9>:1: Fold the terminal 'tighten' commit into the earlier commits it patches
  STILL VALID (carried forward from prior-2, previously against fc2aedea, now recreated as 9ba733e9b3244b06aa8a32ba72726ee0d1e65186 after rebase). This terminal commit has an empty body and only edits code introduced earlier in this same PR: it (a) hoists `RELIABLE_PROPAGATION_TIME` from an anonymous namespace in src/governance/governance.cpp into a public `governance::RELIABLE_PROPAGATION_TIME` in governance.h, deleting the local mirror that c1ec85bb58 had just added to the test file, and (b) rewrites the scheduler test wait/stop logic that 2ba9ef82b8 had just added into a `scheduleFromNow` stop-marker pattern. Because dashpay/dash merges without squashing, this fixup lands in permanent develop history alongside the code it patches, splitting a single logical change across two commits and reducing bisect/blame precision. Fold the constant-relocation hunks into c1ec85bb58 (or a small standalone refactor commit ahead of it) and the scheduler-test rewrite into 2ba9ef82b8 before merge, per the project's one-logical-change-per-commit convention.