Enhance error handling and add error tests in graph package #127
Conversation
WalkthroughThe pull request introduces enhancements to error handling in the caching functionality within the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
pkg/graph/mockGraph.go (1)
22-41: LGTM! Consider adding documentation for error injection fields.The error injection fields are well-structured and provide comprehensive coverage for testing error scenarios. However, adding documentation comments would help other developers understand how to use these fields effectively.
Consider adding a doc comment like:
// Error injection fields - set these fields to simulate error scenarios during testing. // Each field corresponds to a specific operation that can fail.pkg/graph/cache.go (1)
Line range hint
13-139: Consider defining custom error types for better error handlingWhile the current error wrapping implementation is good, consider introducing custom error types for better error handling and testing. This would allow:
- Type-based error handling in calling code
- Easier testing of specific error conditions
- Consistent error messages across the package
Example implementation:
// Define error types type CacheError struct { Op string Err error } func (e *CacheError) Error() string { return fmt.Sprintf("cache %s error: %v", e.Op, e.Err) } func (e *CacheError) Unwrap() error { return e.Err } // Usage example if err != nil { return &CacheError{Op: "build_cache", Err: err} }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-33: pkg/graph/cache.go#L33
Added line #L33 was not covered by tests
[warning] 38-38: pkg/graph/cache.go#L38
Added line #L38 was not covered by tests
[warning] 43-43: pkg/graph/cache.go#L43
Added line #L43 was not covered by testspkg/graph/cache_test.go (2)
378-410: Consider enhancing test coverage and organization.The test table structure is well-organized, but consider these improvements:
- Group related test cases (e.g., read operations vs. write operations)
- Add test cases for concurrent operations
- Make error messages more descriptive by including operation context
Example grouping and enhanced error messages:
tests := []struct { name string setupMock func(*MockStorage) wantErr string }{ + // Read Operations { name: "ToBeCached error", setupMock: func(m *MockStorage) { m.ToBeCachedErr = fmt.Errorf("ToBeCached error") }, - wantErr: "error getting uncached nodes: ToBeCached error", + wantErr: "error getting uncached nodes: failed to retrieve nodes pending cache: ToBeCached error", }, { name: "GetAllKeys error", setupMock: func(m *MockStorage) { m.GetAllKeysErr = fmt.Errorf("GetAllKeys error") }, - wantErr: "error getting keys: GetAllKeys error", + wantErr: "error getting keys: failed to retrieve all storage keys: GetAllKeys error", }, + // Write Operations { name: "SaveCaches error", setupMock: func(m *MockStorage) { m.SaveCachesErr = fmt.Errorf("SaveCaches error") }, - wantErr: "error saving caches: SaveCaches error", + wantErr: "error saving caches: failed to persist cache data: SaveCaches error", }, + // Concurrent Operations + { + name: "Concurrent cache updates", + setupMock: func(m *MockStorage) { + m.SaveCachesErr = fmt.Errorf("concurrent modification detected") + }, + wantErr: "error saving caches: concurrent modification detected", + }, }
414-428: Enhance test setup with cleanup and helper functions.The test setup creates a basic circular dependency, but consider these improvements:
- Add cleanup using
t.Cleanup()to ensure proper teardown- Extract node creation and dependency setup into helper functions
- Vary the node setup based on test case requirements
Example refactor:
+func setupTestNodes(t *testing.T, storage Storage, count int) []*Node { + nodes := make([]*Node, count) + for i := 0; i < count; i++ { + var err error + nodes[i], err = AddNode(storage, fmt.Sprintf("type %d", i+1), + fmt.Sprintf("metadata %d", i), fmt.Sprintf("name %d", i+1)) + assert.NoError(t, err) + } + return nodes +} + +func setupCircularDependency(t *testing.T, storage Storage, nodes []*Node) { + for i := 0; i < len(nodes); i++ { + err := nodes[i].SetDependency(storage, nodes[(i+1)%len(nodes)]) + assert.NoError(t, err) + } +} t.Run(tt.name, func(t *testing.T) { mockStorage := NewMockStorage() - nodes := make([]*Node, 3) + t.Cleanup(func() { + // Add cleanup logic here + mockStorage.Clear() + }) - // Create nodes - for i := 0; i < 3; i++ { - var err error - nodes[i], err = AddNode(mockStorage, fmt.Sprintf("type %d", i+1), - fmt.Sprintf("metadata %d", i), fmt.Sprintf("name %d", i+1)) - assert.NoError(t, err) - } + nodes := setupTestNodes(t, mockStorage, 3) + setupCircularDependency(t, mockStorage, nodes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/graph/cache.go(5 hunks)pkg/graph/cache_test.go(1 hunks)pkg/graph/mockGraph.go(10 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/graph/cache.go
[warning] 33-33: pkg/graph/cache.go#L33
Added line #L33 was not covered by tests
[warning] 38-38: pkg/graph/cache.go#L38
Added line #L38 was not covered by tests
[warning] 43-43: pkg/graph/cache.go#L43
Added line #L43 was not covered by tests
[warning] 52-52: pkg/graph/cache.go#L52
Added line #L52 was not covered by tests
[warning] 139-139: pkg/graph/cache.go#L139
Added line #L139 was not covered by tests
pkg/graph/mockGraph.go
[warning] 57-58: pkg/graph/mockGraph.go#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 69-70: pkg/graph/mockGraph.go#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 81-83: pkg/graph/mockGraph.go#L81-L83
Added lines #L81 - L83 were not covered by tests
[warning] 117-118: pkg/graph/mockGraph.go#L117-L118
Added lines #L117 - L118 were not covered by tests
[warning] 138-140: pkg/graph/mockGraph.go#L138-L140
Added lines #L138 - L140 were not covered by tests
[warning] 155-156: pkg/graph/mockGraph.go#L155-L156
Added lines #L155 - L156 were not covered by tests
[warning] 160-160: pkg/graph/mockGraph.go#L160
Added line #L160 was not covered by tests
[warning] 167-168: pkg/graph/mockGraph.go#L167-L168
Added lines #L167 - L168 were not covered by tests
[warning] 177-178: pkg/graph/mockGraph.go#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 182-182: pkg/graph/mockGraph.go#L182
Added line #L182 was not covered by tests
[warning] 213-214: pkg/graph/mockGraph.go#L213-L214
Added lines #L213 - L214 were not covered by tests
[warning] 223-224: pkg/graph/mockGraph.go#L223-L224
Added lines #L223 - L224 were not covered by tests
[warning] 241-243: pkg/graph/mockGraph.go#L241-L243
Added lines #L241 - L243 were not covered by tests
[warning] 258-261: pkg/graph/mockGraph.go#L258-L261
Added lines #L258 - L261 were not covered by tests
[warning] 264-266: pkg/graph/mockGraph.go#L264-L266
Added lines #L264 - L266 were not covered by tests
[warning] 268-268: pkg/graph/mockGraph.go#L268
Added line #L268 was not covered by tests
[warning] 273-275: pkg/graph/mockGraph.go#L273-L275
Added lines #L273 - L275 were not covered by tests
[warning] 278-283: pkg/graph/mockGraph.go#L278-L283
Added lines #L278 - L283 were not covered by tests
🔇 Additional comments (6)
pkg/graph/mockGraph.go (2)
49-52: LGTM! Constructor properly initializes all fields.
The initialization of the new db field is correct and consistent with other map initializations in the constructor.
264-268: LGTM! Improved key handling in custom data methods.
The custom data methods now use a consistent key format with proper initialization and error handling. The use of fullKey makes the code more maintainable and reduces the chance of key format inconsistencies.
Also applies to: 278-283
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 264-266: pkg/graph/mockGraph.go#L264-L266
Added lines #L264 - L266 were not covered by tests
[warning] 268-268: pkg/graph/mockGraph.go#L268
Added line #L268 was not covered by tests
pkg/graph/cache.go (4)
13-13: LGTM: Improved error context for uncached nodes retrieval
The error wrapping provides clear context about the operation that failed while preserving the original error chain.
43-43: Enhance error handling test coverage for key operations
The error wrapping provides good context, especially for the key conversion error which includes the problematic value. However, these scenarios need test coverage.
Let's check for existing key-related error tests:
#!/bin/bash
# Search for key-related error test cases
rg -A 5 "TestCache.*key.*error" Would you like me to help generate test cases for:
- GetAllKeysAndValues failure scenarios
- Invalid key conversion scenarios
Also applies to: 52-52
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-43: pkg/graph/cache.go#L43
Added line #L43 was not covered by tests
Line range hint 1-270: Improve overall error handling test coverage
While the error handling improvements are well-implemented, several critical error paths lack test coverage. Consider adding a comprehensive error test suite that covers:
- All error scenarios in the Cache function
- Edge cases in cycle detection
- Key conversion failures
- Cache operation failures
Let's check the overall test coverage for error scenarios:
#!/bin/bash
# Search for all error-related test cases
rg -A 5 "TestCache.*error"
# Check for error injection in mock implementations
rg -A 5 "Mock.*error" Would you like me to help design a comprehensive test suite that covers all these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-33: pkg/graph/cache.go#L33
Added line #L33 was not covered by tests
[warning] 38-38: pkg/graph/cache.go#L38
Added line #L38 was not covered by tests
[warning] 43-43: pkg/graph/cache.go#L43
Added line #L43 was not covered by tests
33-33: Add test coverage for buildCache error scenarios
While the error wrapping implementation is good, these error paths aren't covered by tests according to the coverage report.
Let's verify the test coverage:
Would you like me to help generate test cases for these error scenarios? We should include cases where:
- Building cached children fails
- Building cached parents fails
Also applies to: 38-38
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-33: pkg/graph/cache.go#L33
Added line #L33 was not covered by tests
This commit improves error handling throughout the graph package by: 1. Adding detailed error wrapping using fmt.Errorf with %w verb in the Cache function to provide better error context and maintain error chain 2. Adding comprehensive error injection capabilities to MockStorage by: - Adding error fields for each operation - Implementing error checks in all mock methods - Improving error handling in mock methods 3. Adding TestCacheErrors test suite that verifies error handling for: - ToBeCached errors - GetAllKeys errors - GetNodes errors - SaveCaches errors - ClearCacheStack errors 4. Improving MockStorage implementation: - Better initialization of internal maps - More consistent error handling patterns - Cleaner key construction for custom data - Added existence checks for data retrieval These changes improve debuggability by providing more context in error messages and increase test coverage around error conditions. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan
force-pushed
the
naveen/addtests/cache
branch
from
b5e8fe2
to
45efd2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
pkg/graph/cache_test.go (2)
385-410: Consider adding more error scenarios.While the current test cases cover basic error conditions, consider adding:
- Concurrent operation errors
- Multiple simultaneous errors
- Edge cases like empty node list or invalid node IDs
Example additional test case:
{ name: "ClearCacheStack error", setupMock: func(m *MockStorage) { m.ClearCacheStackErr = fmt.Errorf("ClearCacheStack error") }, wantErr: "ClearCacheStack error", }, + { + name: "Multiple simultaneous errors", + setupMock: func(m *MockStorage) { + m.GetNodesErr = fmt.Errorf("GetNodes error") + m.SaveCachesErr = fmt.Errorf("SaveCaches error") + }, + wantErr: "error getting all nodes: GetNodes error", + }, + { + name: "Empty node list error", + setupMock: func(m *MockStorage) { + m.nodes = make(map[uint32]*Node) + }, + wantErr: "no nodes to cache", + },
414-428: Extract test setup into a helper function.The circular dependency setup logic is repeated across multiple test functions. Consider extracting it into a helper function for better maintainability and reusability.
+func setupCircularDependencyGraph(t *testing.T, storage Storage, nodeCount int) []*Node { + nodes := make([]*Node, nodeCount) + + // Create nodes + for i := 0; i < nodeCount; i++ { + var err error + nodes[i], err = AddNode(storage, fmt.Sprintf("type %d", i+1), + fmt.Sprintf("metadata %d", i), fmt.Sprintf("name %d", i+1)) + assert.NoError(t, err) + } + + // Create circle + for i := 0; i < nodeCount; i++ { + err := nodes[i].SetDependency(storage, nodes[(i+1)%nodeCount]) + assert.NoError(t, err) + } + + return nodes +}Then use it in the test:
- nodes := make([]*Node, 3) - - // Create nodes - for i := 0; i < 3; i++ { - var err error - nodes[i], err = AddNode(mockStorage, fmt.Sprintf("type %d", i+1), fmt.Sprintf("metadata %d", i), fmt.Sprintf("name %d", i+1)) - assert.NoError(t, err) - } - - // Create circle: node0 -> node1 -> node2 -> node0 - for i := 0; i < 3; i++ { - err := nodes[i].SetDependency(mockStorage, nodes[(i+1)%3]) - assert.NoError(t, err) - } + nodes := setupCircularDependencyGraph(t, mockStorage, 3)pkg/graph/mockGraph.go (2)
22-22: Consider renaming thedbfield for clarityThe field
dbin theMockStoragestruct is quite generic. Renaming it to something more descriptive likecustomDataStorageorcustomDatawould enhance code readability and maintainability.
25-41: Simplify error injection with a map or structured approachThe numerous error injection fields added to the
MockStoragestruct could become unwieldy as the codebase grows. Consider using a map or a struct to manage error injections dynamically. This approach can improve scalability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
codecov.yml(1 hunks)pkg/graph/cache.go(5 hunks)pkg/graph/cache_test.go(1 hunks)pkg/graph/mockGraph.go(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- codecov.yml
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/graph/cache.go
[warning] 33-33: pkg/graph/cache.go#L33
Added line #L33 was not covered by tests
[warning] 38-38: pkg/graph/cache.go#L38
Added line #L38 was not covered by tests
[warning] 43-43: pkg/graph/cache.go#L43
Added line #L43 was not covered by tests
[warning] 52-52: pkg/graph/cache.go#L52
Added line #L52 was not covered by tests
[warning] 139-139: pkg/graph/cache.go#L139
Added line #L139 was not covered by tests
🔇 Additional comments (4)
pkg/graph/cache.go (3)
13-13: Well-structured error handling improvements!
The error wrapping changes provide clear context about which operation failed while maintaining the error chain using fmt.Errorf with the %w verb. The messages are descriptive and follow a consistent pattern throughout the function.
Also applies to: 33-33, 38-38, 43-43, 52-52, 68-68
13-13: Add test coverage for error scenarios
The static analysis indicates that these error paths aren't covered by tests. While the PR objectives mention adding error tests, these specific error scenarios should be tested:
- ToBeCached error (line 13)
- Building cached children/parents errors (lines 33, 38)
- Getting cached keys and values error (line 43)
- Key conversion error (line 52)
- Cache saving error (line 68)
Let's verify the test coverage:
Would you like me to help generate test cases for these error scenarios using the MockStorage error injection capabilities?
Also applies to: 33-33, 38-38, 43-43, 52-52, 68-68
139-139: Add test coverage for cycle binding failures
While the error wrapping is well-implemented, this critical path in cycle detection needs test coverage. The error could occur in several scenarios:
- When binding keys for cycles fails
- When setting values in cache/children/parents maps fails
- When converting keys to integers fails
Let's analyze the existing test coverage for cycle binding:
Would you like me to help generate test cases that:
- Simulate binding failures in addCyclesToBindMap
- Test error propagation through the cycle detection workflow
- Cover edge cases in key conversion and binding?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-139: pkg/graph/cache.go#L139
Added line #L139 was not covered by tests
pkg/graph/cache_test.go (1)
378-384: LGTM! Well-structured test function.
The test function follows Go best practices with a clear table-driven test pattern and descriptive field names.
This commit improves error handling throughout the graph package by:
Adding detailed error wrapping using fmt.Errorf with %w verb in the Cache function to provide better error context and maintain error chain
Adding comprehensive error injection capabilities to MockStorage by:
Adding TestCacheErrors test suite that verifies error handling for:
Improving MockStorage implementation:
These changes improve debuggability by providing more context in error messages and increase test coverage around error conditions.
Summary by CodeRabbit
New Features
Bug Fixes
These updates enhance the overall robustness and clarity of the caching system, making it easier to diagnose issues and ensuring reliable performance.