Introduce testify #33
Conversation
WalkthroughTests across multiple files were refactored to use Testify (assert/require) instead of manual checks. A new module dependency on Testify and its transitive deps was added in go.mod. One new test case for empty CSV input was introduced. No production code or public API changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🔍 PR Review: Introduce testifyThis PR introduces the ✅ Positive AspectsCode Quality & Best Practices:
Technical Implementation:
📋 Alignment with CLAUDE.md Guidelines✅ Follows project standards:
🔍 Code AnalysisBefore vs After Examples: // Before - verbose and error-prone
if builder.paths[0] != "test.csv" {
t.Errorf("paths[0] = %s, want test.csv", builder.paths[0])
}
// After - clear and concise
assert.Equal(t, "test.csv", builder.paths[0], "first path should be test.csv")Key Improvements:
🚀 Performance Considerations
🔐 Security Considerations
🧪 Test CoverageStrengths:
Considerations:
📝 Minor Suggestions
✅ RecommendationAPPROVE - This is an excellent refactoring that:
The changes are low-risk, high-value improvements that align perfectly with the project's commitment to readable test code. Great work! 🎉 Generated with Claude Code - following filesql project guidelines from CLAUDE.md |
Code Metrics Report
Details | | main (cac524a) | #33 (fed95bd) | +/- |
|---------------------|----------------|---------------|-------|
- | Coverage | 84.0% | 83.7% | -0.3% |
| Files | 9 | 9 | 0 |
| Lines | 1663 | 1663 | 0 |
- | Covered | 1397 | 1393 | -4 |
- | Test Execution Time | 52s | 1m53s | +1m1s |Reported by octocov |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
table_test.go (1)
139-146: Parallel subtests capture the loop variable; add tt := tt to avoid flakiness.
t.Parallel()inside the loop without rebindingttcan race and read the wrong case values.Apply this fix:
for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() result := tableFromFilePath(tt.filePath) assert.Equal(t, tt.expected, result, "tableFromFilePath failed for %s", tt.filePath) }) }types_test.go (1)
67-74: Rebind tt when using parallel subtests in loops.
t.Parallel()within theset.Runloops needstt := ttto prevent capturing the final iterator value.Apply in both loops:
for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() result := tt.header1.equal(tt.header2) assert.Equal(t, tt.expected, result, "Header equality check failed") }) }for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() result := tt.record1.equal(tt.record2) assert.Equal(t, tt.expected, result, "Record equality check failed") }) }Also applies to: 135-142
builder_test.go (1)
373-375: Avoid Go 1.22+ only syntax: replacefor i := range 10000
rangeover an int requires Go 1.22+. Use a classic for-loop to keep broader compatibility.Apply:
- for i := range 10000 { // Full test on GitHub Actions + for i := 0; i < 10000; i++ { // Full test on GitHub Actions fmt.Fprintf(&data, "%d,name_%d,%d\n", i, i, i*10) }stream_test.go (1)
93-113: Make the “gzip compressed CSV” test actually exercise decompressionPreviously it used uncompressed data with
FileTypeCSV. Compress the payload and setFileTypeCSVGZ.t.Run("gzip compressed CSV", func(t *testing.T) { t.Parallel() - // Create gzip compressed CSV data - originalData := "name,age\nAlice,30\nBob,25\n" - var buf bytes.Buffer - - // For this test, we'll use uncompressed data but specify the compressed type - // to test the decompression logic path - reader := strings.NewReader(originalData) - - // Note: This will fail because the data is not actually gzip compressed - // but the test demonstrates the compression handling logic - parser := newStreamingParser(FileTypeCSV, "users", 1024) // Use uncompressed for now - table, err := parser.parseFromReader(reader) + // Create gzip compressed CSV data + originalData := "name,age\nAlice,30\nBob,25\n" + var gzBuf bytes.Buffer + zw := gzip.NewWriter(&gzBuf) + _, _ = zw.Write([]byte(originalData)) + _ = zw.Close() + + reader := bytes.NewReader(gzBuf.Bytes()) + parser := newStreamingParser(FileTypeCSVGZ, "users", 1024) + table, err := parser.parseFromReader(reader) require.NoError(t, err, "ParseFromReader() failed") records := table.getRecords() assert.Len(t, records, 2, "Records length mismatch") - - _ = buf // Prevent unused variable warning })file_test.go (6)
91-99: Fix parallel subtest capture: close over a copy of loop variableWithout rebind,
t.Parallel()subtests can read the wrongtt.- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() file := newFile(tt.path) assert.Equal(t, tt.expected, file.getFileType(), "File type mismatch") assert.Equal(t, tt.path, file.getPath(), "File path mismatch") }) }
137-144: Same loop var capture fix forTestFile_IsCompressed- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() file := newFile(tt.path) assert.Equal(t, tt.expected, file.isCompressed(), "Compression check failed") }) }
200-210: Same fix forTestFile_CompressionTypes- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() file := newFile(tt.path) assert.Equal(t, tt.isGZ, file.isGZ(), "IsGZ() check failed") assert.Equal(t, tt.isBZ2, file.isBZ2(), "IsBZ2() check failed") assert.Equal(t, tt.isXZ, file.isXZ(), "IsXZ() check failed") assert.Equal(t, tt.isZSTD, file.isZSTD(), "IsZSTD() check failed") }) }
401-409: Same fix forTestTableFromFilePath- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() result := tableFromFilePath(tt.filePath) assert.Equal(t, tt.expected, result, "tableFromFilePath failed") }) }
753-760: Same fix forTestFileTypeExtensionsubtests
ttmust be rebound before starting parallel subtests.- for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { t.Parallel() if got := tt.fileType.extension(); got != tt.expected { t.Errorf("FileType.extension() = %v, want %v", got, tt.expected) } }) }
1001-1023: Same fix forTestCompressionDetection_EdgeCasesParallel subtests need loop variable capture.
- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { t.Parallel() file := newFile(tc.filePath) ... }) }
🧹 Nitpick comments (10)
go.mod (1)
8-8: Testify dependency addition looks good; run tidy to prune and normalize requires.Adding testify and its common indirects is appropriate for the assertion refactor. Recommend running
go mod tidyto consolidate multiple require blocks and drop any now-unreferenced indirects.Also applies to: 18-18, 31-31, 51-51
types_test.go (1)
158-163: Optional: parallelize these subtests safely.You can add
t.Parallel()inside this loop too for speed; if you do, also addtt := ttbeforet.Run.column_inference_test.go (3)
110-114: Optional: enable parallel subtests with loop var rebinding.Add
tt := ttthent.Parallel()in thet.Runbody to speed this table-driven test.for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { + t.Parallel() result := inferColumnType(tt.values) assert.Equal(t, tt.expected, result, "inferColumnType failed for values: %v", tt.values) }) }
121-146: Subtests could run in parallel.These three
t.Runblocks don’t share mutable state; considert.Parallel()inside each to reduce wall time.Also applies to: 147-158, 160-183
224-228: Optional: parallelize isDatetime table tests (with tt rebinding).Safe speed-up if you add
tt := ttbeforet.Runandt.Parallel()inside.builder_test.go (2)
241-246: Test name vs. setup mismatch: use a compressed file typeSubtest says “compressed type specification” but uses
FileTypeCSV. Use a compressed type to reflect intent.- builder := NewBuilder().AddReader(reader, "logs", FileTypeCSV) + builder := NewBuilder().AddReader(reader, "logs", FileTypeCSVGZ)
682-689: Simplify “if err != nil { require.NoError(...) }” patternsThese branches are redundant;
require.NoErroralready fails the test. Direct calls are clearer.Example transformation:
- if err != nil { - require.NoError(t, err, "operation should succeed") - } + require.NoError(t, err, "operation should succeed")Applies to the marked ranges and similar occurrences in this file.
Also applies to: 746-754, 799-807, 831-836, 869-879, 895-905, 934-941, 1491-1495, 1534-1538, 1698-1704, 1739-1743, 1779-1783, 1820-1824, 1856-1859
stream_test.go (2)
44-49: Use testify for error assertion on empty CSVMake the intention explicit and fail fast.
- _, err := parser.parseFromReader(reader) - if err == nil { - t.Error("ParseFromReader() should fail for empty data") - } + _, err := parser.parseFromReader(reader) + require.Error(t, err, "ParseFromReader() should fail for empty data")
3-13: Add gzip import for compressed CSV testimport ( "bytes" + "compress/gzip" "os" "path/filepath" "strings" "testing"file_test.go (1)
515-522: Comment label is misleading (xlsx is supported here)The section header says “Unsupported formats” but includes
test.xlsxwithtrue. Clarify the comment.- // Unsupported formats + // Other formats and edge cases (xlsx supported; txt/json/xml unsupported)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
builder_test.go(79 hunks)column_inference_test.go(6 hunks)dump_options_test.go(11 hunks)file_test.go(17 hunks)go.mod(4 hunks)stream_test.go(7 hunks)table_test.go(6 hunks)types_test.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursorrules)
**/*.go: Do not use global variables; manage state via function arguments and return values
Follow Golang coding rules per Effective Go
Public (exported) functions, variables, and struct fields must have documentation comments following go doc rules
Remove duplicate code after completing work
Write code comments in English
Provide user-friendly documentation comments with detailed explanations and example code for public functions
Ensure cross-platform compatibility (e.g., use filepath.Join instead of manual path concatenation; avoid hardcoded "\n")
**/*.go: Write code comments in English
Provide detailed, user-friendly documentation comments (godoc) for all public functions so usage is clear
Do not use global variables; manage state via function arguments/returns
Follow Effective Go (Golang coding rules)
Add comments for all exported (public) functions, variables, and struct fields following godoc rules
Remove duplicate code after implementation
Ensure cross-platform compatibility: use filepath.Join for paths and avoid hardcoded "\n" line breaks
Copilot: Ensure generated code follows Effective Go
Copilot: Include proper godoc comments for all public APIs
Copilot: Use filepath.Join() for path operations for cross-platform compatibility
**/*.go: Write code comments in English
Provide user-friendly documentation comments with detailed explanations and examples for public functions
Do not use global variables; manage state via function inputs/outputs
Follow Golang coding rules per Effective Go
Write comments for all public functions, variables, and struct fields per go doc rules
Remove duplicate code after completing work
Ensure cross-platform compatibility (e.g., use filepath.Join instead of manual path concatenation; avoid hardcoded "\n")
Files:
builder_test.gotypes_test.gotable_test.gocolumn_inference_test.godump_options_test.gofile_test.gostream_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursorrules)
**/*_test.go: Keep test code readable; avoid over-DRY and favor clarity
Use t.Run to structure tests and make inputs/expected outputs explicit
Use t.Parallel in tests where possible
**/*_test.go: Keep test code readable; avoid over-DRY patterns that obscure intent
Structure tests with t.Run subtests and make inputs/expected outputs explicit
Use t.Parallel() to run tests in parallel when possible
Copilot: Suggest parallel test execution with t.Parallel() where appropriate
**/*_test.go: Keep test code readable; avoid excessive DRY
Use t.Run with explicit inputs/expected outputs to clarify test intent
Use t.Parallel() to run tests in parallel whenever possible
Files:
builder_test.gotypes_test.gotable_test.gocolumn_inference_test.godump_options_test.gofile_test.gostream_test.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: nao1215/filesql#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-31T12:53:10.431Z
Learning: Applies to **/*_test.go : Structure tests with t.Run subtests and make inputs/expected outputs explicit
Learnt from: CR
PR: nao1215/filesql#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T12:53:42.359Z
Learning: Applies to **/*_test.go : Use t.Run with explicit inputs/expected outputs to clarify test intent
📚 Learning: 2025-08-31T12:53:10.431Z
Learnt from: CR
PR: nao1215/filesql#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-31T12:53:10.431Z
Learning: Applies to **/*_test.go : Structure tests with t.Run subtests and make inputs/expected outputs explicit
Applied to files:
builder_test.gotypes_test.gotable_test.gocolumn_inference_test.godump_options_test.gofile_test.gostream_test.go
📚 Learning: 2025-08-31T12:53:42.359Z
Learnt from: CR
PR: nao1215/filesql#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T12:53:42.359Z
Learning: Applies to **/*_test.go : Use t.Run with explicit inputs/expected outputs to clarify test intent
Applied to files:
builder_test.gotypes_test.gotable_test.gocolumn_inference_test.godump_options_test.gofile_test.go
📚 Learning: 2025-08-31T12:52:28.036Z
Learnt from: CR
PR: nao1215/filesql#0
File: .cursorrules:0-0
Timestamp: 2025-08-31T12:52:28.036Z
Learning: Applies to **/*_test.go : Use t.Run to structure tests and make inputs/expected outputs explicit
Applied to files:
builder_test.gotypes_test.gotable_test.gocolumn_inference_test.godump_options_test.gofile_test.go
📚 Learning: 2025-08-31T12:52:28.036Z
Learnt from: CR
PR: nao1215/filesql#0
File: .cursorrules:0-0
Timestamp: 2025-08-31T12:52:28.036Z
Learning: Applies to **/*_test.go : Use t.Parallel in tests where possible
Applied to files:
builder_test.gotypes_test.gotable_test.godump_options_test.gofile_test.go
📚 Learning: 2025-08-31T12:53:10.431Z
Learnt from: CR
PR: nao1215/filesql#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-31T12:53:10.431Z
Learning: Applies to **/*_test.go : Copilot: Suggest parallel test execution with t.Parallel() where appropriate
Applied to files:
builder_test.gotypes_test.gotable_test.godump_options_test.gofile_test.go
📚 Learning: 2025-08-31T12:53:10.431Z
Learnt from: CR
PR: nao1215/filesql#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-31T12:53:10.431Z
Learning: Applies to **/*_test.go : Use t.Parallel() to run tests in parallel when possible
Applied to files:
builder_test.godump_options_test.gofile_test.go
📚 Learning: 2025-08-31T12:53:42.359Z
Learnt from: CR
PR: nao1215/filesql#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T12:53:42.359Z
Learning: Applies to **/*_test.go : Use t.Parallel() to run tests in parallel whenever possible
Applied to files:
builder_test.godump_options_test.gofile_test.go
📚 Learning: 2025-08-31T12:53:10.431Z
Learnt from: CR
PR: nao1215/filesql#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-31T12:53:10.431Z
Learning: Applies to **/*_test.go : Keep test code readable; avoid over-DRY patterns that obscure intent
Applied to files:
table_test.gofile_test.go
📚 Learning: 2025-08-31T12:53:42.359Z
Learnt from: CR
PR: nao1215/filesql#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T12:53:42.359Z
Learning: Applies to **/*_test.go : Keep test code readable; avoid excessive DRY
Applied to files:
table_test.gofile_test.go
📚 Learning: 2025-08-31T12:52:28.036Z
Learnt from: CR
PR: nao1215/filesql#0
File: .cursorrules:0-0
Timestamp: 2025-08-31T12:52:28.036Z
Learning: Applies to **/*_test.go : Keep test code readable; avoid over-DRY and favor clarity
Applied to files:
table_test.go
⏰ 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). (5)
- GitHub Check: Unit test (linux) (ubuntu-latest, 1.24)
- GitHub Check: Unit test (linux) (macos-latest, 1.24)
- GitHub Check: Unit test (linux) (windows-latest, 1.24)
- GitHub Check: claude-review
- GitHub Check: Unit test (linux) (ubuntu-latest)
🔇 Additional comments (8)
table_test.go (3)
7-7: Adoption of Testify assert is appropriate.Consistent with the PR goal; import scope is limited to tests.
24-31: Assertions read clearly and preserve intent.Straightforward migration from manual checks; no behavior change.
50-51: Equality assertions LGTM.Good coverage of name, header, count, and values.
Also applies to: 56-57, 64-65, 74-75, 85-86
dump_options_test.go (2)
6-6: Testify import aligns with refactor goal.Scope restricted to tests; fine.
190-192: Direct option assertions look good.Nice clarity on defaults, immutability, and chaining behavior.
Also applies to: 201-208, 217-224, 312-318
types_test.go (2)
6-6: Testify import LGTM.Matches the test style elsewhere.
18-18: Element-wise and length assertions are clear.Keeps intent explicit without over-DRYing.
Also applies to: 21-21, 86-86, 89-89
column_inference_test.go (1)
6-8: Good use of assert/require mix.
require.Lenfor structure checks andassertfor field equality is the right split.
| rows, err := db.QueryContext(ctx, "SELECT * FROM users") | ||
| assert.NoError(t, err, "Query should succeed") | ||
| defer rows.Close() | ||
| assert.NoError(t, rows.Err(), "Rows should not have errors") | ||
| _ = db.Close() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use require before deferring on rows to prevent nil deref on failure
If the query fails, rows may be nil and defer rows.Close() can panic.
- rows, err := db.QueryContext(ctx, "SELECT * FROM users")
- assert.NoError(t, err, "Query should succeed")
- defer rows.Close()
+ rows, err := db.QueryContext(ctx, "SELECT * FROM users")
+ require.NoError(t, err, "Query should succeed")
+ defer rows.Close()
assert.NoError(t, rows.Err(), "Rows should not have errors")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rows, err := db.QueryContext(ctx, "SELECT * FROM users") | |
| assert.NoError(t, err, "Query should succeed") | |
| defer rows.Close() | |
| assert.NoError(t, rows.Err(), "Rows should not have errors") | |
| _ = db.Close() | |
| rows, err := db.QueryContext(ctx, "SELECT * FROM users") | |
| require.NoError(t, err, "Query should succeed") | |
| defer rows.Close() | |
| assert.NoError(t, rows.Err(), "Rows should not have errors") | |
| _ = db.Close() |
🤖 Prompt for AI Agents
In builder_test.go around lines 416 to 420, the test defers rows.Close() before
asserting the query succeeded which can panic if rows is nil; change the order
to assert/require that err is nil (use require.NoError or require.NoErrorf to
stop the test on failure) immediately after db.QueryContext, then defer
rows.Close() only after the require passes (and keep checking rows.Err() after
iteration), ensuring any db.Close() remains after rows.Close() handling.
| for _, table := range []string{"orders", "products"} { | ||
| rows, err := db.QueryContext(ctx, "SELECT * FROM "+table) // #nosec G202 -- table name is safe | ||
| assert.NoError(t, err, "Query %s should succeed", table) | ||
| assert.NoError(t, rows.Err(), "Rows should not have errors for %s", table) | ||
| _ = rows.Close() // Close immediately in the loop | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen query assertion to avoid nil deref in loop
Same issue inside the loop; fail fast on query error.
- rows, err := db.QueryContext(ctx, "SELECT * FROM "+table) // #nosec G202 -- table name is safe
- assert.NoError(t, err, "Query %s should succeed", table)
+ rows, err := db.QueryContext(ctx, "SELECT * FROM "+table) // #nosec G202 -- table name is safe
+ require.NoError(t, err, "Query %s should succeed", table)
assert.NoError(t, rows.Err(), "Rows should not have errors for %s", table)
_ = rows.Close() // Close immediately in the loop📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, table := range []string{"orders", "products"} { | |
| rows, err := db.QueryContext(ctx, "SELECT * FROM "+table) // #nosec G202 -- table name is safe | |
| assert.NoError(t, err, "Query %s should succeed", table) | |
| assert.NoError(t, rows.Err(), "Rows should not have errors for %s", table) | |
| _ = rows.Close() // Close immediately in the loop | |
| } | |
| for _, table := range []string{"orders", "products"} { | |
| rows, err := db.QueryContext(ctx, "SELECT * FROM "+table) // #nosec G202 -- table name is safe | |
| require.NoError(t, err, "Query %s should succeed", table) | |
| assert.NoError(t, rows.Err(), "Rows should not have errors for %s", table) | |
| _ = rows.Close() // Close immediately in the loop | |
| } |
🤖 Prompt for AI Agents
In builder_test.go around lines 448 to 453, the loop uses assert.NoError for the
db.QueryContext call which doesn't stop execution on failure and can lead to a
nil rows dereference; change the check to fail fast (either replace
assert.NoError with require.NoError or add an immediate if err != nil {
t.Fatalf(...)} after the query) so the test aborts when QueryContext returns an
error before calling rows.Err() or rows.Close().
| rows, err := db.QueryContext(ctx, "SELECT name FROM sqlite_master WHERE type='table'") | ||
| assert.NoError(t, err, "should be able to query database") | ||
| defer rows.Close() | ||
| assert.NoError(t, rows.Err(), "rows should not have errors") | ||
|
|
||
| _ = db.Close() | ||
| // Clean up temp files |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use require before deferring rows.Close()
Avoid potential nil deref when the query fails.
- rows, err := db.QueryContext(ctx, "SELECT name FROM sqlite_master WHERE type='table'")
- assert.NoError(t, err, "should be able to query database")
- defer rows.Close()
+ rows, err := db.QueryContext(ctx, "SELECT name FROM sqlite_master WHERE type='table'")
+ require.NoError(t, err, "should be able to query database")
+ defer rows.Close()
assert.NoError(t, rows.Err(), "rows should not have errors")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rows, err := db.QueryContext(ctx, "SELECT name FROM sqlite_master WHERE type='table'") | |
| assert.NoError(t, err, "should be able to query database") | |
| defer rows.Close() | |
| assert.NoError(t, rows.Err(), "rows should not have errors") | |
| _ = db.Close() | |
| // Clean up temp files | |
| rows, err := db.QueryContext(ctx, "SELECT name FROM sqlite_master WHERE type='table'") | |
| require.NoError(t, err, "should be able to query database") | |
| defer rows.Close() | |
| assert.NoError(t, rows.Err(), "rows should not have errors") | |
| _ = db.Close() | |
| // Clean up temp files |
🤖 Prompt for AI Agents
In builder_test.go around lines 600 to 606, the test calls db.QueryContext and
immediately defers rows.Close() before asserting no error, which risks a nil
dereference if the query failed; change to assert the query returned no error
using require.NoError(t, err) (or if using testify require) before deferring
rows.Close(), then check rows.Err() with require.NoError as appropriate so defer
is only set when rows is non-nil.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| if got := tt.format.String(); got != tt.want { | ||
| t.Errorf("OutputFormat.String() = %v, want %v", got, tt.want) | ||
| } | ||
| got := tt.format.String() | ||
| assert.Equal(t, tt.want, got, "OutputFormat.String() returned unexpected value") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix loop variable capture with parallel subtests.
All these subtest loops call t.Parallel() but don’t rebind tt, leading to nondeterministic failures.
Patch each loop:
for _, tt := range tests {
+ tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := tt.format.String()
assert.Equal(t, tt.want, got, "OutputFormat.String() returned unexpected value")
})
}Repeat similarly for the other loops (Extension, CompressionType.String, CompressionType.Extension, FileExtension, and String edge cases), adding tt := tt immediately inside each for _, tt := range tests { ... }.
Also applies to: 78-84, 127-133, 176-182, 267-276, 296-302
🤖 Prompt for AI Agents
In dump_options_test.go around lines 39-45 (and likewise for ranges 78-84,
127-133, 176-182, 267-276, 296-302), the subtests call t.Parallel() but capture
the loop variable tt directly, causing racey/nondeterministic failures; fix by
rebinding the loop variable inside each loop body (i.e., add tt := tt
immediately after the for ... { and before t.Run) so each subtest gets its own
copy of tt before calling t.Parallel().
Summary by CodeRabbit
Tests
Chores