introduce fuzzing test #43
Conversation
WalkthroughThis pull request introduces a GitHub Actions workflow to automate fuzz testing and a new test file for the CSV decoding package. The workflow file ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Scheduler
participant GH as GitHub Actions Runner
participant List as "List Job"
participant Fuzz as "Fuzz Job"
U->>GH: Trigger workflow (manual or schedule)
GH->>List: Start list job (10m timeout)
List-->>GH: Provide fuzz tests output
GH->>Fuzz: Initiate fuzz job (matrix based on list output, 360m timeout)
Fuzz-->>GH: Execute fuzz tests
GH->>U: Report testing results
Poem
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces fuzz tests to improve the resiliency of our CSV decoding functionality within the headercsv package by testing various data structures.
- Added fuzz tests to decode data into map, slice, and struct types
- Introduced a GitHub workflow to schedule and run the fuzz tests regularly
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| fuzz_test.go | Added fuzz tests for different output types in CSV decoding |
| .github/workflows/fuzz.yml | Added a GitHub workflow for scheduling and running fuzz tests |
Comments suppressed due to low confidence (3)
fuzz_test.go:8
- [nitpick] Consider renaming the fuzz test function FuzzDecodeAll_map to FuzzDecodeAllMap to adhere to Go naming conventions.
func FuzzDecodeAll_map(f *testing.F) {
fuzz_test.go:21
- [nitpick] Consider renaming the fuzz test function FuzzDecodeAll_slice to FuzzDecodeAllSlice for consistency with Go naming conventions.
func FuzzDecodeAll_slice(f *testing.F) {
fuzz_test.go:34
- [nitpick] Consider renaming the fuzz test function FuzzDecodeAll_struct to FuzzDecodeAllStruct to better align with standard Go naming practices.
func FuzzDecodeAll_struct(f *testing.F) {
Pull Request Test Coverage Report for Build 14100686368Details
💛 - Coveralls |
![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 (6)
.github/workflows/fuzz.yml (1)
25-43: Consider adjusting the fuzz job configurationThe fuzz job setup looks good overall with proper dependency on the list job and matrix strategy. However:
- The 360-minute timeout is quite long. Consider splitting into shorter runs if possible.
- The fuzz time (355m) is very close to the timeout (360m), which might lead to job termination before proper cleanup.
- timeout-minutes: 360 + timeout-minutes: 370This would provide a better buffer between the fuzz time and the job timeout.
fuzz_test.go (5)
8-19: Improve fuzz test assertions and coverageThe map fuzzer correctly sets up the test cases, but currently only checks if decoding succeeds. Consider:
- Adding validation of the decoded data
- Including assertion for expected output when using seed cases
- Adding error cases that should fail in a predictable way
func FuzzDecodeAll_map(f *testing.F) { f.Add("A\nb\n") f.Add("A,B\nb,c\n") + // Add malformed but interesting test cases + f.Add("A,A\nb,c\n") // Duplicate headers + f.Add("A,\nb,c\n") // Empty header f.Fuzz(func(t *testing.T, a string) { dec := NewDecoder(strings.NewReader(a)) var v []map[string]any if err := dec.DecodeAll(&v); err != nil { t.Skip(err) } + // Validate the decoded data + if len(v) > 0 && len(a) > 0 { + // Check that we have at least some keys in our maps + hasKeys := false + for _, row := range v { + if len(row) > 0 { + hasKeys = true + break + } + } + if !hasKeys && strings.Contains(a, ",") { + t.Errorf("Expected at least one key in decoded maps for input with commas") + } + } }) }
21-32: Enhance slice fuzz test with validationSimilar to the map fuzzer, this test would benefit from validating the decoded data and adding more diverse test cases.
func FuzzDecodeAll_slice(f *testing.F) { f.Add("A\nb\n") f.Add("A,B\nb,c\n") + f.Add("A,B,C\n1,2,3\n4,5,6\n") // Multiple rows with numeric values f.Fuzz(func(t *testing.T, a string) { dec := NewDecoder(strings.NewReader(a)) var v [][]any if err := dec.DecodeAll(&v); err != nil { t.Skip(err) } + // Basic validation + if len(v) > 0 { + headerCount := strings.Count(strings.Split(a, "\n")[0], ",") + 1 + for i, row := range v { + // Each row should have the same number of elements as headers + if len(row) != headerCount && headerCount > 0 { + t.Errorf("Row %d has %d columns, expected %d", i, len(row), headerCount) + } + } + } }) }
34-51: Add more test cases for struct fuzzingThe struct fuzz test correctly handles various data types but could benefit from more diverse seed cases. Also, consider validating the decoded values.
func FuzzDecodeAll_struct(f *testing.F) { f.Add("String,Int,Uint,Float64,Bool,Any\nHello,42,42,3.14,true,World\n") + // Add edge cases + f.Add("String,Int,Uint,Float64,Bool,Any\n\"Quoted, String\",-1,0,0.0,false,null\n") + f.Add("String,Int,Uint,Float64,Bool,Any\n,,,,,\n") // Empty values f.Fuzz(func(t *testing.T, a string) { dec := NewDecoder(strings.NewReader(a)) var v []struct { String string Int int Uint uint Float64 float64 Bool bool Any any } if err := dec.DecodeAll(&v); err != nil { t.Skip(err) } + + // For known seed cases, verify expected values + if a == "String,Int,Uint,Float64,Bool,Any\nHello,42,42,3.14,true,World\n" && len(v) > 0 { + if v[0].String != "Hello" || v[0].Int != 42 || v[0].Uint != 42 || + v[0].Float64 != 3.14 || v[0].Bool != true || v[0].Any != "World" { + t.Errorf("Decoded values don't match expected output for seed case") + } + } }) }
3-6: Consider adding benchmarks alongside fuzz testsThe test file correctly imports necessary packages, but consider adding benchmarks to complement your fuzz tests. This would help evaluate performance impacts of any changes made to fix issues found by fuzzing.
1-51: Add documentation and error handling strategyWhile the fuzz tests are well-structured, adding package documentation would improve maintainability. Also, the current error handling (skipping tests on error) is appropriate for exploring valid inputs, but consider a strategy for handling expected vs. unexpected errors.
package headercsv +// This file contains fuzz tests for the headercsv package. +// These tests use Go's built-in fuzzing to find edge cases in CSV decoding. +// The tests focus on three main decoding targets: maps, slices, and structs. import ( "strings" "testing" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/fuzz.yml(1 hunks)fuzz_test.go(1 hunks)
🔇 Additional comments (3)
.github/workflows/fuzz.yml (3)
1-5: LGTM: Well-structured workflow setupThe workflow name and triggers are appropriately defined. Using both manual trigger (
workflow_dispatch) and scheduled runs (Tuesdays and Thursdays) is a good practice for fuzz testing.
11-24: LGTM: Well-structured list jobThe
listjob is properly configured:
- Uses up-to-date GitHub Actions with pinned versions and SHA hashes (good security practice)
- Has a reasonable timeout
- Correctly exports the list of fuzz tests for the next job
7-10:✅ Verification successful
Verify if these permissions are necessary
The workflow requests write permissions for repository contents and pull requests. While these might be needed if the fuzzer creates issues or PRs for discovered bugs, it's worth confirming if both are required.
🏁 Script executed:
#!/bin/bash # Check if the fuzzing action documentation mentions required permissions curl -s https://raw.githubusercontent.com/shogo82148/actions-go-fuzz/main/README.md | grep -i "permission"Length of output: 531
Verified Permissions for Fuzz Workflow
The actions-go-fuzz documentation confirms that when using the pull-request reporting mode, the workflow must have bothcontents: write(to enable pushes/branch creation) andpull-requests: write(to create and modify pull requests). If you switch to a different report method (like slack), you may be able to remove or reduce these permissions. For the current configuration, these permissions appear necessary.
Summary by CodeRabbit
Chores
Tests