Test/p2p handler coverage#822
Open
liam wants to merge 7 commits intobsv-blockchain:mainfrom
Open
Conversation
handleNodeStatusTopic was at 39.1% — only happy paths and the size guard ran. Six focused unit tests cover the validation branches: - bad JSON: no notification, no peer state mutation - peer ID spoofing: sender peer != claimed peer, ban score +20, no notif - invalid BaseURL: SSRF rejection (loopback), ban score +20, no notif - invalid BestBlockHash: notification fires before parse, peer height not updated - notification channel full: unbuffered chan exercises default branch - storage-update side effect: Storage="full" reaches the registry Lifts handleNodeStatusTopic 39.1% -> 89.1% and updateStorage 0% -> 100%. Package total 73.9% -> 74.9%.
shouldSkipDuringSync was at 17.6% — only the no-sync-peer return ran. Six tests for each exit path: - no sync peer: syncCoordinator nil -> false - not syncing: FSM RUNNING -> false - announcement below sync peer height -> true - announcement from non-sync peer -> true - originator string fails to decode -> true (error short-circuit) - happy path: syncing, height ok, sender == sync peer -> false Lifts shouldSkipDuringSync 17.6% -> 100%. Package total 74.9% -> 75.1%.
Both functions were unexercised: handleBanEvent at 36.4% (only the non-add early return ran via integration), disconnectBannedPeerByID at 0%. Six tests across the dispatch chain: - handleBanEvent: non-add action, empty PeerID, invalid PeerID decode, valid PeerID dispatching to disconnectBannedPeerByID - disconnectBannedPeerByID: peer found in connected list (registry removal), peer not in connected list (no mutation) MockServerP2PClient.GetPeers gains an optional peers field for tests that need to drive specific connected-peer lists. Existing callers fall through to the previous empty-PeerInfo default. Lifts handleBanEvent 36.4% -> 100%, disconnectBannedPeerByID 0% -> 100%. Package total 75.1% -> 75.7%.
Apply gofmt to mock.go (struct field alignment) and reorder imports in server_handler_test.go (alphabetical: go-bt before go-p2p-message-bus). Adds the missing blank line before the merged-in cleanup tests block.
Sixteen Client.go methods sat at 0% coverage — none of the catchup recording, reputation update, validity reporting, peer-info, or byte-accounting wrappers had unit tests. The wrappers are uniform: build proto, call gRPC, propagate gRPC errors, return service error on Ok/Success false, otherwise return nil. Tests cover the three exit paths per method using the existing MockPeerServiceClient pattern: - ok / grpc_error / not_ok for: RecordCatchupAttempt, RecordCatchupSuccess, RecordCatchupFailure, RecordCatchupMalicious, UpdateCatchupError, UpdateCatchupReputation, RecordBytesDownloaded - ok_specific_peer / ok_all_peers / grpc_error / not_ok for ResetReputation - ok / grpc_error / not_success for ReportValidSubtree, ReportValidBlock - ok / grpc_error for GetPeersForCatchup, GetPeerRegistry, IsPeerMalicious, IsPeerUnhealthy - found / not_found / grpc_error for GetPeer MockPeerServiceClient gains a RecordBytesDownloadedFunc field — the existing method always returned Ok=true, blocking error/!ok testing. The fallback preserves the prior behaviour. Coverage: each of the 16 wrappers 0% -> 100%, package total 75.7% -> 80.1%. NewClient / NewClientWithAddress remain at 0% (real gRPC dial, out of scope for unit tests).
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: This PR adds comprehensive test coverage for P2P Client gRPC wrapper methods and Server handler functions. The changes are test-only and follow consistent patterns. Summary:
Quality observations:
No issues found. The tests are well-structured, defensive, and align with the project's testing standards in AGENTS.md. This is an advisory review. Human reviewers make final approval decisions. |
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-06 16:30 UTC |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.