refactor(stream): improve deadlines #59
Conversation
📝 WalkthroughWalkthroughThe changes introduce a standardized mock implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant MockStream as mocks.Stream
participant StreamManager as StreamManager
Test->>MockStream: NewStream(t)
Test->>MockStream: MustWrite(data)
Test->>StreamManager: ReadStreamWithBuffer(MockStream, ...)
StreamManager->>MockStream: applyDeadline (SetReadDeadline)
alt Deadline error (mocknet)
StreamManager->>MockStream: isMockNetError(err)
StreamManager-->>Test: Continue (ignore error)
else Deadline error (real)
StreamManager->>MockStream: Reset()
StreamManager-->>Test: Return error
end
StreamManager->>MockStream: Read()
StreamManager-->>Test: Return data or error
This diagram illustrates the new, centralized deadline handling and error tolerance for both real and mock streams during read operations in tests. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
p2p/mocks/stream.go (2)
43-44: Consider using a larger range for random stream IDs.The current implementation limits stream IDs to values under 10,000, which could potentially lead to ID collisions in large-scale tests with many mock streams.
- id: rand.Uint64() % 10_000, + id: rand.Uint64(),
1-218: Consider adding a Reset method to clear buffer contents.The mock implementation is comprehensive but lacks a method to reset the buffer contents between test cases without creating a new instance.
Consider adding a method like:
func (s *Stream) Reset() error { s.mu.Lock() defer s.mu.Unlock() s.buffer.Reset() s.readDeadline = time.Time{} s.writeDeadline = time.Time{} s.errSetReadDeadLine = false s.errSetWriteDeadLine = false s.errRead = false return nil }p2p/stream_manager_test.go (1)
16-106: Consider adding boundary test cases.The test cases cover basic scenarios, but could be enhanced with boundary conditions such as empty payloads or exactly at the
MaxPayloadsize.Add test cases for:
- Empty payload (0 bytes)
- Payload exactly at MaxPayload
- Payload exceeding MaxPayload by 1 byte
Example test case:
{ name: "payload at max size", expectedLength: MaxPayload - PayloadHeaderLen, expectError: false, streamProvider: func() network.Stream { s := mocks.NewStream(t) buf := make([]byte, PayloadHeaderLen) binary.LittleEndian.PutUint32(buf, MaxPayload - PayloadHeaderLen) s.MustWrite(buf) s.MustWrite(bytes.Repeat([]byte("a"), int(MaxPayload - PayloadHeaderLen))) return s }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
keysign/signature_notifier_test.go(0 hunks)p2p/mocks/stream.go(1 hunks)p2p/party_coordinator_test.go(0 hunks)p2p/stream_manager.go(4 hunks)p2p/stream_manager_test.go(7 hunks)
💤 Files with no reviewable changes (2)
- p2p/party_coordinator_test.go
- keysign/signature_notifier_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
p2p/stream_manager.go (1)
p2p/mocks/stream.go (1)
Stream(19-34)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (13)
p2p/mocks/stream.go (3)
57-86: Well-implemented blocking I/O simulation with deadlines.The implementation correctly simulates blocking I/O with timeout behavior using goroutines and channels, properly handling both immediate reads and deadline-based reads.
93-122: Consistent implementation with Read method.The Write method follows the same pattern as the Read method, ensuring consistency in the blocking I/O simulation with deadlines.
124-127: Helpful test utility methods.These convenience methods integrate well with the testing framework and help reduce boilerplate in test code.
p2p/stream_manager.go (4)
208-210: Good refactoring of deadline application logic.Replacing the direct deadline application with a centralized helper function improves code organization and maintainability.
243-244: Consistent handling of write deadlines.The same approach is correctly applied to write deadlines, ensuring consistency in the codebase.
266-295: Well-designed error tolerance with proper resource cleanup.The
applyDeadlinefunction correctly handles errors by resetting the stream when needed and includes special handling for mocknet errors. The comments clearly explain the behavior.
297-320: Robust error detection for mocknet streams.The
isMockNetErrorfunction properly identifies errors from mocknet streams that don't support deadlines, with thorough validation of error properties.Consider adding a unit test for this function to ensure it correctly identifies mocknet errors and rejects other types of errors:
func TestIsMockNetError(t *testing.T) { tests := []struct { name string err error expected bool }{ { name: "nil error", err: nil, expected: false, }, { name: "non-OpError", err: errors.New("random error"), expected: false, }, { name: "mocknet error", err: &net.OpError{Op: "set", Net: "pipe", Err: errors.New("deadline not supported")}, expected: true, }, { name: "other OpError", err: &net.OpError{Op: "read", Net: "tcp", Err: errors.New("connection reset")}, expected: false, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { result := isMockNetError(tc.err) require.Equal(t, tc.expected, result) }) } }p2p/stream_manager_test.go (6)
11-11: Good addition of mocks import.The import of the new mock package aligns with the refactoring to use a standardized mock implementation.
29-36: Simplified stream creation with the new mock.Using the new standardized mock stream simplifies the test code and improves clarity.
44-53: Better error simulation with dedicated methods.The error simulation is now more explicit with the dedicated
ErrSetReadDeadlinemethod.
97-103: Improved error assertion pattern.Using
require.Errorand early return for error cases improves readability and makes test failures more obvious.
111-112: Type correction in interface.The function signature correctly specifies the return type as
*mocks.Streaminstead of a genericnetwork.Stream, making the tests more type-safe.
154-162: Good refactoring with helper function.The addition of the
hasStreamhelper function improves readability and simplifies stream presence checking.
Code paths for unit tests and real-world usage diverged. Also global bool toggle could be changed by any LoC in the project |
This PR eliminates global
init()andApplyDeadlineinp2ppackage, improves stream mock,s and converges code paths between unit tests and real code executionCloses #55
Summary by CodeRabbit
New Features
Refactor
Chores