test: add multi-node daemon test infrastructure with peer injection #196
Conversation
…peer inj…" This reverts commit 44a9b6e.
|
🤖 Claude Code Review Status: Complete Current Review: Found 3 issues requiring attention:
The PR adds useful multi-node test infrastructure, but the critical bug in |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // InjectPeerForTesting directly injects a peer into the registry for testing purposes. | ||
| // This method allows deterministic peer setup without requiring actual P2P network connections. | ||
| func (s *Server) InjectPeerForTesting(peerID peer.ID, clientName, dataHubURL string, height uint32, blockHash string) { | ||
| s.addConnectedPeer(peerID, clientName) |
There was a problem hiding this comment.
Critical Bug: Incorrect function call parameters
The addConnectedPeer function signature expects 5 parameters:
func (s *Server) addConnectedPeer(peerID peer.ID, clientName string, height uint32, blockHash *chainhash.Hash, dataHubURL string)But this call only passes 2 parameters. This will cause a compilation error.
Fix: The call should include all required parameters or the function should be updated to match the new signature.
| coinbaseTx := nodeA.MineToMaturityAndGetSpendableCoinbaseTx(t, nodeA.Ctx) | ||
| t.Logf(" Coinbase transaction available for spending: %s", coinbaseTx.TxIDChainHash().String()) | ||
|
|
||
| // nodeA.InjectPeer(t, nodeB) |
There was a problem hiding this comment.
Code cleanup: Commented-out code
This test file contains multiple instances of commented-out code (lines 80, 101, 106-111).
Per project conventions, commented-out code should be removed before merging. If this code is needed for debugging during development, consider using build tags or test helper flags instead.
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| t.Errorf("Timeout waiting for block with hash %s", blockHash.String()) |
There was a problem hiding this comment.
Test error handling: Use t.Fatalf instead of t.Errorf
When a timeout occurs in test wait functions, the test should fail immediately and stop execution. Using t.Errorf continues execution after logging the error, which can lead to confusing follow-up failures.
Suggested fix:
t.Fatalf("Timeout waiting for block with hash %s", blockHash.String())This matches the pattern used in other wait functions like WaitForBlock (line 1332).
Readds #186