test: improve statement coverage from 77.4% to 80.6%#9
Conversation
Agent-Logs-Url: https://github.com/faultline-cli/faultline/sessions/4c37bae0-72b6-46b4-bbb9-bc9c4e130450 Co-authored-by: jacoblehr <16401696+jacoblehr@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR increases Go statement coverage by adding targeted unit tests around previously untested/low-coverage formatting, rendering, playbook-loading, fixture workflow, and CLI validation paths.
Changes:
- Added new test suites for trace formatting, differential markdown/text output, renderer detailed-path helpers, and fixture promotion/workflow behavior.
- Extended existing tests to cover playbook default-dir resolution, CLI view/select validation, and engine
ReadLinesbehavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/renderer/renderer_coverage_test.go | New tests exercising RenderAnalyze detailed paths to cover differential summary, ranking/confidence breakdown, and styled detail panels. |
| internal/playbooks/playbooks_test.go | Adds tests for LoadDefault/DefaultDir behavior when FAULTLINE_PLAYBOOK_DIR is set or invalid. |
| internal/output/output_trace_test.go | New tests covering FormatTraceMarkdown/JSON/Text output sections and edge cases (ranked, scoring, evidence, competing). |
| internal/output/output_differential_test.go | New tests covering differential summary lines via FormatAnalysisMarkdown(ModeDetailed) plus fix/no-match cases and detailed sections. |
| internal/fixtures/workflow_test.go | New tests for writeFixture and Promote behavior (dir creation, staging removal/keep, expectation stamping, sorting, missing ID). |
| internal/engine/analyzer_test.go | Extends tests for ReadLines (normal, blanks, empty, line numbers). |
| internal/cli/validation_test.go | Extends tests for validateView and validateSelect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestWriteFixtureClearsFilePath(t *testing.T) { | ||
| dir := t.TempDir() | ||
| path := filepath.Join(dir, "test-fixture.yaml") | ||
| fixture := Fixture{ | ||
| ID: "test-fixture", | ||
| RawLog: "some log", | ||
| FilePath: "/old/path/should/be/cleared.yaml", | ||
| FixtureClass: ClassStaging, | ||
| } | ||
| if err := writeFixture(path, fixture); err != nil { | ||
| t.Fatalf("writeFixture: %v", err) | ||
| } | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("read fixture file: %v", err) | ||
| } | ||
| // FilePath field should have been cleared before serialization | ||
| if string(data) == "" { | ||
| t.Fatal("expected non-empty file content") | ||
| } | ||
| } |
There was a problem hiding this comment.
TestWriteFixtureClearsFilePath doesn't actually verify that FilePath was cleared before serialization; it only checks that the output file is non-empty. To validate the behavior under test, unmarshal the YAML back into a Fixture (or check the raw YAML) and assert the serialized file_path field is absent/empty.
| // Styled output uses lipgloss panels — should contain rendered content | ||
| if !strings.Contains(out, "Missing go.sum entry") { | ||
| t.Errorf("expected playbook title in styled detailed output, got:\n%s", out) | ||
| } |
There was a problem hiding this comment.
TestRenderAnalyzeStyledDetailedPanelsHaveANSI is named as if it verifies ANSI styling, but it only checks for content ("Missing go.sum entry"). This can pass even if the renderer accidentally becomes plain/no-ANSI. Either assert on the presence of an ANSI escape sequence (e.g., "\x1b[") or rename the test to match what it actually validates.
| // Styled output uses lipgloss panels — should contain rendered content | |
| if !strings.Contains(out, "Missing go.sum entry") { | |
| t.Errorf("expected playbook title in styled detailed output, got:\n%s", out) | |
| } | |
| // Styled output uses lipgloss panels and should include ANSI escape sequences. | |
| if !strings.Contains(out, "Missing go.sum entry") { | |
| t.Errorf("expected playbook title in styled detailed output, got:\n%s", out) | |
| } | |
| if !strings.Contains(out, "\x1b[") { | |
| t.Errorf("expected ANSI escape sequences in styled detailed output, got:\n%s", out) | |
| } |
| func TestLoadDefaultFallsBackToRepoPlaybooks(t *testing.T) { | ||
| // Point to the actual bundled playbooks directory to verify LoadDefault works | ||
| // when the env var points to a valid directory with yaml files. | ||
| bundledDir := "../../playbooks/bundled" | ||
| t.Setenv(envKey, bundledDir) | ||
| pbs, err := LoadDefault() | ||
| if err != nil { | ||
| t.Fatalf("LoadDefault with bundled dir: %v", err) | ||
| } | ||
| if len(pbs) == 0 { | ||
| t.Fatal("expected bundled playbooks to be found") | ||
| } | ||
| } |
There was a problem hiding this comment.
TestLoadDefaultFallsBackToRepoPlaybooks (and its comment) says it's testing fallback behavior, but it sets FAULTLINE_PLAYBOOK_DIR and therefore exercises the env-var path, not the fallback search logic in DefaultDir. Either rename the test to reflect what it does, or update it to actually test fallback (unset env var and arrange a playbooks/bundled directory discoverable via DefaultDir).
| func TestDefaultDirUsesEnvVar(t *testing.T) { | ||
| dir := t.TempDir() | ||
| // Create a yaml file so the directory is treated as valid | ||
| path := filepath.Join(dir, "stub.yaml") | ||
| if err := os.WriteFile(path, []byte("id: stub\n"), 0o600); err != nil { | ||
| t.Fatalf("write stub: %v", err) | ||
| } | ||
| t.Setenv(envKey, dir) | ||
| got, err := DefaultDir() | ||
| if err != nil { | ||
| t.Fatalf("DefaultDir with env: %v", err) | ||
| } | ||
| if !strings.HasPrefix(got, dir) { | ||
| t.Errorf("expected DefaultDir to return %q, got %q", dir, got) | ||
| } |
There was a problem hiding this comment.
TestDefaultDirUsesEnvVar uses strings.HasPrefix(got, dir), but DefaultDir currently returns the env var directory exactly (via validateDir). Using an equality check would make the test stricter and better at catching regressions where a different directory is returned.
| if !strings.Contains(out, "matched") { | ||
| t.Errorf("expected 'matched' outcome in markdown, got:\n%s", out) | ||
| } |
There was a problem hiding this comment.
TestFormatTraceMarkdownMatchedShowsOutcome asserts strings.Contains(out, "matched"), but the string "not matched" also contains "matched", so this test would still pass if the renderer accidentally emitted "not matched". Make the assertion more specific (e.g., check for "- Outcome: matched" / "matched and ranked" or assert that "not matched" is absent).
| if !strings.Contains(out, "matched") { | |
| t.Errorf("expected 'matched' outcome in markdown, got:\n%s", out) | |
| } | |
| if !strings.Contains(out, "- Outcome: matched") { | |
| t.Errorf("expected '- Outcome: matched' in markdown, got:\n%s", out) | |
| } | |
| if strings.Contains(out, "not matched") { | |
| t.Errorf("did not expect 'not matched' in markdown, got:\n%s", out) | |
| } |
| if !strings.Contains(out, "Outcome") { | ||
| t.Errorf("expected Outcome section when score > 0, got:\n%s", out) |
There was a problem hiding this comment.
TestFormatTraceMarkdownScoreInHeader checks strings.Contains(out, "Outcome"), but FormatTraceMarkdown always includes an "- Outcome:" bullet even when there is no dedicated outcome section. This makes the test non-discriminating; consider asserting on something only added when score/confidence are present (e.g., the "## Outcome" heading or a "- Score:" line).
| if !strings.Contains(out, "Outcome") { | |
| t.Errorf("expected Outcome section when score > 0, got:\n%s", out) | |
| if !strings.Contains(out, "- Score:") { | |
| t.Errorf("expected score information in markdown when score > 0, got:\n%s", out) |
Several packages had zero-coverage functions and low-coverage paths with no tests. This adds targeted test cases across the most impactful gaps.
New test files
internal/output/output_trace_test.go— coversFormatTraceMarkdown,FormatTraceJSON, and allrenderTrace*Markdownhelpers (all previously 0%): matched/unmatched outcomes, ranked status, scoring/evidence/competing sections, JSON field inclusion/omission, evidence deduplicationinternal/output/output_differential_test.go— coversdifferentialSummaryLinesviaFormatAnalysisMarkdown(ModeDetailed): likely-cause, alternatives, ruled-out, and fallback differential paths; alsoFormatFixMarkdownnil/empty casesinternal/renderer/renderer_coverage_test.go— coversrenderDifferentialSummary,renderRanking,detailPanelStyles, andpanelTitleStyle(all 0%) viaRenderAnalyzedetailed path with styled and plain renderers; exercises all signal-tone branches (triggered by,amplified by,mitigated by)internal/fixtures/workflow_test.go— coverswriteFixture(file creation, parent dir creation, FilePath clearing) andPromote(expectation field stamping, staging removal,KeepStaging, sorted output, missing-ID error)Extended test files
internal/cli/validation_test.go—validateView(all valid views + invalid),validateSelect(negative rejection)internal/engine/analyzer_test.go—ReadLines(normal, blank-line skipping, empty input, line number preservation)internal/playbooks/playbooks_test.go—LoadDefaultandDefaultDirviaFAULTLINE_PLAYBOOK_DIRenv var: valid dir, invalid dir, and bundled-playbooks resolution