Add some tests for dotc1z.File. #598
Conversation
WalkthroughTests now use test-scoped contexts; saveC1z returns a gRPC InvalidArgument status when the output path is empty; and new tests for loadC1z/saveC1z were added covering error cleanup, custom tmpDir behavior, round-trip save/load, overwrite behavior, and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
ggreer
force-pushed
the
ggreer/file-test
branch
from
0cef482 to
d874dbf
Compare
![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 (1)
pkg/dotc1z/file_test.go (1)
20-20: Consider using more restrictive file permissions in tests.The static analysis tool flags the use of
0644file permissions. While acceptable for test files, using0600would be more secure and prevent unintended access by group/other users.🔎 Apply this pattern to restrict file permissions:
-err := os.WriteFile(invalidFile, []byte("invalid"), 0644) +err := os.WriteFile(invalidFile, []byte("invalid"), 0600)Apply similar changes to all
os.WriteFilecalls in this file (lines 20, 60, 92, 113, 130, 159).Also applies to: 60-60, 92-92, 113-113, 130-130, 159-159
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/dotc1z/c1file_test.go(9 hunks)pkg/dotc1z/file.go(2 hunks)pkg/dotc1z/file_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/dotc1z/file_test.go (1)
pkg/dotc1z/decoder.go (2)
C1ZFileHeader(23-23)NewDecoder(208-241)
pkg/dotc1z/file.go (1)
vendor/google.golang.org/grpc/rpc_util.go (1)
Errorf(973-975)
🪛 GitHub Check: go-lint
pkg/dotc1z/file_test.go
[failure] 159-159:
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 130-130:
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 113-113:
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 92-92:
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 60-60:
G306: Expect WriteFile permissions to be 0600 or less (gosec)
[failure] 20-20:
G306: Expect WriteFile permissions to be 0600 or less (gosec)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (5)
pkg/dotc1z/file.go (2)
14-15: LGTM!The gRPC imports are necessary for the enhanced error handling below and follow standard conventions.
63-65: LGTM!The use of gRPC status error with
InvalidArgumentcode is semantically appropriate for a missing required parameter and provides better error handling for gRPC clients.pkg/dotc1z/c1file_test.go (1)
74-74: LGTM! Excellent test hygiene improvement.Using
t.Context()instead ofcontext.Background()is a best practice that provides automatic cancellation when tests complete or timeout, preventing resource leaks.Note: The PR description states "Uses context.Background() instead of t.Context() in c1file_test.go" but the actual changes use
t.Context()instead ofcontext.Background(), which appears to be inverted in the description.Also applies to: 162-162, 246-246, 266-266, 283-283, 334-334, 374-374, 429-429, 489-489
pkg/dotc1z/file_test.go (2)
14-52: LGTM! Comprehensive test coverage for loadC1z.The tests appropriately cover error handling, temp directory cleanup, and custom tmpDir scenarios with clear assertions.
54-186: LGTM! Excellent comprehensive test coverage for saveC1z.The tests thoroughly cover all critical scenarios including:
- Valid save with round-trip verification
- Error handling for empty output path (with proper gRPC status code verification)
- Error handling for non-existent source db
- File overwrite behavior
- Edge case of empty db file
pkg/dotc1z/file_test.go
Outdated
| // We can't directly check this, but we can verify the dbPath doesn't exist | ||
| if dbPath != "" { | ||
| _, statErr := os.Stat(dbPath) | ||
| require.Error(t, statErr) |
There was a problem hiding this comment.
It should be removed. I realize now that we can't hit that code path since the test would fail when require.Empty(t, dbPath) is called.
Also: - Return a grpc status error for empty output path. - Use context.Background() instead of t.Context() in c1file_test.go.
ggreer
force-pushed
the
ggreer/file-test
branch
from
d874dbf to
a558b83
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/dotc1z/file_test.go (1)
17-28: Consider verifying temp directory cleanup.The comment states "should fail and clean up temp dir," but the test only verifies that
dbPathis empty. Consider explicitly checking thatloadC1zcleaned up any temporary directories it created during the failed decode attempt.💡 Suggested verification approach
After the
loadC1zcall, you could verify cleanup by checking that no unexpected directories remain intmpDir:dbPath, err := loadC1z(invalidFile, tmpDir) require.Error(t, err) require.Empty(t, dbPath) + + // Verify no temp directories were left behind + entries, err := os.ReadDir(tmpDir) + require.NoError(t, err) + // Should only contain the invalidFile we created + require.Len(t, entries, 1)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/dotc1z/c1file_test.go(9 hunks)pkg/dotc1z/file.go(2 hunks)pkg/dotc1z/file_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/dotc1z/file.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/dotc1z/file_test.go (1)
pkg/dotc1z/decoder.go (2)
C1ZFileHeader(23-23)NewDecoder(208-241)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (3)
pkg/dotc1z/c1file_test.go (1)
74-74: LGTM: Improved test lifecycle management.The migration from
context.Background()tot.Context()ensures automatic context cancellation when tests complete or timeout, improving resource cleanup and test reliability.Also applies to: 162-162, 246-246, 266-266, 283-283, 334-334, 374-374, 429-429, 489-489
pkg/dotc1z/file_test.go (2)
47-179: Excellent test coverage for saveC1z.The test suite comprehensively covers:
- Round-trip save/load with content verification
- gRPC status error for empty output path (aligns with PR objectives)
- Error handling for non-existent source files
- Overwrite behavior with size and content verification
- Edge case of empty database files
The use of
NewDecoderfor round-trip verification ensures the saved files are valid and properly formatted.
36-40: No issue with the test—behavior is intentional but undocumented.The test correctly validates that
loadC1zgracefully creates an empty database when given a non-existent file path. This is by design: the function checks if the file exists and has non-zero size (if stat, err := os.Stat(filePath); err == nil && stat.Size() != 0), and only processes the file if both conditions are met. For missing or empty files, it creates an empty db file and returns the path with no error.While the behavior is intentional, consider adding a doc comment to
loadC1zto clarify that it returns a successfully-created database path (possibly empty) rather than an error when the input file doesn't exist.
Also:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.