fix: prevent nil pointer panic in codereview compareTestResults
Fix for https://github.com/boldsoftware/sketch/issues/175
Fix nil pointer dereference when comparing test results between commits
where a package exists in the after state but not the before state.
The panic occurred at line 867 when accessing beforeResult.TestStatus[test]
without checking if beforeResult was nil. This happens when testing new
packages that didn't exist in the base commit.
Now properly check for nil beforeResult and default to testStatusUnknown
for tests in packages that didn't exist in the before state.
Add comprehensive unit tests covering:
- New package with passing tests (the panic scenario)
- New package with failing tests (regression detection)
- Existing package regressions (ensure normal flow still works)
- End-to-end integration test for new package scenario
Co-Authored-By: sketch <hello@sketch.dev>
Change-ID: sab802d91eff08039k
diff --git a/claudetool/codereview/differential.go b/claudetool/codereview/differential.go
index d13cd9e..ff25b22 100644
--- a/claudetool/codereview/differential.go
+++ b/claudetool/codereview/differential.go
@@ -864,7 +864,10 @@
slog.WarnContext(context.Background(), "unknown test status", "package", pkg, "test", test)
continue
}
- beforeStatus := beforeResult.TestStatus[test]
+ beforeStatus := testStatusUnknown
+ if beforeResult != nil {
+ beforeStatus = beforeResult.TestStatus[test]
+ }
if isRegression(beforeStatus, afterStatus) {
testLevelRegressions = append(testLevelRegressions, testRegression{
Package: pkg,
diff --git a/claudetool/codereview/differential_test.go b/claudetool/codereview/differential_test.go
new file mode 100644
index 0000000..827bf63
--- /dev/null
+++ b/claudetool/codereview/differential_test.go
@@ -0,0 +1,180 @@
+package codereview
+
+import (
+ "testing"
+ "time"
+)
+
+// TestCompareTestResults_NilPointerPanic tests the fix for the nil pointer panic
+// that occurred when comparing test results between commits where a package
+// exists in the after state but not in the before state.
+func TestCompareTestResults_NilPointerPanic(t *testing.T) {
+ // Create a CodeReviewer instance (minimal setup)
+ reviewer := &CodeReviewer{
+ sketchBaseRef: "main",
+ }
+
+ // Create test results where:
+ // - before: empty (no packages)
+ // - after: has a package with tests
+ beforeResults := []testJSON{} // Empty - no packages existed before
+
+ afterResults := []testJSON{
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/newpkg",
+ Action: "run",
+ Test: "TestNewFunction",
+ },
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/newpkg",
+ Action: "pass",
+ Test: "TestNewFunction",
+ Elapsed: 0.001,
+ },
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/newpkg",
+ Action: "pass",
+ Test: "", // package-level pass
+ Elapsed: 0.001,
+ },
+ }
+
+ // This should not panic - before the fix, this would cause a nil pointer dereference
+ // when trying to access beforeResult.TestStatus[test] where beforeResult is nil
+ regressions, err := reviewer.compareTestResults(beforeResults, afterResults)
+ if err != nil {
+ t.Fatalf("compareTestResults failed: %v", err)
+ }
+
+ // We expect no regressions since the new test is passing
+ if len(regressions) != 0 {
+ t.Errorf("Expected no regressions for passing new test, got %d", len(regressions))
+ }
+}
+
+// TestCompareTestResults_NilPointerPanic_FailingTest tests the same scenario
+// but with a failing test to ensure we properly detect regressions
+func TestCompareTestResults_NilPointerPanic_FailingTest(t *testing.T) {
+ reviewer := &CodeReviewer{
+ sketchBaseRef: "main",
+ }
+
+ beforeResults := []testJSON{} // Empty - no packages existed before
+
+ afterResults := []testJSON{
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/newpkg",
+ Action: "run",
+ Test: "TestNewFunction",
+ },
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/newpkg",
+ Action: "fail",
+ Test: "TestNewFunction",
+ Elapsed: 0.001,
+ },
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/newpkg",
+ Action: "fail",
+ Test: "", // package-level fail
+ Elapsed: 0.001,
+ },
+ }
+
+ // This should not panic and should detect the regression
+ regressions, err := reviewer.compareTestResults(beforeResults, afterResults)
+ if err != nil {
+ t.Fatalf("compareTestResults failed: %v", err)
+ }
+
+ // We expect 1 regression for the failing new test
+ if len(regressions) != 1 {
+ t.Errorf("Expected 1 regression for failing new test, got %d", len(regressions))
+ }
+
+ if len(regressions) > 0 {
+ regression := regressions[0]
+ if regression.Package != "sketch.dev/newpkg" {
+ t.Errorf("Expected package 'sketch.dev/newpkg', got %q", regression.Package)
+ }
+ if regression.Test != "TestNewFunction" {
+ t.Errorf("Expected test 'TestNewFunction', got %q", regression.Test)
+ }
+ if regression.BeforeStatus != testStatusUnknown {
+ t.Errorf("Expected before status testStatusUnknown, got %v", regression.BeforeStatus)
+ }
+ if regression.AfterStatus != testStatusFail {
+ t.Errorf("Expected after status testStatusFail, got %v", regression.AfterStatus)
+ }
+ }
+}
+
+// TestCompareTestResults_ExistingPackage tests the normal case where
+// a package exists in both before and after states
+func TestCompareTestResults_ExistingPackage(t *testing.T) {
+ reviewer := &CodeReviewer{
+ sketchBaseRef: "main",
+ }
+
+ // Package exists in both before and after
+ beforeResults := []testJSON{
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/existing",
+ Action: "pass",
+ Test: "TestExisting",
+ Elapsed: 0.001,
+ },
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/existing",
+ Action: "pass",
+ Test: "",
+ Elapsed: 0.001,
+ },
+ }
+
+ afterResults := []testJSON{
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/existing",
+ Action: "fail",
+ Test: "TestExisting",
+ Elapsed: 0.001,
+ },
+ {
+ Time: time.Now(),
+ Package: "sketch.dev/existing",
+ Action: "fail",
+ Test: "",
+ Elapsed: 0.001,
+ },
+ }
+
+ // This should detect the regression from pass to fail
+ regressions, err := reviewer.compareTestResults(beforeResults, afterResults)
+ if err != nil {
+ t.Fatalf("compareTestResults failed: %v", err)
+ }
+
+ // We expect 1 regression
+ if len(regressions) != 1 {
+ t.Errorf("Expected 1 regression, got %d", len(regressions))
+ }
+
+ if len(regressions) > 0 {
+ regression := regressions[0]
+ if regression.BeforeStatus != testStatusPass {
+ t.Errorf("Expected before status testStatusPass, got %v", regression.BeforeStatus)
+ }
+ if regression.AfterStatus != testStatusFail {
+ t.Errorf("Expected after status testStatusFail, got %v", regression.AfterStatus)
+ }
+ }
+}
diff --git a/claudetool/codereview/testdata/new_package_with_tests.txtar b/claudetool/codereview/testdata/new_package_with_tests.txtar
new file mode 100644
index 0000000..8d6eede
--- /dev/null
+++ b/claudetool/codereview/testdata/new_package_with_tests.txtar
@@ -0,0 +1,33 @@
+New package with tests should not cause nil pointer panic
+
+-- go.mod --
+module sketch.dev
+
+go 1.21
+
+-- .commit --
+Initial commit with no packages
+
+-- newpkg/main.go --
+package newpkg
+
+func Hello() string {
+ return "hello"
+}
+
+-- newpkg/main_test.go --
+package newpkg
+
+import "testing"
+
+func TestHello(t *testing.T) {
+ if got := Hello(); got != "hello" {
+ t.Errorf("Hello() = %q, want %q", got, "hello")
+ }
+}
+
+-- .commit --
+Add new package with tests
+
+-- .run_test --
+OK