diff --git a/internal/quality/container_unwrap_internal_test.go b/internal/quality/container_unwrap_internal_test.go index 802c5d5..fe241c7 100644 --- a/internal/quality/container_unwrap_internal_test.go +++ b/internal/quality/container_unwrap_internal_test.go @@ -5,6 +5,7 @@ import ( "go/parser" "go/token" "go/types" + "strings" "testing" "golang.org/x/tools/go/packages" @@ -1021,3 +1022,385 @@ func f() { t.Error("matchTrackedInExpr should return false when no identifiers match tracked set") } } + +// --- generateSuggestion tests --- + +// TestGenerateSuggestion verifies all 5 switch cases + default. +func TestGenerateSuggestion(t *testing.T) { + tests := []struct { + name string + effectType taxonomy.SideEffectType + desc string + wantParts []string + }{ + { + name: "LogWrite", + effectType: taxonomy.LogWrite, + desc: "writes to logger", + wantParts: []string{"log output", "implementation detail"}, + }, + { + name: "StdoutWrite", + effectType: taxonomy.StdoutWrite, + desc: "prints to stdout", + wantParts: []string{"stdout"}, + }, + { + name: "GoroutineSpawn", + effectType: taxonomy.GoroutineSpawn, + desc: "spawns worker", + wantParts: []string{"goroutine lifecycle", "concurrency detail"}, + }, + { + name: "ContextCancellation", + effectType: taxonomy.ContextCancellation, + desc: "cancels context", + wantParts: []string{"context usage", "implementation detail"}, + }, + { + name: "CallbackInvocation", + effectType: taxonomy.CallbackInvocation, + desc: "invokes callback", + wantParts: []string{"callback invocation", "implementation detail"}, + }, + { + name: "Default_UnknownType", + effectType: taxonomy.SideEffectType("CustomEffect"), + desc: "does something custom", + wantParts: []string{"CustomEffect", "contract behavior"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := generateSuggestion(tt.effectType, tt.desc) + for _, part := range tt.wantParts { + if !strings.Contains(got, part) { + t.Errorf("generateSuggestion(%s, %q) = %q, want to contain %q", + tt.effectType, tt.desc, got, part) + } + } + // Verify the description appears in the output. + if !strings.Contains(got, tt.desc) { + t.Errorf("generateSuggestion(%s, %q) = %q, want to contain description %q", + tt.effectType, tt.desc, got, tt.desc) + } + }) + } +} + +// --- rhsReferencesAnyTracked tests --- + +// TestRhsReferencesAnyTracked_DirectMatch verifies direct containsObject path. +func TestRhsReferencesAnyTracked_DirectMatch(t *testing.T) { + src := `package p +func f() { + x := 1 + y := x + _ = y +}` + file, info := parseAndTypeCheck(t, src) + xObj := extractVarObj(t, file, info, 0, 0) // x := 1 + rhs := extractRHS(t, file, 0, 1) // x (RHS of y := x) + + tracked := map[types.Object]bool{xObj: true} + if !rhsReferencesAnyTracked(rhs, tracked, info) { + t.Error("rhsReferencesAnyTracked should return true when tracked var is directly in RHS") + } +} + +// TestRhsReferencesAnyTracked_ResolveExprRootFallback verifies resolveExprRoot path. +func TestRhsReferencesAnyTracked_ResolveExprRootFallback(t *testing.T) { + src := `package p +type S struct{ Field int } +func f() { + x := S{Field: 1} + y := x.Field + _ = y +}` + file, info := parseAndTypeCheck(t, src) + xObj := extractVarObj(t, file, info, 1, 0) // x := S{...} (func is Decls[1] after type decl) + rhs := extractRHS(t, file, 1, 1) // x.Field + + tracked := map[types.Object]bool{xObj: true} + if !rhsReferencesAnyTracked(rhs, tracked, info) { + t.Error("rhsReferencesAnyTracked should return true via resolveExprRoot for x.Field") + } +} + +// TestRhsReferencesAnyTracked_NoMatch verifies false when no tracked var in RHS. +func TestRhsReferencesAnyTracked_NoMatch(t *testing.T) { + src := `package p +func f() { + x := 1 + y := 2 + _ = x + _ = y +}` + file, info := parseAndTypeCheck(t, src) + xObj := extractVarObj(t, file, info, 0, 0) // x := 1 + rhs := extractRHS(t, file, 0, 1) // 2 (RHS of y := 2) + + tracked := map[types.Object]bool{xObj: true} + if rhsReferencesAnyTracked(rhs, tracked, info) { + t.Error("rhsReferencesAnyTracked should return false when no tracked var in RHS") + } +} + +// --- handleTransformationCalls tests --- + +// TestHandleTransformationCalls_TransformWithTrackedArg verifies transformation +// call with tracked data returns the pointer destination. +func TestHandleTransformationCalls_TransformWithTrackedArg(t *testing.T) { + // Use a local function stub matching the transformation pattern + // (byte-like param + pointer dest) instead of importing encoding/json. + src := `package p + +func unmarshal(data []byte, v *map[string]any) {} +func f() { + data := []byte("{}") + var target map[string]any + unmarshal(data, &target) +}` + file, info := parseAndTypeCheck(t, src) + dataObj := extractVarObj(t, file, info, 1, 0) // data := []byte("{}") + + // Extract the unmarshal call (it's an ExprStmt, not AssignStmt). + call := extractCallFromFunc(t, file, 1, 2) + + tracked := map[types.Object]bool{dataObj: true} + dest, handled := handleTransformationCalls(call, tracked, info) + if !handled { + t.Fatal("handleTransformationCalls should handle transformation call with tracked arg") + } + if dest == nil { + t.Fatal("handleTransformationCalls should return non-nil dest for pointer arg") + } + if dest.Name() != "target" { + t.Errorf("dest.Name() = %q, want %q", dest.Name(), "target") + } +} + +// TestHandleTransformationCalls_NoTransformation verifies non-transformation call. +func TestHandleTransformationCalls_NoTransformation(t *testing.T) { + src := `package p +func add(a, b int) int { return a + b } +func f() { + x := 1 + add(x, 2) +}` + file, info := parseAndTypeCheck(t, src) + xObj := extractVarObj(t, file, info, 1, 0) // x := 1 + + call := extractCallFromFunc(t, file, 1, 1) // add(x, 2) + + tracked := map[types.Object]bool{xObj: true} + _, handled := handleTransformationCalls(call, tracked, info) + if handled { + t.Error("handleTransformationCalls should return handled=false for non-transformation call") + } +} + +// TestHandleTransformationCalls_NoTrackedArg verifies transformation call +// without tracked arg returns handled=false. +func TestHandleTransformationCalls_NoTrackedArg(t *testing.T) { + src := `package p + +func unmarshal(data []byte, v *map[string]any) {} +func f() { + unrelated := 42 + data := []byte("{}") + var target map[string]any + unmarshal(data, &target) + _ = unrelated +}` + file, info := parseAndTypeCheck(t, src) + unrelObj := extractVarObj(t, file, info, 1, 0) // unrelated := 42 + call := extractCallFromFunc(t, file, 1, 3) // unmarshal(data, &target) + + tracked := map[types.Object]bool{unrelObj: true} + _, handled := handleTransformationCalls(call, tracked, info) + if handled { + t.Error("handleTransformationCalls should return handled=false when no tracked arg flows into call") + } +} + +// --- extractDataFlowLHS tests --- + +// TestExtractDataFlowLHS_ValidIdent verifies LHS extraction for valid identifier. +func TestExtractDataFlowLHS_ValidIdent(t *testing.T) { + src := `package p +func f() { + x := 42 + _ = x +}` + file, info := parseAndTypeCheck(t, src) + + fn, ok := file.Decls[0].(*ast.FuncDecl) + if !ok { + t.Fatal("expected *ast.FuncDecl") + } + assign, ok := fn.Body.List[0].(*ast.AssignStmt) + if !ok { + t.Fatal("expected *ast.AssignStmt") + } + + obj := extractDataFlowLHS(assign, 0, info) + if obj == nil { + t.Fatal("extractDataFlowLHS should return non-nil for valid LHS ident") + } + if obj.Name() != "x" { + t.Errorf("obj.Name() = %q, want %q", obj.Name(), "x") + } +} + +// TestExtractDataFlowLHS_BlankIdent verifies nil return for blank identifier "_". +func TestExtractDataFlowLHS_BlankIdent(t *testing.T) { + src := `package p +func f() { + _ = 42 +}` + file, info := parseAndTypeCheck(t, src) + + fn, ok := file.Decls[0].(*ast.FuncDecl) + if !ok { + t.Fatal("expected *ast.FuncDecl") + } + assign, ok := fn.Body.List[0].(*ast.AssignStmt) + if !ok { + t.Fatal("expected *ast.AssignStmt") + } + + obj := extractDataFlowLHS(assign, 0, info) + if obj != nil { + t.Error("extractDataFlowLHS should return nil for blank identifier '_'") + } +} + +// TestExtractDataFlowLHS_NonIdentLHS verifies nil return for non-identifier LHS (e.g., s.Field). +func TestExtractDataFlowLHS_NonIdentLHS(t *testing.T) { + src := `package p +type S struct{ Field int } +func f() { + var s S + s.Field = 42 + _ = s +}` + file, info := parseAndTypeCheck(t, src) + + fn, ok := file.Decls[1].(*ast.FuncDecl) // func f() is Decls[1] after type S + if !ok { + t.Fatal("expected *ast.FuncDecl") + } + // s.Field = 42 is the second statement (after var s S) + assign, ok := fn.Body.List[1].(*ast.AssignStmt) + if !ok { + t.Fatal("expected *ast.AssignStmt for s.Field = 42") + } + + obj := extractDataFlowLHS(assign, 0, info) + if obj != nil { + t.Error("extractDataFlowLHS should return nil for SelectorExpr LHS (s.Field)") + } +} + +// TestExtractDataFlowLHS_Reassignment verifies the info.Uses fallback for = reassignment. +func TestExtractDataFlowLHS_Reassignment(t *testing.T) { + src := `package p +func f() { + x := 1 + x = 2 + _ = x +}` + file, info := parseAndTypeCheck(t, src) + + fn, ok := file.Decls[0].(*ast.FuncDecl) + if !ok { + t.Fatal("expected *ast.FuncDecl") + } + assign, ok := fn.Body.List[1].(*ast.AssignStmt) // x = 2 (reassignment) + if !ok { + t.Fatal("expected *ast.AssignStmt for x = 2") + } + + obj := extractDataFlowLHS(assign, 0, info) + if obj == nil { + t.Fatal("extractDataFlowLHS should return non-nil for reassignment via info.Uses") + } + if obj.Name() != "x" { + t.Errorf("obj.Name() = %q, want %q", obj.Name(), "x") + } +} + +// TestExtractDataFlowLHS_OutOfRange verifies nil return when rhsIdx is out of range. +func TestExtractDataFlowLHS_OutOfRange(t *testing.T) { + src := `package p +func f() { + x := 42 + _ = x +}` + file, info := parseAndTypeCheck(t, src) + + fn, ok := file.Decls[0].(*ast.FuncDecl) + if !ok { + t.Fatal("expected *ast.FuncDecl") + } + assign, ok := fn.Body.List[0].(*ast.AssignStmt) + if !ok { + t.Fatal("expected *ast.AssignStmt") + } + + // rhsIdx=5 is beyond len(assign.Lhs) + obj := extractDataFlowLHS(assign, 5, info) + if obj != nil { + t.Error("extractDataFlowLHS should return nil when rhsIdx is out of range") + } +} + +// TestTraceForwardDataFlow_MultiIteration verifies that traceForwardDataFlow +// propagates tracked variables across multiple iterations through chained +// data-extraction assignments: a → b → c. +func TestTraceForwardDataFlow_MultiIteration(t *testing.T) { + src := `package p + +type Inner struct{ Name string } +type Outer struct{ Items []Inner } +type Result struct{ Data Outer } + +func f() { + r := Result{Data: Outer{Items: []Inner{{Name: "x"}}}} + a := r.Data + b := a.Items[0] + c := b.Name + _ = c +}` + file, info, fset := parseAndTypeCheckWithFset(t, src) + + // func f() is Decls[3] (after 3 type declarations) + rObj := extractVarObj(t, file, info, 3, 0) // r := Result{...} + aObj := extractVarObj(t, file, info, 3, 1) // a := r.Data + bObj := extractVarObj(t, file, info, 3, 2) // b := a.Items[0] + cObj := extractVarObj(t, file, info, 3, 3) // c := b.Name + + tracked := map[types.Object]bool{rObj: true} + pkg := &packages.Package{ + Syntax: []*ast.File{file}, + TypesInfo: info, + Fset: fset, + } + + result := traceForwardDataFlow(tracked, pkg) + + if !result[rObj] { + t.Error("should preserve original tracked variable r") + } + if !result[aObj] { + t.Error("iteration 1: should track a (from a := r.Data)") + } + if !result[bObj] { + t.Error("iteration 2: should track b (from b := a.Items[0])") + } + if !result[cObj] { + t.Error("iteration 3: should track c (from c := b.Name)") + } +} diff --git a/internal/quality/mapping.go b/internal/quality/mapping.go index ee751ac..d71cd8d 100644 --- a/internal/quality/mapping.go +++ b/internal/quality/mapping.go @@ -995,98 +995,26 @@ func traceForwardDataFlow(tracked map[types.Object]bool, testPkg *packages.Packa } for rhsIdx, rhs := range assign.Rhs { - // Check if any tracked variable appears in this RHS. - rhsReferencesTracked := false - for obj := range tracked { - if containsObject(rhs, obj, info) { - rhsReferencesTracked = true - break - } - } - // Also check via resolveExprRoot for compound - // expressions like result.Content[0].Text. - if !rhsReferencesTracked { - root := resolveExprRoot(rhs, info) - if root != nil { - rootObj := info.Uses[root] - if rootObj == nil { - rootObj = info.Defs[root] - } - if rootObj != nil && tracked[rootObj] { - rhsReferencesTracked = true - } - } - } - - if !rhsReferencesTracked { + if !rhsReferencesAnyTracked(rhs, tracked, info) { continue } // Check if the RHS contains a transformation call. - // If so, extract the pointer destination as the - // new tracked variable (bridging across the transform). - transformHandled := false - ast.Inspect(rhs, func(cn ast.Node) bool { - if transformHandled { - return false - } - call, ok := cn.(*ast.CallExpr) - if !ok { - return true - } - _, ptrIdx, isTransform := isTransformationCall(call, info) - if !isTransform { - return true - } - // Verify a tracked variable flows into this call. - callHasTracked := false - for _, arg := range call.Args { - for obj := range tracked { - if containsObject(arg, obj, info) { - callHasTracked = true - break - } - } - if callHasTracked { - break - } - } - if !callHasTracked { - return true - } - dest := extractPointerDest(call, ptrIdx, info) + dest, handled := handleTransformationCalls(rhs, tracked, info) + if handled { if dest != nil { newTracked[dest] = true - transformHandled = true } - return false - }) - - if transformHandled { continue } // Non-transformation assignment: only track LHS - // when the RHS is a data-extraction expression - // (field access, index, type assertion, or type - // conversion). Method calls and function calls - // are excluded to prevent false positives from - // patterns like got := s.Get("key") where s is - // tracked from a NewStore() return value. + // when the RHS is a data-extraction expression. if !isDataExtraction(rhs) { continue } - if rhsIdx < len(assign.Lhs) { - lhsExpr := assign.Lhs[rhsIdx] - if ident, ok := lhsExpr.(*ast.Ident); ok && ident.Name != "_" { - obj := info.Defs[ident] - if obj == nil { - obj = info.Uses[ident] - } - if obj != nil { - newTracked[obj] = true - } - } + if lhsObj := extractDataFlowLHS(assign, rhsIdx, info); lhsObj != nil { + newTracked[lhsObj] = true } } return true @@ -1106,6 +1034,94 @@ func traceForwardDataFlow(tracked map[types.Object]bool, testPkg *packages.Packa return tracked } +// rhsReferencesAnyTracked checks whether any tracked variable appears in the +// RHS expression, either via direct containsObject or resolveExprRoot fallback +// for compound expressions like result.Content[0].Text. +func rhsReferencesAnyTracked(rhs ast.Expr, tracked map[types.Object]bool, info *types.Info) bool { + for obj := range tracked { + if containsObject(rhs, obj, info) { + return true + } + } + // Also check via resolveExprRoot for compound expressions. + root := resolveExprRoot(rhs, info) + if root != nil { + rootObj := info.Uses[root] + if rootObj == nil { + rootObj = info.Defs[root] + } + if rootObj != nil && tracked[rootObj] { + return true + } + } + return false +} + +// handleTransformationCalls inspects the RHS for transformation calls +// (e.g., json.Unmarshal(data, &target)) where a tracked variable flows +// into the call arguments. Returns the pointer destination and whether +// a transformation was fully resolved — i.e., a qualifying call was found +// AND its pointer destination was successfully extracted. When handled is +// false, the caller falls through to the isDataExtraction path. +func handleTransformationCalls(rhs ast.Expr, tracked map[types.Object]bool, info *types.Info) (types.Object, bool) { + var dest types.Object + handled := false + ast.Inspect(rhs, func(cn ast.Node) bool { + if handled { + return false + } + call, ok := cn.(*ast.CallExpr) + if !ok { + return true + } + _, ptrIdx, isTransform := isTransformationCall(call, info) + if !isTransform { + return true + } + // Verify a tracked variable flows into this call. + callHasTracked := false + for _, arg := range call.Args { + for obj := range tracked { + if containsObject(arg, obj, info) { + callHasTracked = true + break + } + } + if callHasTracked { + break + } + } + if !callHasTracked { + return true + } + d := extractPointerDest(call, ptrIdx, info) + if d != nil { + dest = d + handled = true + } + return false + }) + return dest, handled +} + +// extractDataFlowLHS extracts the LHS variable from an assignment at the +// given RHS index. Returns nil for blank identifiers, non-ident LHS, or +// out-of-range indices. +func extractDataFlowLHS(assign *ast.AssignStmt, rhsIdx int, info *types.Info) types.Object { + if rhsIdx >= len(assign.Lhs) { + return nil + } + ident, ok := assign.Lhs[rhsIdx].(*ast.Ident) + if !ok || ident.Name == "_" { + return nil + } + obj := info.Defs[ident] + if obj == nil { + obj = info.Uses[ident] + } + return obj +} + // matchTrackedInExpr walks the expression tree via ast.Inspect and // returns true if any identifier's types.Object is in the tracked set. // It checks both direct identity (via info.Uses and info.Defs) and diff --git a/internal/quality/report.go b/internal/quality/report.go index ee45fde..bf930b7 100644 --- a/internal/quality/report.go +++ b/internal/quality/report.go @@ -27,141 +27,188 @@ func WriteJSON(w io.Writer, reports []taxonomy.QualityReport, summary *taxonomy. return enc.Encode(output) } +// qualityStyles bundles the lipgloss styles used by the quality text report. +type qualityStyles struct { + header lipgloss.Style + good lipgloss.Style + warn lipgloss.Style + bad lipgloss.Style + muted lipgloss.Style +} + +// newQualityStyles returns the standard quality report style palette. +func newQualityStyles() qualityStyles { + return qualityStyles{ + header: lipgloss.NewStyle().Bold(true), + good: lipgloss.NewStyle().Foreground(lipgloss.Color("2")), // green + warn: lipgloss.NewStyle().Foreground(lipgloss.Color("3")), // yellow + bad: lipgloss.NewStyle().Foreground(lipgloss.Color("1")), // red + muted: lipgloss.NewStyle().Foreground(lipgloss.Color("240")), // gray + } +} + // WriteText writes a human-readable quality report with lipgloss styling. func WriteText(w io.Writer, reports []taxonomy.QualityReport, summary *taxonomy.PackageSummary) error { - // Styles. - header := lipgloss.NewStyle().Bold(true) - good := lipgloss.NewStyle().Foreground(lipgloss.Color("2")) // green - warn := lipgloss.NewStyle().Foreground(lipgloss.Color("3")) // yellow - bad := lipgloss.NewStyle().Foreground(lipgloss.Color("1")) // red - muted := lipgloss.NewStyle().Foreground(lipgloss.Color("240")) // gray + s := newQualityStyles() for i, r := range reports { if i > 0 { _, _ = fmt.Fprintln(w) } + writeReportHeader(w, r, s) + writeContractCoverage(w, r.ContractCoverage, s) + writeOverSpecification(w, r.OverSpecification.Count, s) + writeDetectionConfidence(w, r.AssertionDetectionConfidence, s) + writeGapsSection(w, r.ContractCoverage, s) + writeDiscardedReturns(w, r.ContractCoverage, s) + writeSuggestionsSection(w, r.OverSpecification.Suggestions, s) + writeAmbiguousEffects(w, r.AmbiguousEffects) + writeUnmappedAssertions(w, r.UnmappedAssertions) + } - // Header line. - _, _ = fmt.Fprintln(w, header.Render(fmt.Sprintf( - "=== %s -> %s ===", - r.TestFunction, - r.TargetFunction.QualifiedName()))) - - _, _ = fmt.Fprintf(w, " Test: %s\n", r.TestLocation) - _, _ = fmt.Fprintf(w, " Target: %s\n", r.TargetFunction.Location) - - // Contract Coverage. - covPct := r.ContractCoverage.Percentage - covStyle := good - if covPct < 50 { - covStyle = bad - } else if covPct < 80 { - covStyle = warn - } - _, _ = fmt.Fprintf(w, " Contract Coverage: %s (%d/%d)\n", - covStyle.Render(fmt.Sprintf("%.0f%%", covPct)), - r.ContractCoverage.CoveredCount, - r.ContractCoverage.TotalContractual) - - // Over-Specification. - overCount := r.OverSpecification.Count - overStyle := good - if overCount > 0 { - overStyle = warn - } - if overCount > 3 { - overStyle = bad - } - _, _ = fmt.Fprintf(w, " Over-Specified: %s\n", - overStyle.Render(fmt.Sprintf("%d", overCount))) - - // Detection Confidence. - detConf := r.AssertionDetectionConfidence - detStyle := good - if detConf < 70 { - detStyle = warn - } - if detConf < 50 { - detStyle = bad - } - _, _ = fmt.Fprintf(w, " Detection Confidence: %s\n", - detStyle.Render(fmt.Sprintf("%d%%", detConf))) - - // Gaps. - if len(r.ContractCoverage.Gaps) > 0 { - _, _ = fmt.Fprintln(w, muted.Render(" Gaps (untested contractual effects):")) - for i, gap := range r.ContractCoverage.Gaps { - _, _ = fmt.Fprintf(w, " - %s: %s (%s)\n", - gap.Type, gap.Description, gap.Location) - // Hint: show the suggested assertion if available. - if i < len(r.ContractCoverage.GapHints) && r.ContractCoverage.GapHints[i] != "" { - _, _ = fmt.Fprintf(w, " hint: %s\n", r.ContractCoverage.GapHints[i]) - } + writeSSADiagnostics(w, summary, s) + writePackageSummary(w, summary, s) + + return nil +} + +// writeReportHeader writes the test-to-target header and location lines. +func writeReportHeader(w io.Writer, r taxonomy.QualityReport, s qualityStyles) { + _, _ = fmt.Fprintln(w, s.header.Render(fmt.Sprintf( + "=== %s -> %s ===", + r.TestFunction, + r.TargetFunction.QualifiedName()))) + _, _ = fmt.Fprintf(w, " Test: %s\n", r.TestLocation) + _, _ = fmt.Fprintf(w, " Target: %s\n", r.TargetFunction.Location) +} + +// writeContractCoverage writes the contract coverage metric with threshold-based styling. +func writeContractCoverage(w io.Writer, cc taxonomy.ContractCoverage, s qualityStyles) { + covPct := cc.Percentage + covStyle := s.good + if covPct < 50 { + covStyle = s.bad + } else if covPct < 80 { + covStyle = s.warn + } + _, _ = fmt.Fprintf(w, " Contract Coverage: %s (%d/%d)\n", + covStyle.Render(fmt.Sprintf("%.0f%%", covPct)), + cc.CoveredCount, + cc.TotalContractual) +} + +// writeOverSpecification writes the over-specification count with threshold-based styling. +func writeOverSpecification(w io.Writer, overCount int, s qualityStyles) { + overStyle := s.good + if overCount > 0 { + overStyle = s.warn + } + if overCount > 3 { + overStyle = s.bad + } + _, _ = fmt.Fprintf(w, " Over-Specified: %s\n", + overStyle.Render(fmt.Sprintf("%d", overCount))) +} + +// writeDetectionConfidence writes the detection confidence metric with threshold-based styling. +func writeDetectionConfidence(w io.Writer, detConf int, s qualityStyles) { + detStyle := s.good + if detConf < 70 { + detStyle = s.warn + } + if detConf < 50 { + detStyle = s.bad + } + _, _ = fmt.Fprintf(w, " Detection Confidence: %s\n", + detStyle.Render(fmt.Sprintf("%d%%", detConf))) +} + +// writeGapsSection writes the untested contractual effects gaps list with optional hints. +func writeGapsSection(w io.Writer, cc taxonomy.ContractCoverage, s qualityStyles) { + if len(cc.Gaps) > 0 { + _, _ = fmt.Fprintln(w, s.muted.Render(" Gaps (untested contractual effects):")) + for i, gap := range cc.Gaps { + _, _ = fmt.Fprintf(w, " - %s: %s (%s)\n", + gap.Type, gap.Description, gap.Location) + if i < len(cc.GapHints) && cc.GapHints[i] != "" { + _, _ = fmt.Fprintf(w, " hint: %s\n", cc.GapHints[i]) } } + } +} - // Discarded returns (definitively unasserted). - if len(r.ContractCoverage.DiscardedReturns) > 0 { - _, _ = fmt.Fprintln(w, muted.Render(" Discarded returns (definitively unasserted):")) - for i, dr := range r.ContractCoverage.DiscardedReturns { - _, _ = fmt.Fprintf(w, " - %s: %s (%s)\n", - dr.Type, dr.Description, dr.Location) - if i < len(r.ContractCoverage.DiscardedReturnHints) && r.ContractCoverage.DiscardedReturnHints[i] != "" { - _, _ = fmt.Fprintf(w, " hint: %s\n", r.ContractCoverage.DiscardedReturnHints[i]) - } +// writeDiscardedReturns writes the definitively unasserted discarded returns list with optional hints. +func writeDiscardedReturns(w io.Writer, cc taxonomy.ContractCoverage, s qualityStyles) { + if len(cc.DiscardedReturns) > 0 { + _, _ = fmt.Fprintln(w, s.muted.Render(" Discarded returns (definitively unasserted):")) + for i, dr := range cc.DiscardedReturns { + _, _ = fmt.Fprintf(w, " - %s: %s (%s)\n", + dr.Type, dr.Description, dr.Location) + if i < len(cc.DiscardedReturnHints) && cc.DiscardedReturnHints[i] != "" { + _, _ = fmt.Fprintf(w, " hint: %s\n", cc.DiscardedReturnHints[i]) } } + } +} - // Suggestions. - if len(r.OverSpecification.Suggestions) > 0 { - _, _ = fmt.Fprintln(w, muted.Render(" Suggestions:")) - for _, s := range r.OverSpecification.Suggestions { - _, _ = fmt.Fprintf(w, " - %s\n", s) - } +// writeSuggestionsSection writes the over-specification suggestions list. +func writeSuggestionsSection(w io.Writer, suggestions []string, s qualityStyles) { + if len(suggestions) > 0 { + _, _ = fmt.Fprintln(w, s.muted.Render(" Suggestions:")) + for _, sg := range suggestions { + _, _ = fmt.Fprintf(w, " - %s\n", sg) } + } +} - // Ambiguous effects — per-item list so agents can target GoDoc fixes. - if len(r.AmbiguousEffects) > 0 { - _, _ = fmt.Fprintf(w, " Ambiguous effects (excluded from metrics): %d\n", - len(r.AmbiguousEffects)) - for _, ae := range r.AmbiguousEffects { - _, _ = fmt.Fprintf(w, " - %s: %s (%s)\n", - ae.Type, ae.Description, ae.Location) - } +// writeAmbiguousEffects writes the ambiguous effects list. +func writeAmbiguousEffects(w io.Writer, effects []taxonomy.SideEffect) { + if len(effects) > 0 { + _, _ = fmt.Fprintf(w, " Ambiguous effects (excluded from metrics): %d\n", + len(effects)) + for _, ae := range effects { + _, _ = fmt.Fprintf(w, " - %s: %s (%s)\n", + ae.Type, ae.Description, ae.Location) } + } +} - // Unmapped assertions — per-item list with location, type, and reason. - if len(r.UnmappedAssertions) > 0 { - _, _ = fmt.Fprintf(w, " Unmapped assertions: %d\n", - len(r.UnmappedAssertions)) - for _, ua := range r.UnmappedAssertions { - if ua.UnmappedReason != "" { - _, _ = fmt.Fprintf(w, " - %s %s [%s]\n", - ua.AssertionLocation, ua.AssertionType, ua.UnmappedReason) - } else { - _, _ = fmt.Fprintf(w, " - %s %s\n", - ua.AssertionLocation, ua.AssertionType) - } +// writeUnmappedAssertions writes the unmapped assertions list with optional reasons. +func writeUnmappedAssertions(w io.Writer, assertions []taxonomy.AssertionMapping) { + if len(assertions) > 0 { + _, _ = fmt.Fprintf(w, " Unmapped assertions: %d\n", + len(assertions)) + for _, ua := range assertions { + if ua.UnmappedReason != "" { + _, _ = fmt.Fprintf(w, " - %s %s [%s]\n", + ua.AssertionLocation, ua.AssertionType, ua.UnmappedReason) + } else { + _, _ = fmt.Fprintf(w, " - %s %s\n", + ua.AssertionLocation, ua.AssertionType) } } } +} - // SSA diagnostics. +// writeSSADiagnostics writes SSA construction failure warnings if applicable. +func writeSSADiagnostics(w io.Writer, summary *taxonomy.PackageSummary, s qualityStyles) { if summary != nil && summary.SSADegraded && len(summary.SSADegradedPackages) > 0 { _, _ = fmt.Fprintln(w) _, _ = fmt.Fprintf(w, " %s SSA construction failed for %d package(s):\n", - warn.Render("⚠"), + s.warn.Render("⚠"), len(summary.SSADegradedPackages)) for _, pkg := range summary.SSADegradedPackages { _, _ = fmt.Fprintf(w, " - %s\n", pkg) } - _, _ = fmt.Fprintln(w, muted.Render(" Quality metrics for these packages are partial (AST-only).")) + _, _ = fmt.Fprintln(w, s.muted.Render(" Quality metrics for these packages are partial (AST-only).")) } +} - // Package summary. +// writePackageSummary writes the package-level summary footer. +func writePackageSummary(w io.Writer, summary *taxonomy.PackageSummary, s qualityStyles) { if summary != nil && summary.TotalTests > 0 { _, _ = fmt.Fprintln(w) - _, _ = fmt.Fprintln(w, header.Render("=== Package Summary ===")) + _, _ = fmt.Fprintln(w, s.header.Render("=== Package Summary ===")) _, _ = fmt.Fprintf(w, " Tests analyzed: %d\n", summary.TotalTests) _, _ = fmt.Fprintf(w, " Average contract coverage: %.0f%%\n", summary.AverageContractCoverage) @@ -171,7 +218,7 @@ func WriteText(w io.Writer, reports []taxonomy.QualityReport, summary *taxonomy. summary.AssertionDetectionConfidence) if len(summary.WorstCoverageTests) > 0 { - _, _ = fmt.Fprintln(w, muted.Render(" Lowest coverage tests:")) + _, _ = fmt.Fprintln(w, s.muted.Render(" Lowest coverage tests:")) for _, worst := range summary.WorstCoverageTests { _, _ = fmt.Fprintf(w, " - %s: %.0f%% (%d/%d)\n", worst.TestFunction, @@ -181,6 +228,4 @@ func WriteText(w io.Writer, reports []taxonomy.QualityReport, summary *taxonomy. } } } - - return nil } diff --git a/internal/quality/report_internal_test.go b/internal/quality/report_internal_test.go new file mode 100644 index 0000000..fcd935b --- /dev/null +++ b/internal/quality/report_internal_test.go @@ -0,0 +1,403 @@ +package quality + +import ( + "bytes" + "fmt" + "strings" + "testing" + + "github.com/charmbracelet/lipgloss" + "github.com/unbound-force/gaze/internal/taxonomy" +) + +// TestWriteContractCoverage_Thresholds verifies the style threshold boundaries +// for contract coverage: <50 → bad, >=50 && <80 → warn, >=80 → good. +func TestWriteContractCoverage_Thresholds(t *testing.T) { + tests := []struct { + name string + covPct float64 + wantStyle func(s qualityStyles) lipgloss.Style + }{ + {name: "49_bad", covPct: 49, wantStyle: func(s qualityStyles) lipgloss.Style { return s.bad }}, + {name: "50_warn", covPct: 50, wantStyle: func(s qualityStyles) lipgloss.Style { return s.warn }}, + {name: "79_warn", covPct: 79, wantStyle: func(s qualityStyles) lipgloss.Style { return s.warn }}, + {name: "80_good", covPct: 80, wantStyle: func(s qualityStyles) lipgloss.Style { return s.good }}, + } + + s := newQualityStyles() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + cc := taxonomy.ContractCoverage{ + Percentage: tt.covPct, + CoveredCount: 1, + TotalContractual: 2, + } + writeContractCoverage(&buf, cc, s) + out := buf.String() + if !strings.Contains(out, "Contract Coverage:") { + t.Errorf("expected 'Contract Coverage:' in output, got %q", out) + } + // Verify the correct style was applied to the percentage value. + wantRendered := tt.wantStyle(s).Render(fmt.Sprintf("%.0f%%", tt.covPct)) + if !strings.Contains(out, wantRendered) { + t.Errorf("expected styled rendering %q in output, got %q", wantRendered, out) + } + }) + } +} + +// TestWriteOverSpecification_Thresholds verifies the style threshold boundaries +// for over-specification: 0 → good, >0 → warn, >3 → bad. +func TestWriteOverSpecification_Thresholds(t *testing.T) { + tests := []struct { + name string + overCount int + wantStyle func(s qualityStyles) lipgloss.Style + }{ + {name: "0_good", overCount: 0, wantStyle: func(s qualityStyles) lipgloss.Style { return s.good }}, + {name: "1_warn", overCount: 1, wantStyle: func(s qualityStyles) lipgloss.Style { return s.warn }}, + {name: "3_warn_boundary", overCount: 3, wantStyle: func(s qualityStyles) lipgloss.Style { return s.warn }}, + {name: "4_bad", overCount: 4, wantStyle: func(s qualityStyles) lipgloss.Style { return s.bad }}, + } + + s := newQualityStyles() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + writeOverSpecification(&buf, tt.overCount, s) + out := buf.String() + if !strings.Contains(out, "Over-Specified:") { + t.Errorf("expected 'Over-Specified:' in output, got %q", out) + } + // Verify the correct style was applied to the count value. + wantRendered := tt.wantStyle(s).Render(fmt.Sprintf("%d", tt.overCount)) + if !strings.Contains(out, wantRendered) { + t.Errorf("expected styled rendering %q in output, got %q", wantRendered, out) + } + }) + } +} + +// TestWriteDetectionConfidence_Thresholds verifies the style threshold boundaries +// for detection confidence: <50 → bad, >=50 && <70 → warn, >=70 → good. +func TestWriteDetectionConfidence_Thresholds(t *testing.T) { + tests := []struct { + name string + detConf int + wantStyle func(s qualityStyles) lipgloss.Style + }{ + {name: "49_bad", detConf: 49, wantStyle: func(s qualityStyles) lipgloss.Style { return s.bad }}, + {name: "50_warn", detConf: 50, wantStyle: func(s qualityStyles) lipgloss.Style { return s.warn }}, + {name: "69_warn", detConf: 69, wantStyle: func(s qualityStyles) lipgloss.Style { return s.warn }}, + {name: "70_good", detConf: 70, wantStyle: func(s qualityStyles) lipgloss.Style { return s.good }}, + } + + s := newQualityStyles() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + writeDetectionConfidence(&buf, tt.detConf, s) + out := buf.String() + if !strings.Contains(out, "Detection Confidence:") { + t.Errorf("expected 'Detection Confidence:' in output, got %q", out) + } + // Verify the correct style was applied to the confidence value. + wantRendered := tt.wantStyle(s).Render(fmt.Sprintf("%d%%", tt.detConf)) + if !strings.Contains(out, wantRendered) { + t.Errorf("expected styled rendering %q in output, got %q", wantRendered, out) + } + }) + } +} + +// TestWriteSSADiagnostics_Rendering verifies that SSA diagnostics render +// correctly when degraded, and produce no output otherwise. +func TestWriteSSADiagnostics_Rendering(t *testing.T) { + s := newQualityStyles() + + t.Run("degraded_with_packages", func(t *testing.T) { + var buf bytes.Buffer + summary := &taxonomy.PackageSummary{ + SSADegraded: true, + SSADegradedPackages: []string{"example.com/pkg/a", "example.com/pkg/b"}, + } + writeSSADiagnostics(&buf, summary, s) + out := buf.String() + if !strings.Contains(out, "SSA construction failed") { + t.Errorf("expected 'SSA construction failed' in output, got %q", out) + } + if !strings.Contains(out, "2 package(s)") { + t.Errorf("expected '2 package(s)' in output, got %q", out) + } + if !strings.Contains(out, "example.com/pkg/a") { + t.Errorf("expected package name in output, got %q", out) + } + if !strings.Contains(out, "partial (AST-only)") { + t.Errorf("expected 'partial (AST-only)' in output, got %q", out) + } + }) + + t.Run("nil_summary", func(t *testing.T) { + var buf bytes.Buffer + writeSSADiagnostics(&buf, nil, s) + if buf.Len() != 0 { + t.Errorf("expected no output for nil summary, got %q", buf.String()) + } + }) + + t.Run("not_degraded", func(t *testing.T) { + var buf bytes.Buffer + summary := &taxonomy.PackageSummary{SSADegraded: false} + writeSSADiagnostics(&buf, summary, s) + if buf.Len() != 0 { + t.Errorf("expected no output for non-degraded summary, got %q", buf.String()) + } + }) + + t.Run("degraded_empty_packages", func(t *testing.T) { + var buf bytes.Buffer + summary := &taxonomy.PackageSummary{ + SSADegraded: true, + SSADegradedPackages: []string{}, + } + writeSSADiagnostics(&buf, summary, s) + if buf.Len() != 0 { + t.Errorf("expected no output for degraded with empty packages, got %q", buf.String()) + } + }) +} + +// TestWritePackageSummary_WorstCoverage verifies that the package summary +// renders worst coverage tests correctly. +func TestWritePackageSummary_WorstCoverage(t *testing.T) { + s := newQualityStyles() + + t.Run("with_worst_coverage", func(t *testing.T) { + var buf bytes.Buffer + summary := &taxonomy.PackageSummary{ + TotalTests: 5, + AverageContractCoverage: 65, + TotalOverSpecifications: 2, + AssertionDetectionConfidence: 80, + WorstCoverageTests: []taxonomy.QualityReport{ + { + TestFunction: "TestFoo", + ContractCoverage: taxonomy.ContractCoverage{ + Percentage: 25, + CoveredCount: 1, + TotalContractual: 4, + }, + }, + }, + } + writePackageSummary(&buf, summary, s) + out := buf.String() + if !strings.Contains(out, "Package Summary") { + t.Errorf("expected 'Package Summary' in output, got %q", out) + } + if !strings.Contains(out, "Tests analyzed: 5") { + t.Errorf("expected 'Tests analyzed: 5' in output, got %q", out) + } + if !strings.Contains(out, "Lowest coverage tests:") { + t.Errorf("expected 'Lowest coverage tests:' in output, got %q", out) + } + if !strings.Contains(out, "TestFoo") { + t.Errorf("expected 'TestFoo' in output, got %q", out) + } + if !strings.Contains(out, "25%") { + t.Errorf("expected '25%%' in output, got %q", out) + } + }) + + t.Run("nil_summary", func(t *testing.T) { + var buf bytes.Buffer + writePackageSummary(&buf, nil, s) + if buf.Len() != 0 { + t.Errorf("expected no output for nil summary, got %q", buf.String()) + } + }) + + t.Run("zero_tests", func(t *testing.T) { + var buf bytes.Buffer + summary := &taxonomy.PackageSummary{TotalTests: 0} + writePackageSummary(&buf, summary, s) + if buf.Len() != 0 { + t.Errorf("expected no output for zero tests, got %q", buf.String()) + } + }) +} + +// TestWriteGapsSection_WithoutHint verifies that a gap without a corresponding +// hint renders correctly (no hint line emitted). +func TestWriteGapsSection_WithoutHint(t *testing.T) { + s := newQualityStyles() + + t.Run("gap_without_hint", func(t *testing.T) { + var buf bytes.Buffer + cc := taxonomy.ContractCoverage{ + Gaps: []taxonomy.SideEffect{ + {Type: "ReturnValue", Description: "returns int", Location: "foo.go:10"}, + }, + GapHints: []string{""}, // empty hint + } + writeGapsSection(&buf, cc, s) + out := buf.String() + if !strings.Contains(out, "ReturnValue") { + t.Errorf("expected 'ReturnValue' in output, got %q", out) + } + if strings.Contains(out, "hint:") { + t.Errorf("expected no hint line for empty hint, got %q", out) + } + }) + + t.Run("gap_with_short_hints_slice", func(t *testing.T) { + var buf bytes.Buffer + cc := taxonomy.ContractCoverage{ + Gaps: []taxonomy.SideEffect{ + {Type: "ReturnValue", Description: "returns int", Location: "foo.go:10"}, + {Type: "ErrorReturn", Description: "returns error", Location: "foo.go:11"}, + }, + GapHints: []string{"check err"}, // shorter than Gaps + } + writeGapsSection(&buf, cc, s) + out := buf.String() + if !strings.Contains(out, "check err") { + t.Errorf("expected hint 'check err' for first gap, got %q", out) + } + // Only one hint line should appear (for the first gap only). + if strings.Count(out, "hint:") != 1 { + t.Errorf("expected exactly 1 hint line, got %d in %q", strings.Count(out, "hint:"), out) + } + }) + + t.Run("no_gaps", func(t *testing.T) { + var buf bytes.Buffer + cc := taxonomy.ContractCoverage{} + writeGapsSection(&buf, cc, s) + if buf.Len() != 0 { + t.Errorf("expected no output for no gaps, got %q", buf.String()) + } + }) +} + +// TestWriteDiscardedReturns_WithoutHint verifies that a discarded return +// without a corresponding hint renders correctly (no hint line emitted). +func TestWriteDiscardedReturns_WithoutHint(t *testing.T) { + s := newQualityStyles() + + t.Run("discarded_without_hint", func(t *testing.T) { + var buf bytes.Buffer + cc := taxonomy.ContractCoverage{ + DiscardedReturns: []taxonomy.SideEffect{ + {Type: "ReturnValue", Description: "returns int", Location: "foo.go:10"}, + }, + DiscardedReturnHints: []string{""}, // empty hint + } + writeDiscardedReturns(&buf, cc, s) + out := buf.String() + if !strings.Contains(out, "ReturnValue") { + t.Errorf("expected 'ReturnValue' in output, got %q", out) + } + if strings.Contains(out, "hint:") { + t.Errorf("expected no hint line for empty hint, got %q", out) + } + }) + + t.Run("no_discarded", func(t *testing.T) { + var buf bytes.Buffer + cc := taxonomy.ContractCoverage{} + writeDiscardedReturns(&buf, cc, s) + if buf.Len() != 0 { + t.Errorf("expected no output for no discarded returns, got %q", buf.String()) + } + }) +} + +// TestWriteUnmappedAssertions_WithoutReason verifies that an unmapped assertion +// without a reason renders correctly (no reason suffix). +func TestWriteUnmappedAssertions_WithoutReason(t *testing.T) { + t.Run("without_reason", func(t *testing.T) { + var buf bytes.Buffer + assertions := []taxonomy.AssertionMapping{ + { + AssertionLocation: "foo_test.go:10", + AssertionType: "equality", + UnmappedReason: "", + }, + } + writeUnmappedAssertions(&buf, assertions) + out := buf.String() + if !strings.Contains(out, "foo_test.go:10") { + t.Errorf("expected location in output, got %q", out) + } + if strings.Contains(out, "[") { + t.Errorf("expected no reason bracket for empty reason, got %q", out) + } + }) + + t.Run("with_reason", func(t *testing.T) { + var buf bytes.Buffer + assertions := []taxonomy.AssertionMapping{ + { + AssertionLocation: "foo_test.go:10", + AssertionType: "equality", + UnmappedReason: "no matching effect", + }, + } + writeUnmappedAssertions(&buf, assertions) + out := buf.String() + if !strings.Contains(out, "[no matching effect]") { + t.Errorf("expected '[no matching effect]' in output, got %q", out) + } + }) + + t.Run("no_assertions", func(t *testing.T) { + var buf bytes.Buffer + writeUnmappedAssertions(&buf, nil) + if buf.Len() != 0 { + t.Errorf("expected no output for nil assertions, got %q", buf.String()) + } + }) +} + +// TestWriteText_MultiReportSeparator verifies that a blank line separator +// appears between multiple reports. +func TestWriteText_MultiReportSeparator(t *testing.T) { + reports := []taxonomy.QualityReport{ + { + TestFunction: "TestA", + TargetFunction: taxonomy.FunctionTarget{ + Function: "A", + Package: "pkg", + }, + ContractCoverage: taxonomy.ContractCoverage{Percentage: 100}, + }, + { + TestFunction: "TestB", + TargetFunction: taxonomy.FunctionTarget{ + Function: "B", + Package: "pkg", + }, + ContractCoverage: taxonomy.ContractCoverage{Percentage: 100}, + }, + } + + var buf bytes.Buffer + if err := WriteText(&buf, reports, nil); err != nil { + t.Fatalf("WriteText() error = %v", err) + } + out := buf.String() + + // The separator between reports is a blank line (two consecutive newlines + // with only whitespace between them). + testAIdx := strings.Index(out, "TestA") + testBIdx := strings.Index(out, "TestB") + if testAIdx < 0 || testBIdx < 0 { + t.Fatalf("expected both TestA and TestB in output, got %q", out) + } + between := out[testAIdx:testBIdx] + if !strings.Contains(between, "\n\n") { + t.Errorf("expected blank line separator between reports, got %q", between) + } +} diff --git a/openspec/changes/quality-pipeline-tests/.openspec.yaml b/openspec/changes/quality-pipeline-tests/.openspec.yaml new file mode 100644 index 0000000..8a32ad5 --- /dev/null +++ b/openspec/changes/quality-pipeline-tests/.openspec.yaml @@ -0,0 +1,2 @@ +schema: unbound-force +created: 2026-07-29 diff --git a/openspec/changes/quality-pipeline-tests/design.md b/openspec/changes/quality-pipeline-tests/design.md new file mode 100644 index 0000000..436d80f --- /dev/null +++ b/openspec/changes/quality-pipeline-tests/design.md @@ -0,0 +1,106 @@ +## Context + +Three functions in `internal/quality/` have elevated CRAP scores (issue #198): + +- `WriteText` (CC=32, CRAP=39.9) — monolithic report renderer with 11 distinct output sections, each with its own conditional logic and style selection. +- `traceForwardDataFlow` (CC=32, CRAP=32.4) — deeply nested AST walker with three distinct concerns (RHS reference checking, transformation call handling, LHS extraction) interleaved in a single function. +- `generateSuggestion` (CC=6, 0% direct coverage) — small switch statement with no direct tests. + +The crap package already established the decomposition pattern in `crap/compare_report.go` (extracted `writeScoreTable`, `writeSummarySection`, `writeQuadrantSection`, etc.) and in `crap/report.go` (extracted 6 section helpers). The quality package's `mapping.go` was partially decomposed in Phase 2b (#166) — `matchContainerUnwrap` dropped from CC=50 to CC=8, but `traceForwardDataFlow` absorbed the complexity at CC=32. + +This design follows the same helper-extraction pattern, adapting it to the quality package's simpler style system. + +## Goals / Non-Goals + +### Goals + +- Reduce `WriteText` CC from 32 to ≤ 5 +- Reduce `traceForwardDataFlow` CC from 32 to ≤ 12 +- Add direct unit tests for `generateSuggestion` (all 6 branches) +- Add targeted tests for extracted helpers covering previously untested paths +- All new tests run without `testing.Short()` guards (synthetic data only) + +### Non-Goals + +- Reusing the shared `report.Styles` struct from `internal/report/` — the quality report uses a simpler 5-style palette (header/good/warn/bad/muted) vs the crap report's 20+ field struct. Adding quality-specific fields to the shared struct would bloat it for all consumers. +- Changing `WriteText` output format — byte-for-byte identical output after decomposition. +- Decomposing `WriteJSON` — it is CC=1 (delegates to `json.NewEncoder`). +- Further decomposing helpers extracted from `traceForwardDataFlow` — the extracted helpers will have CC ≤ 12 each, which is acceptable. + +## Decisions + +### D1: Local `qualityStyles` struct + +Define a package-local `qualityStyles` struct in `report.go` bundling the 5 lipgloss styles currently created as local variables in `WriteText`: + +```go +type qualityStyles struct { + header lipgloss.Style + good lipgloss.Style + warn lipgloss.Style + bad lipgloss.Style + muted lipgloss.Style +} +``` + +**Rationale**: The `crap/report.go` pattern uses `report.Styles` (shared), but the quality report's palette is fundamentally different (good/warn/bad threshold coloring vs tier-based coloring). A local struct keeps the styles co-located with their consumers and avoids coupling to the crap report's style system. + +### D2: 11 section helpers for `WriteText` + +Each helper follows the signature pattern `func writeXxx(w io.Writer, ..., s qualityStyles)` and handles one logical output section: + +| Helper | Lines | CC | Responsibility | +|--------|-------|----|----------------| +| `writeReportHeader` | 44-51 | 1 | `=== Test -> Target ===` header | +| `writeContractCoverage` | 53-64 | 3 | Coverage percentage with threshold coloring | +| `writeOverSpecification` | 66-76 | 3 | Over-spec count with threshold coloring | +| `writeDetectionConfidence` | 78-88 | 3 | Confidence percentage with threshold coloring | +| `writeGapsSection` | 90-101 | 5 | Gap list with hints | +| `writeDiscardedReturns` | 103-113 | 5 | Discarded returns with hints | +| `writeSuggestionsSection` | 115-121 | 3 | Over-spec suggestions | +| `writeAmbiguousEffects` | 123-131 | 3 | Ambiguous effects list | +| `writeUnmappedAssertions` | 133-146 | 4 | Unmapped assertions with reasons | +| `writeSSADiagnostics` | 149-159 | 6 | SSA degradation warnings | +| `writePackageSummary` | 161-183 | 6 | Package-level summary footer | + +After extraction, `WriteText` becomes a ~20-line orchestrator (CC=3): style initialization, report loop with separator, then two trailing section calls. + +### D3: 3 helpers for `traceForwardDataFlow` + +Extract three concern-specific helpers: + +1. **`rhsReferencesAnyTracked`** — checks whether an RHS expression references any tracked variable (direct `containsObject` check + `resolveExprRoot` fallback). Absorbs lines 998-1023. +2. **`handleTransformationCalls`** — handles the transformation call detection and pointer destination extraction. Absorbs the inner `ast.Inspect` closure at lines 1029-1063. +3. **`extractDataFlowLHS`** — handles the non-transformation LHS extraction with `isDataExtraction` gating. Absorbs lines 1069-1090. + +After extraction, `traceForwardDataFlow` becomes an iteration loop that calls these three helpers sequentially per RHS element, with convergence checking. Target CC: ~10. + +### D4: Test file placement + +| Tests for | File | Package | +|-----------|------|---------| +| `generateSuggestion` | `container_unwrap_internal_test.go` | `quality` (internal) | +| `traceForwardDataFlow` helpers | `container_unwrap_internal_test.go` | `quality` (internal) | +| `WriteText` section helpers | `report_internal_test.go` (new) | `quality` (internal) | + +**Rationale**: The extracted helpers are all unexported, requiring internal package tests. `container_unwrap_internal_test.go` already contains tests for mapping helpers and has the `parseAndTypeCheck` infrastructure. Report helpers need a separate file because they test formatting output (different concern, different test patterns). + +### D5: Test patterns + +- **`generateSuggestion`**: Table-driven with 6 cases (5 switch arms + default), asserting both `strings.Contains` for key phrases and `strings.HasPrefix` for format consistency. +- **Report helpers**: Construct synthetic `taxonomy.QualityReport` structs, call each helper, assert output contains expected strings. Focus on threshold boundary values (49/50/79/80 for coverage, 0/1/4 for over-spec, 49/50/69/70 for confidence). +- **Data flow helpers**: Use `parseAndTypeCheck` to create synthetic AST from Go source strings, following the established pattern in `container_unwrap_internal_test.go`. + +## Risks / Trade-offs + +### R1: Increased function count + +Adding 14 new functions (11 + 3 helpers) increases the function count in `report.go` and `mapping.go`. This is an accepted trade-off — each function is small, focused, and independently testable. The `crap/report.go` decomposition (6 helpers) and `crap/compare_report.go` decomposition (6 helpers) established this pattern as project convention. + +### R2: `traceForwardDataFlow` may not reach CC ≤ 12 + +The Dewey learning from Phase 2b notes: "traceForwardDataFlow landed at complexity 32 despite the design estimating ~12." The outer iteration loop, AST inspection, and convergence check contribute inherent complexity. After extracting 3 helpers, the residual CC depends on how much branching remains in the orchestrator. If CC lands at ~10-12, that is acceptable. If it lands higher, we accept the result — the value is in making each concern independently testable, not in hitting an exact CC target. + +### R3: Style threshold tests are fragile to format changes + +Tests that assert on specific output strings (e.g., checking that coverage renders with a specific color code) will break if the output format changes. This is acceptable — the tests catch unintended format regressions, and intentional changes should update the tests. diff --git a/openspec/changes/quality-pipeline-tests/proposal.md b/openspec/changes/quality-pipeline-tests/proposal.md new file mode 100644 index 0000000..861d3c1 --- /dev/null +++ b/openspec/changes/quality-pipeline-tests/proposal.md @@ -0,0 +1,78 @@ +## Why + +Three functions in `internal/quality/` have high CRAP scores due to a combination of high cyclomatic complexity and incomplete test coverage. Issue [#198](https://github.com/unbound-force/gaze/issues/198) tracks them: + +| Function | File | CC | CRAP | Coverage | +|----------|------|----|------|----------| +| `WriteText` | `report.go:31-186` | 32 | 39.9 | 80.3% | +| `traceForwardDataFlow` | `mapping.go:980-1107` | 32 | 32.4 | 92.8% | +| `generateSuggestion` | `overspec.go:71-95` | 6 | 42 | 0% direct | + +`WriteText` and `traceForwardDataFlow` are too complex to test thoroughly as monoliths — decomposing them into focused helpers makes each piece independently testable. `generateSuggestion` is small but has zero direct test coverage (only indirect through `ComputeOverSpecification`). + +This continues the CRAPload reduction effort from issues #166 (phases 2a/2b/2c). + +## What Changes + +### Decomposition + +1. **`WriteText`** (CC 32 → 3): Extract 11 section-rendering helpers (`writeReportHeader`, `writeContractCoverage`, `writeOverSpecification`, `writeDetectionConfidence`, `writeGapsSection`, `writeDiscardedReturns`, `writeSuggestionsSection`, `writeAmbiguousEffects`, `writeUnmappedAssertions`, `writeSSADiagnostics`, `writePackageSummary`). Define a local `qualityStyles` struct to bundle the 5 lipgloss styles. + +2. **`traceForwardDataFlow`** (CC 32 → ~10): Extract 3 helpers (`rhsReferencesAnyTracked`, `handleTransformationCalls`, `extractDataFlowLHS`). + +### New Tests + +3. **`generateSuggestion`**: Table-driven test covering all 5 switch cases plus the default fallback. + +4. **Extracted helper tests**: Style threshold boundary tests, SSA diagnostics rendering, package summary rendering, transformation call bridging, multi-iteration convergence. + +## Capabilities + +### New Capabilities + +- None — this is a chore (internal code quality improvement). + +### Modified Capabilities + +- `WriteText`: Identical output behavior, reduced CC from 32 to 3 via helper extraction. +- `traceForwardDataFlow`: Identical behavior, reduced CC from 32 to ~10 via helper extraction. + +### Removed Capabilities + +- None. + +## Impact + +- **Files modified**: `internal/quality/report.go`, `internal/quality/mapping.go` +- **Files added**: `internal/quality/report_internal_test.go` (new test file for `WriteText` helpers) +- **Files modified (tests)**: `internal/quality/container_unwrap_internal_test.go` (additional tests for `generateSuggestion` and `traceForwardDataFlow` helpers) +- **No API surface changes**: All extracted helpers are unexported. `WriteText` and `traceForwardDataFlow` signatures and behavior are unchanged. +- **No behavioral changes**: Pure refactoring + test additions. + +## Constitution Alignment + +Assessed against the Gaze project constitution (`.specify/memory/constitution.md` v1.3.0). + +### I. Accuracy + +**Assessment**: PASS + +Decomposition extracts helpers without changing any analysis logic. All existing tests pass unchanged, confirming output equivalence. New tests increase coverage of previously untested paths (SSA diagnostics rendering, style threshold boundaries, transformation call bridging), reducing the risk of latent inaccuracies. + +### II. Minimal Assumptions + +**Assessment**: N/A + +No new assumptions introduced. The change is internal refactoring of report formatting and data flow tracing — no changes to how host projects are analyzed. + +### III. Actionable Output + +**Assessment**: PASS + +Report output is byte-for-byte identical after decomposition. The extracted helpers make each output section independently testable, improving confidence that report output remains correct as the codebase evolves. + +### IV. Testability + +**Assessment**: PASS + +This change directly improves testability — it decomposes two CC=32 functions into independently testable helpers (all CC ≤ 6) and adds targeted tests for previously uncovered paths. Coverage strategy: unit tests using synthetic data (no `testing.Short()` guards needed). The `parseAndTypeCheck` pattern from `container_unwrap_internal_test.go` is reused for AST-level tests. diff --git a/openspec/changes/quality-pipeline-tests/specs/decomposition.md b/openspec/changes/quality-pipeline-tests/specs/decomposition.md new file mode 100644 index 0000000..846988c --- /dev/null +++ b/openspec/changes/quality-pipeline-tests/specs/decomposition.md @@ -0,0 +1,106 @@ +## ADDED Requirements + +### Requirement: WriteText Helper Extraction + +`WriteText` in `internal/quality/report.go` MUST be decomposed into section-rendering helpers. Each helper MUST have cyclomatic complexity ≤ 6. The resulting `WriteText` function MUST have cyclomatic complexity ≤ 5. The output of `WriteText` MUST remain byte-for-byte identical after decomposition. + +A local `qualityStyles` struct MUST bundle the 5 lipgloss styles (header, good, warn, bad, muted). Each helper MUST accept an `io.Writer` and the data it needs, plus the `qualityStyles` struct. + +#### Scenario: WriteText output equivalence + +- **GIVEN** a set of `taxonomy.QualityReport` values and a `*taxonomy.PackageSummary` +- **WHEN** `WriteText` is called before and after decomposition +- **THEN** the output bytes MUST be identical + +#### Scenario: Style threshold boundary for contract coverage + +- **GIVEN** a `QualityReport` with `ContractCoverage.Percentage` at boundary values (49, 50, 79, 80) +- **WHEN** `writeContractCoverage` renders the coverage +- **THEN** values < 50 MUST use the `bad` style, values >= 50 and < 80 MUST use `warn`, values >= 80 MUST use `good` + +#### Scenario: Style threshold boundary for over-specification + +- **GIVEN** a `QualityReport` with `OverSpecification.Count` at boundary values (0, 1, 3, 4) +- **WHEN** `writeOverSpecification` renders the count +- **THEN** value 0 MUST use the `good` style, values > 0 and <= 3 MUST use `warn`, values > 3 MUST use `bad` + +#### Scenario: Style threshold boundary for detection confidence + +- **GIVEN** a `QualityReport` with `AssertionDetectionConfidence` at boundary values (49, 50, 69, 70) +- **WHEN** `writeDetectionConfidence` renders the confidence +- **THEN** values < 50 MUST use the `bad` style, values >= 50 and < 70 MUST use `warn`, values >= 70 MUST use `good` + +#### Scenario: SSA diagnostics rendering + +- **GIVEN** a `PackageSummary` with `SSADegraded=true` and `SSADegradedPackages=["pkg/a", "pkg/b"]` +- **WHEN** `writeSSADiagnostics` renders the section +- **THEN** the output MUST contain the warning indicator, the package count, and each package name + +#### Scenario: Package summary with worst coverage + +- **GIVEN** a `PackageSummary` with `TotalTests > 0` and populated `WorstCoverageTests` +- **WHEN** `writePackageSummary` renders the section +- **THEN** the output MUST contain the "Lowest coverage tests" sub-section with each test name and coverage percentage + +#### Scenario: Multi-report separator + +- **GIVEN** two `QualityReport` values +- **WHEN** `WriteText` renders both reports +- **THEN** a blank line MUST separate the two reports + +### Requirement: traceForwardDataFlow Helper Extraction + +`traceForwardDataFlow` in `internal/quality/mapping.go` MUST be decomposed into concern-specific helpers. The resulting `traceForwardDataFlow` function SHOULD have cyclomatic complexity ≤ 12. + +Three helpers MUST be extracted: +- `rhsReferencesAnyTracked`: MUST check whether an RHS expression references any tracked variable via direct identity and `resolveExprRoot` fallback. +- `handleTransformationCalls`: MUST detect transformation calls in an RHS expression and extract the pointer destination as a new tracked variable. +- `extractDataFlowLHS`: MUST extract the LHS variable from a non-transformation assignment, gated on `isDataExtraction`. + +#### Scenario: RHS reference detection via resolveExprRoot + +- **GIVEN** an assignment `x := result.Field.SubField` where `result` is tracked +- **WHEN** `rhsReferencesAnyTracked` checks the RHS +- **THEN** it MUST return true via the `resolveExprRoot` fallback path + +#### Scenario: Transformation call bridging + +- **GIVEN** an assignment containing `json.Unmarshal(data, &target)` where `data` is tracked +- **WHEN** `handleTransformationCalls` processes the RHS +- **THEN** it MUST return `target` as a new tracked variable + +#### Scenario: Non-data-extraction gating + +- **GIVEN** an assignment `got := s.Get("key")` where `s` is tracked +- **WHEN** `extractDataFlowLHS` processes the assignment +- **THEN** it MUST return nil (method call is not a data extraction) + +#### Scenario: Multi-iteration convergence + +- **GIVEN** a chain `a := result.Data; b := a.Items[0]; c := b.Name` where `result` is tracked +- **WHEN** `traceForwardDataFlow` processes with `maxContainerChainDepth >= 3` +- **THEN** `a`, `b`, and `c` MUST all be in the tracked set after convergence + +### Requirement: generateSuggestion Direct Tests + +`generateSuggestion` in `internal/quality/overspec.go` MUST have direct unit tests covering all 6 branches (5 switch cases + default). + +#### Scenario: Each switch case produces a type-specific suggestion + +- **GIVEN** each of the 5 effect types: `LogWrite`, `StdoutWrite`, `GoroutineSpawn`, `ContextCancellation`, `CallbackInvocation` +- **WHEN** `generateSuggestion` is called with that type and a description +- **THEN** the returned string MUST contain the description AND MUST contain type-specific guidance text + +#### Scenario: Default case produces generic suggestion + +- **GIVEN** an effect type not in the 5 enumerated cases (e.g., `MapMutation`) +- **WHEN** `generateSuggestion` is called +- **THEN** the returned string MUST contain both the effect type name and the description + +## MODIFIED Requirements + +None. + +## REMOVED Requirements + +None. diff --git a/openspec/changes/quality-pipeline-tests/tasks.md b/openspec/changes/quality-pipeline-tests/tasks.md new file mode 100644 index 0000000..e92a95d --- /dev/null +++ b/openspec/changes/quality-pipeline-tests/tasks.md @@ -0,0 +1,64 @@ + + +## 1. `generateSuggestion` Direct Tests + +- [x] 1.1 [P] Add `TestGenerateSuggestion` table-driven test in `internal/quality/container_unwrap_internal_test.go`. Include 6 cases: `LogWrite` (assert "log output" and "implementation detail"), `StdoutWrite` (assert "stdout"), `GoroutineSpawn` (assert "goroutine lifecycle" and "concurrency detail"), `ContextCancellation` (assert "context usage"), `CallbackInvocation` (assert "callback invocation"), and a default case using `MapMutation` (assert contains both the type name and description). Each case MUST verify the description appears in the output. + +## 2. `WriteText` Decomposition + +- [x] 2.1 Define `qualityStyles` struct in `internal/quality/report.go` with 5 fields: `header`, `good`, `warn`, `bad`, `muted` (all `lipgloss.Style`). Add a `newQualityStyles()` constructor that returns the struct with the same style definitions currently inline in `WriteText` (lines 33-37). +- [x] 2.2 Extract 11 section helpers in `internal/quality/report.go`. Each helper takes `(w io.Writer, ..., s qualityStyles)` and handles one output section. Move lines from `WriteText` into each helper without changing output. Helpers: `writeReportHeader` (lines 44-51), `writeContractCoverage` (lines 53-64), `writeOverSpecification` (lines 66-76), `writeDetectionConfidence` (lines 78-88), `writeGapsSection` (lines 90-101), `writeDiscardedReturns` (lines 103-113), `writeSuggestionsSection` (lines 115-121), `writeAmbiguousEffects` (lines 123-131), `writeUnmappedAssertions` (lines 133-146), `writeSSADiagnostics` (lines 149-159), `writePackageSummary` (lines 161-183). +- [x] 2.3 Rewrite `WriteText` as an orchestrator: call `newQualityStyles()`, loop over reports with separator, delegate to the 11 helpers. Verify all existing `TestWriteText_*` tests pass unchanged. +- [x] 2.4 Add tests in new file `internal/quality/report_internal_test.go` (package `quality`): + - `TestWriteContractCoverage_Thresholds`: table-driven with boundary values 49 (bad), 50 (warn), 79 (warn), 80 (good). + - `TestWriteOverSpecification_Thresholds`: table-driven with values 0 (good), 1 (warn), 3 (warn — boundary), 4 (bad). + - `TestWriteDetectionConfidence_Thresholds`: table-driven with values 49 (bad), 50 (warn), 69 (warn), 70 (good). + - `TestWriteSSADiagnostics_Rendering`: verify SSA warning text, package count, and each package name appear in output. Also test the nil/non-degraded case produces no output. + - `TestWritePackageSummary_WithWorstCoverage`: verify "Lowest coverage tests" section renders with test names and percentages. Also test nil summary and zero-tests cases produce no output. + - `TestWriteGapsSection_WithoutHint`: verify a gap renders without a hint line when `GapHints` is shorter than `Gaps`. + - `TestWriteDiscardedReturns_WithoutHint`: verify a discarded return renders without a hint line when `DiscardedReturnHints` is shorter than `DiscardedReturns` (mirrors `writeGapsSection` conditional hint logic). + - `TestWriteUnmappedAssertions_WithoutReason`: verify an assertion with empty `UnmappedReason` renders without the `[reason]` suffix. + - `TestWriteText_MultiReport`: verify a blank line separator appears between two reports. + +## 3. `traceForwardDataFlow` Decomposition + +- [x] 3.1 Extract `rhsReferencesAnyTracked(rhs ast.Expr, tracked map[types.Object]bool, info *types.Info) bool` in `internal/quality/mapping.go`. Move lines 998-1023 (the direct `containsObject` loop + `resolveExprRoot` fallback) into this helper. +- [x] 3.2 Extract `handleTransformationCalls(rhs ast.Expr, tracked map[types.Object]bool, info *types.Info) (dest types.Object, handled bool)` in `internal/quality/mapping.go`. Move lines 1028-1063 (the inner `ast.Inspect` closure for transformation call detection) into this helper. Return the extracted pointer destination and whether a transformation was handled. +- [x] 3.3 Extract `extractDataFlowLHS(assign *ast.AssignStmt, rhsIdx int, rhs ast.Expr, info *types.Info) types.Object` in `internal/quality/mapping.go`. Move lines 1069-1090 (the `isDataExtraction` gate + LHS ident extraction) into this helper. Return the extracted object or nil. +- [x] 3.4 Rewrite `traceForwardDataFlow` to call the 3 helpers sequentially per RHS element. Verify all existing `TestTraceForwardDataFlow_*` tests pass unchanged. +- [x] 3.5 Add tests in `internal/quality/container_unwrap_internal_test.go`: + - `TestRhsReferencesAnyTracked_DirectMatch`: verify direct `containsObject` path returns true. + - `TestRhsReferencesAnyTracked_ResolveExprRootFallback`: verify `resolveExprRoot` path returns true for `result.Field.SubField` where `result` is tracked. + - `TestRhsReferencesAnyTracked_NoMatch`: verify false when no tracked variable is referenced. + - `TestHandleTransformationCalls_JsonUnmarshal`: verify pointer destination extraction for `json.Unmarshal(data, &target)` pattern where `data` is tracked. Use `parseAndTypeCheck` with synthetic source. + - `TestHandleTransformationCalls_NoTrackedArg`: verify `handled=false` when no tracked variable flows into the call arguments. + - `TestHandleTransformationCalls_NonTransform`: verify `handled=false` for a regular function call that is not a transformation. + - `TestExtractDataFlowLHS_FieldAccess`: verify LHS extraction for `x := result.Field` (data extraction). + - `TestExtractDataFlowLHS_MethodCall`: verify nil return for `got := s.Get("key")` (not data extraction). + - `TestTraceForwardDataFlow_MultiIteration`: verify chain `a := result.Data; b := a.Items[0]; c := b.Name` tracks all three variables across multiple iterations. + +## 4. Verification + +- [x] 4.1 Run `go test -race -count=1 -short ./internal/quality/...` — all tests MUST pass. +- [x] 4.2 Run `golangci-lint run` — zero issues. +- [x] 4.3 Verify existing `TestWriteText_*` tests produce identical output (no behavioral change). +- [x] 4.4 Verify existing `TestTraceForwardDataFlow_*` tests pass unchanged. + + +