diff --git a/.claude/skills/test/SKILL.md b/.claude/skills/test/SKILL.md new file mode 100644 index 0000000..2050d2d --- /dev/null +++ b/.claude/skills/test/SKILL.md @@ -0,0 +1,347 @@ +--- +name: test +description: Write rigorous unit + golden tests for the optiqor CLI. Table-driven by default; named TestFunctionName_Scenario_ExpectedBehavior; stdlib only (no testify); race detector always; time + randomness injected; t.Helper() in helpers; golden snapshots for every renderer / CLI output; YAML fixtures live in testdata/fixtures/. Use when the user says "test this", "add tests for X", "write a test", "cover this", "TDD this", or similar. +--- + +# test + +Write tests a senior Go engineer would ship. Strict table-driven layout, stdlib comparators, race-clean, time-injected. The CLI's hard rule: every renderer output is pinned by a golden file. + +## When to invoke + +- "test this" / "add tests for X" / "cover X with tests" +- "write a test" / "TDD this" +- "the test for X is failing — fix it" (read, find root cause, write the minimal repro test alongside the fix) +- "update the golden snapshot" (only if a deliberate output change — see §Golden updates) + +## Hard rules + +### 1. Table-driven is the default + +Any test with more than one case uses an inline table, one row per scenario, each row run inside `t.Run(tc.name, ...)`. + +```go +for _, tc := range []struct { + name string + // … +}{ + {name: "happy path", /* … */}, + {name: "empty input", /* … */}, +} { + t.Run(tc.name, func(t *testing.T) { /* … */ }) +} +``` + +No package-level fixtures. No shared mutable state across rows. + +**No `_TableDriven` suffix in the test function name.** TDT is the default; the suffix is noise. `TestRender_Demo` consolidating multiple render scenarios into a table is named `TestRender_Demo`, not `TestRender_Demo_TableDriven`. The subtest names (`tc.name`) carry the scenario. + +### 2. Naming + +`TestFunctionName_Scenario_ExpectedBehavior`. Read aloud, it forms a sentence. + +| Good | Bad | +|---|---| +| `TestParseValues_EmptyFile_ReturnsErrEmpty` | `TestParse` | +| `TestRender_DemoPlain_GoldenStable` | `TestRender` | +| `TestRoast_AllDetectors_HaveRewrite` | `TestRoastWorks` | +| `TestShare_PIIStrip_RemovesSourcePaths` | `TestShareUpload` | + +### 3. Stdlib only — no testify, no gomock, no third-party assertion libs + +CLAUDE.md hard rule: "Prefer stdlib." Comparators: + +- `==` for primitives, strings, numbers. +- `reflect.DeepEqual` for structs/maps/slices. +- `errors.Is` / `errors.As` for error checks. +- `bytes.Equal` for byte slices. +- Golden files for anything multi-line or whitespace-sensitive. + +### 4. Race detector always + +`go test -race ./...` is the project's CI bar. Any test that wouldn't pass `-race` is broken; fix it before shipping. + +### 5. Time + randomness injected + +Never call `time.Now()`, `rand.Intn`, or `crypto/rand.Read` directly from code under test. Accept a `Clock`, `*rand.Rand`, or `io.Reader`. In tests, pin them: + +```go +now := time.Date(2026, 5, 24, 0, 0, 0, 0, time.UTC) +clock := func() time.Time { return now } +``` + +The CLI's `share` package and any future scheduler use this pattern. A flaky clock = a flaky CI. + +### 6. Helpers call `t.Helper()` + +Any function taking `*testing.T` that isn't itself a `TestXxx` calls `t.Helper()` on its first line. Otherwise `t.Fatalf` reports the helper line, not the caller's. + +### 7. Subtests are `t.Run` + +Use `t.Run(tc.name, …)` so failures name the row (`TestParse/empty_file: …`), not the loop line. + +### 8. Golden tests for every renderer output + +The CLI's whole point is deterministic output that a customer can paste into a PR. Every renderer (text, JSON, HTML, roast, score) has a golden snapshot in `testdata/golden/`. The mandatory ±40% accuracy disclosure is asserted in every renderer test — that's the hard rule per CLAUDE.md, and the disclosure string must stay byte-identical across renderers. + +Regenerate goldens with the project's update flag (see §Golden updates). Eyeball every diff. + +### 9. Pure functions only (no LLM calls in tests) + +CLAUDE.md hard rule: "No LLM calls." No telemetry, no HTTP egress (except the opt-in `--share` upload client, which has its own fake `Upload` interface). Tests must be hermetic — runnable on a plane, in an airgap, without internet. + +### 10. One concern per test + +A test asserts one thing. Three behaviours = three named subtests or three `TestXxx` functions. A failure must name the broken contract. + +--- + +## Decision: unit vs golden vs CLI invocation + +| Type | Where | Coverage | Example | +|---|---|---|---| +| **Unit** | `_test.go` next to the code, same package | Pure logic: parsers, rule engines, score math, share-URL hashing | `pkg/rules/rules_test.go` | +| **Golden** | `testdata/golden/*.txt` asserted via `_test.go` | Renderer output byte-stability | `internal/render/golden_test.go` | +| **CLI invocation** | `cmd/optiqor/golden_test.go` | Whole-binary invocation through Cobra; asserts stdout/exit-code against golden | `cmd/optiqor/golden_test.go` already exists with this pattern | + +There is no integration-test boundary in the CLI — the CLI does not talk to a database. The closest thing is the `--share` upload, which is tested against an `httptest.Server` fake. + +--- + +## Templates (copy-paste, edit) + +### Template A — table-driven detector test + +```go +func TestCPUOverprovisioned(t *testing.T) { + for _, tc := range []struct { + name string + in parser.Workload + want []rules.Finding + }{ + { + name: "request half the limit triggers", + in: parser.Workload{ + Name: "api", + CPURequest: q("1"), CPULimit: q("4"), + }, + want: []rules.Finding{{ + DetectorID: "cpu-overprovisioned", + Workload: "api", + Severity: rules.SeverityMed, + }}, + }, + { + name: "request equals limit does not trigger", + in: parser.Workload{ + Name: "api", + CPURequest: q("2"), CPULimit: q("2"), + }, + want: nil, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := CPUOverprovisioned().Run([]parser.Workload{tc.in}) + if !findingsMatch(got, tc.want) { + t.Errorf("got %+v, want %+v", got, tc.want) + } + }) + } +} +``` + +Notes: +- Inline struct literal. `name` first. No shared `var cases = …`. +- Helper `q("2")` parses a Quantity — keep helpers small, `t.Helper()` inside. +- Comparator `findingsMatch` does field-equality on the ID + Severity + Workload — ignore cost cents and exact title text so renaming a title doesn't break the test (test the contract, not the prose). + +### Template B — golden renderer test + +```go +func TestRender_Demo_PlainGolden(t *testing.T) { + report := loadFixtureReport(t, "demo_findings.json") + var buf bytes.Buffer + if err := Render(&buf, report, Options{Color: false, Width: 78}); err != nil { + t.Fatal(err) + } + goldenAssert(t, "demo_plain", buf.Bytes()) +} + +func TestRender_Demo_JSONGolden(t *testing.T) { + report := loadFixtureReport(t, "demo_findings.json") + var buf bytes.Buffer + if err := RenderJSON(&buf, report); err != nil { + t.Fatal(err) + } + goldenAssert(t, "demo_json", buf.Bytes()) +} +``` + +Both invoke the same fixture so a renderer-pair drift is caught immediately. Disclosure presence is asserted separately: + +```go +func TestRender_DisclosureAlwaysPresent(t *testing.T) { + for _, mode := range []string{"plain", "color", "json", "roast"} { + t.Run(mode, func(t *testing.T) { + out := renderForTest(t, mode) + if !bytes.Contains(out, []byte(AccuracyDisclosure)) { + t.Errorf("%s mode is missing the mandatory ±40%% disclosure", mode) + } + }) + } +} +``` + +### Template C — `httptest.Server` fake for `--share` + +```go +func TestShare_UploadsSanitizedPayload(t *testing.T) { + var got []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + t.Setenv("OPTIQOR_SHARE_URL", srv.URL+"/api/v1/share") + + if err := Upload(context.Background(), Report{ /* … */ }); err != nil { + t.Fatal(err) + } + if bytes.Contains(got, []byte("/home/user/secrets")) { + t.Error("source path leaked — sanitiser failed") + } +} +``` + +Notes: +- `httptest.NewServer` is the only network-shaped test allowed. +- `t.Setenv` over `os.Setenv` so the env var auto-restores. +- `t.Cleanup` over `defer` so it survives subtest cancellation. + +### Template D — CLI invocation through Cobra + +The CLI repo already has a pattern at `cmd/optiqor/golden_test.go`. The shape: + +```go +func TestCmd_Analyze_DemoChart_GoldenStable(t *testing.T) { + var stdout, stderr bytes.Buffer + rc := runCmd(t, []string{"analyze", "--no-color", "testdata/fixtures/basic-chart"}, + &stdout, &stderr) + if rc != 0 { + t.Fatalf("exit code: got %d, want 0; stderr=%s", rc, stderr.String()) + } + goldenAssert(t, "analyze_basic_chart_plain", stdout.Bytes()) +} +``` + +`runCmd` invokes the same `RootCmd()` the binary does, with redirected streams. Exit code is the Cobra `Execute()` return mapped to the project's exit-code matrix (0 clean / 1 findings / 2 invocation / 3 runtime). + +--- + +## Process for a new test + +``` +1. Read the code under test + - What's the contract? (godoc + signatures) + - What's the input shape, what's the output shape? + - For a detector: what counts as a positive, what counts as negative? + - For a renderer: what's the byte-identity contract? + +2. Enumerate scenarios + - One row per behaviour the code must guarantee + - Detector: positive case, negative case, boundary (exactly at threshold) + - Renderer: every option combination that materially changes output + +3. Pick the test type + - Pure logic → unit (Template A) + - Renderer / CLI output → golden (Template B or D) + - Share upload → httptest.Server fake (Template C) + +4. Write the table + - Anonymous struct inline + - name string field first + +5. Wire t.Run subtests + - Each row is a t.Run(tc.name, func(t *testing.T) { ... }) + - Use t.Helper() in any helpers + +6. Inject time / randomness + - Pin time.Date(...) for time-dependent assertions + - Accept io.Reader / *rand.Rand instead of calling globals + +7. Run the gate (see below) + +8. Commit per the project's commit skill + - `test(): cover ` is the conventional subject +``` + +## Quality gate (run before declaring done) + +From the repo root: + +```bash +gofmt -l . # must be empty +go vet ./... # must be clean +go test -race -count=1 ./... # must pass +golangci-lint run --timeout=2m ./... # must report 0 issues +``` + +If golden snapshots diverge, the gate fails by design. Eyeball the diff: + +- If the change is **intentional** (you changed the renderer / score formula / disclosure text), run `UPDATE_GOLDEN=1 go test ./...` to regenerate, eyeball the diff again, commit the regenerated files alongside the code change. +- If the change is **unintentional**, the test is doing its job — find what drifted and fix it. + +## Coverage targets + +- `pkg/rules/`, `pkg/parser/`, `pkg/htmlrender/`: **90%** (public surface; every detector has positive + negative cases minimum) +- `internal/analyze/`, `internal/render/`, `internal/share/`, `internal/config/`: **70%** +- `internal/roast/`, `internal/render/style/`: **70%** including golden coverage of every styled-mode permutation +- `cmd/optiqor/`: covered by the golden tests in `cmd/optiqor/golden_test.go` — direct unit tests on Cobra command construction are not worth writing + +Check with: + +```bash +go test -cover ./... +``` + +## Golden updates + +If you intentionally changed a renderer or detector and a golden diff is the expected output: + +```bash +UPDATE_GOLDEN=1 go test ./... +git diff testdata/golden/ # eyeball every line +git add testdata/golden/.txt # only the ones that actually changed +``` + +Never commit a golden update without reading the diff. A test that's failing only because the golden is stale is the same as no test — it tells you nothing. + +The accuracy disclosure inside any golden file must remain byte-identical to `AccuracyDisclosure`. If the disclosure text moved across a column or the surrounding box characters changed, ensure the disclosure substring is intact. + +## Anti-patterns + +| ❌ Do not | ✅ Instead | +|---|---| +| Compare `err.Error() == "expected text"` | `errors.Is(err, ErrSentinel)` | +| Share a `var fixture = ...` across tests | Build fresh inside each test | +| `time.Sleep` to wait for async work | The CLI is synchronous — there is no async to wait for. If you're tempted, the design is wrong. | +| `os.Setenv` without `t.Setenv` | `t.Setenv` — auto-restores | +| Pull in `testify/assert` for a one-line test | `if got != want { t.Errorf(...) }` — stdlib hard rule | +| Hit a real network endpoint | `httptest.NewServer` — never the real `optiqor.dev/r/` | +| Run the LLM, even a "small" one | Per CLAUDE.md: no LLM in CLI. Tests are deterministic. | +| Commit golden updates without reading the diff | `git diff testdata/golden/` first, every time | +| Test colour-rendering by stripping ANSI codes in the assertion | Run the test twice: one golden for `Color: false`, one for `Color: true`. Different concerns, different snapshots. | +| `init()` in test files to register fixtures | Push into `setUpXxx(t)` helpers; no `init()` in tests | +| Asserting on stderr log output | Logs are for humans, not contracts. Surface contract via return value or exit code. | + +## Examples in this repo (read these for tone) + +- **Table-driven detector**: [`pkg/rules/rules_test.go`](../../../pkg/rules/rules_test.go) and friends — positive + negative + threshold-boundary in rows. +- **Renderer golden**: [`internal/render/text_test.go`](../../../internal/render/text_test.go) — golden assertions for plain + JSON; disclosure-presence test that loops over modes. +- **CLI golden**: [`cmd/optiqor/golden_test.go`](../../../cmd/optiqor/golden_test.go) — full Cobra invocation, exit-code + stdout assertion. +- **`httptest.Server` for share upload**: [`internal/share/share_test.go`](../../../internal/share/share_test.go) — fake server captures payload, asserts sanitiser stripped PII. +- **Helper with `t.Helper()`**: [`pkg/parser/helm_test.go`](../../../pkg/parser/helm_test.go) — `q("2Gi")` quantity helper, `loadFixture(t, "basic-chart")` chart helper. +- **Boundary assertion**: [`internal/analyze/grade_test.go`](../../../internal/analyze/grade_test.go) — explicit `// 100 - 20 = 80` math-restate comments are kept because the test rationale is the threshold itself. + +When in doubt, read one of those and match its shape. diff --git a/CLAUDE.md b/CLAUDE.md index 5a3842f..e80784e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -150,15 +150,51 @@ Golden tests in `testdata/golden/` assert byte-identical output across runs. A f ## Testing -- Unit tests beside the code (`foo.go` + `foo_test.go`). -- Golden tests in `testdata/golden/` for every renderer mode. -- One test command: `go test ./...`. No separate integration suite. The CLI is pure functions over Helm input. -- Race detector in CI. -- Every detector in `pkg/rules` needs at least one positive and one negative test. -- Every renderer mode has a golden fixture. +- Table-driven by default. One `Test` per SUT method, one row per scenario, `t.Run(tc.name, ...)` so failures name the row. Do **not** suffix names with `_TableDriven` — every test should already be table-driven, so the marker is noise. +- Unit tests beside the code: `foo.go` + `foo_test.go`. +- Stdlib only — no testify, no gomock. `errors.Is` / `errors.As` for error checks, `bytes.Equal` for bytes, golden files for multi-line / whitespace-sensitive output. +- Race detector always: `go test -race -count=1 ./...` is the CI bar. +- Inject time + randomness — `time.Date(...)`, `bytes.NewReader`, never `time.Now()` / `rand.Intn` in code under test. +- Helpers call `t.Helper()` on the first line. +- Golden tests in `testdata/golden/` for every renderer mode. The mandatory ±40% accuracy disclosure is asserted byte-identical across modes. Regenerate with `UPDATE_GOLDEN=1 go test ./...` and eyeball the diff before committing. +- Every detector in `pkg/rules` needs at least one positive and one negative table row. +- `go test ./...` is the only test command. No separate integration suite — the CLI is pure functions over Helm input. `pkg/` (the public surface) has stricter test discipline. Coverage target: 90%. External tests (`pkg/foo_test.go` in package `foo_test`) for any exported symbol with a non-trivial contract. +See `.claude/skills/test/SKILL.md` for the full procedure: templates, anti-patterns, coverage targets, and references to clean examples in the repo. + +## Comments + +Comments explain WHY, never what. Decoration is debt; every comment is a thing a future engineer must keep in sync with the code. When in doubt, delete. Re-add only when a reader would miss a non-obvious constraint, trade-off, ADR reference, security/performance rationale, or invariant. + +The CLI is **Apache-2.0 OSS**, so `pkg/` is the public surface that external Go programs import. Lean slightly more conservative there: one-line godoc on every exported symbol is appropriate when it documents a contract beyond the name (`// Run executes all detectors against the workloads and returns findings in detector-declaration order` is a real contract; `// Run runs the detectors.` is not). + +**Banned:** + +- Markdown headers (`#`, `##`, `###`) inside `//` comments. +- `Note:`, `Important:`, `Caution:`, `Warning:` labels. If it's important, the code structure should make the rule unmissable. +- Em-dash-heavy narrative essays in package or function docstrings. One terse sentence beats five lines of glue. +- Decorative section dividers: `// ─── Helpers ───`, `// ====== validators ======`. Use blank lines. +- Multi-paragraph package docstrings narrating "Layout philosophy:", "Implementation notes:", "Design constraints:". Compress to one or two terse sentences. +- Bullet-listed enumerations of struct fields or function returns — the type tags already enumerate them. +- Restating the signature (`// Foo returns a Foo.`) or the next line of code. +- "Helper function to...", "Utility for...", "This function..." preambles. +- Emojis. None, anywhere. +- TODOs without a name and issue: `// TODO(@shivam, #42): ...` is fine; `// TODO: do later` is not. + +**Always keep:** + +- The CLI's hard-rule pins: no LLM in CLI, no telemetry by default, ±40% accuracy disclosure is mandatory in every renderer output. Comments that pin these stay verbatim. +- Detector references to CIS / NSA hardening guides — those anchors are load-bearing for security findings. +- Cross-file references to other packages, ADRs, or upstream specs. + +**How to audit a comment**: ask *"if I delete this, what does the next reader fail to understand?"* If the answer is "nothing — the name + signature + types already say it", delete. If the answer names a specific constraint, trade-off, or hard-rule pin, keep but compress. + +**Reference commits**: the 2026-05-24 cleanup (`feba8e0` openapi header, `9f817cd` cli sweep) stripped ~400 lines of comment cruft from this repo. Read those diffs for the tone applied at scale; new code should land at that compression level from the start. + +## Don't + --- ## Code style @@ -289,7 +325,7 @@ CLI binaries run on customer laptops. The supply chain is a primary attack surfa Conventional Commits. One concern per commit. DCO sign-off required (`-s` flag, enforced by CI). -See [.claude/skills/commit/SKILL.md](.claude/skills/commit/SKILL.md) for the rules and the local quality gate (gofmt + vet + build + test -race + lint). +Local quality gate before pushing: `gofmt -l .` (must be empty), `go vet ./...`, `go build ./...`, `go test -race -count=1 ./...`, `golangci-lint run --timeout=2m ./...`. ### Pull requests @@ -303,11 +339,11 @@ See [.claude/skills/commit/SKILL.md](.claude/skills/commit/SKILL.md) for the rul ### Reviews -See [.claude/skills/pr-review/SKILL.md](.claude/skills/pr-review/SKILL.md) for voice, line-anchoring, and verdict rules. +One inline comment per issue, anchored to the exact line. Short summary at the end. Terse maintainer voice — no em-dashes, no markdown headers in the comment body, no bold-labels. ### Issues -See [.claude/skills/open-issue/SKILL.md](.claude/skills/open-issue/SKILL.md) for the audit checklist, classification table, and issue-body shapes. +One issue per finding. Repo-correct (`optiqor-cli/` for OSS-surface bugs, `optiqor/` for backend). Title is a single declarative sentence. Body is plain prose, no `## Problem` / `## Expected Behavior` headers. ### Decisions diff --git a/cmd/optiqor/golden_test.go b/cmd/optiqor/golden_test.go index 5fa3305..3f93bc8 100644 --- a/cmd/optiqor/golden_test.go +++ b/cmd/optiqor/golden_test.go @@ -9,38 +9,33 @@ import ( "testing" ) -// -update regenerates the golden files. Use only after a deliberate -// UX change. +// -update regenerates the golden files after a deliberate UX change. var update = flag.Bool("update", false, "update golden files") const goldenDir = "../../testdata/golden" -type goldenCase struct { - name string - args []string -} - -var goldenCases = []goldenCase{ - {name: "demo_plain", args: []string{"--no-color", "demo"}}, - {name: "demo_json", args: []string{"demo", "--json"}}, - {name: "analyze_fixture_plain", args: []string{"--no-color", "analyze", "../../testdata/fixtures/basic-chart/values.yaml"}}, - {name: "analyze_fixture_severity_high", args: []string{"--no-color", "analyze", "../../testdata/fixtures/basic-chart/values.yaml", "--severity", "high"}}, - {name: "analyze_fixture_detector_filter", args: []string{"--no-color", "analyze", "../../testdata/fixtures/basic-chart/values.yaml", "--detector", "image-pinned-latest"}}, - {name: "score_fixture_plain", args: []string{"--no-color", "score", "../../testdata/fixtures/basic-chart/values.yaml"}}, - {name: "score_fixture_json", args: []string{"score", "../../testdata/fixtures/basic-chart/values.yaml", "--json"}}, - {name: "audit_fixture_plain", args: []string{"--no-color", "audit", "../../testdata/fixtures/basic-chart/values.yaml", "--fail-on", ""}}, -} - -func TestGolden(t *testing.T) { +func TestCmd_Golden_Stable(t *testing.T) { if err := os.MkdirAll(goldenDir, 0o755); err != nil { t.Fatal(err) } // Pin width or the dev's $COLUMNS leaks into goldens and diverges - // from CI (no TTY → fallback 80). + // from CI (no TTY -> fallback 80). t.Setenv("COLUMNS", "80") - for _, tc := range goldenCases { + for _, tc := range []struct { + name string + args []string + }{ + {name: "demo_plain", args: []string{"--no-color", "demo"}}, + {name: "demo_json", args: []string{"demo", "--json"}}, + {name: "analyze_fixture_plain", args: []string{"--no-color", "analyze", "../../testdata/fixtures/basic-chart/values.yaml"}}, + {name: "analyze_fixture_severity_high", args: []string{"--no-color", "analyze", "../../testdata/fixtures/basic-chart/values.yaml", "--severity", "high"}}, + {name: "analyze_fixture_detector_filter", args: []string{"--no-color", "analyze", "../../testdata/fixtures/basic-chart/values.yaml", "--detector", "image-pinned-latest"}}, + {name: "score_fixture_plain", args: []string{"--no-color", "score", "../../testdata/fixtures/basic-chart/values.yaml"}}, + {name: "score_fixture_json", args: []string{"score", "../../testdata/fixtures/basic-chart/values.yaml", "--json"}}, + {name: "audit_fixture_plain", args: []string{"--no-color", "audit", "../../testdata/fixtures/basic-chart/values.yaml", "--fail-on", ""}}, + } { t.Run(tc.name, func(t *testing.T) { cmd := newRootCmd() var buf bytes.Buffer diff --git a/cmd/optiqor/main_test.go b/cmd/optiqor/main_test.go index 635ed50..3ec0a14 100644 --- a/cmd/optiqor/main_test.go +++ b/cmd/optiqor/main_test.go @@ -6,7 +6,7 @@ import ( "testing" ) -func TestRoot_Help(t *testing.T) { +func TestRoot_Help_ContainsCommandsAndDisclosure(t *testing.T) { cmd := newRootCmd() var buf bytes.Buffer cmd.SetOut(&buf) @@ -28,7 +28,7 @@ func TestRoot_Help(t *testing.T) { } } -func TestVersion_Output(t *testing.T) { +func TestRoot_Version_NamesBinary(t *testing.T) { cmd := newRootCmd() var buf bytes.Buffer cmd.SetOut(&buf) @@ -43,10 +43,7 @@ func TestVersion_Output(t *testing.T) { } } -// TestDemo_RunsAndIncludesDisclosure exercises the full demo path -// (embedded fixture → parser → rules → render) and checks the -// accuracy disclosure shows up. -func TestDemo_RunsAndIncludesDisclosure(t *testing.T) { +func TestDemo_FullPath_EmitsDisclosureAndWorkloads(t *testing.T) { cmd := newRootCmd() var buf bytes.Buffer cmd.SetOut(&buf) @@ -61,6 +58,7 @@ func TestDemo_RunsAndIncludesDisclosure(t *testing.T) { "Helm chart cost", "api", "worker", + // ±40% accuracy disclosure is mandatory in every renderer output (CLAUDE.md hard rule). "±40%", "optiqor.dev/get", } { @@ -73,9 +71,7 @@ func TestDemo_RunsAndIncludesDisclosure(t *testing.T) { } } -// TestAnalyze_FixtureFile asserts well-known severities and -// detectors fire; lets count grow naturally as the library expands. -func TestAnalyze_FixtureFile(t *testing.T) { +func TestAnalyze_FixtureFile_FiresWellKnownDetectors(t *testing.T) { cmd := newRootCmd() var buf bytes.Buffer cmd.SetOut(&buf) @@ -85,8 +81,6 @@ func TestAnalyze_FixtureFile(t *testing.T) { t.Fatalf("execute analyze: %v\n%s", err, buf.String()) } out := buf.String() - // Renderer emits severity badges and bare workload identifiers - // (no "workload: " prefix). for _, want := range []string{ "15 workloads", "HIGH", @@ -106,7 +100,7 @@ func TestAnalyze_FixtureFile(t *testing.T) { } } -func TestAnalyze_JSONShape(t *testing.T) { +func TestAnalyze_JSON_ShapeContainsDisclosureAndDetectors(t *testing.T) { cmd := newRootCmd() var buf bytes.Buffer cmd.SetOut(&buf) @@ -131,67 +125,93 @@ func TestAnalyze_JSONShape(t *testing.T) { } } -func TestResolveColor_NoColorFlag(t *testing.T) { - cmd := newRootCmd() - if got := resolveColor(cmd, true); got { - t.Error("resolveColor with --no-color should be false") - } -} - -func TestResolveColor_NoColorEnv(t *testing.T) { - t.Setenv("NO_COLOR", "1") - cmd := newRootCmd() - if got := resolveColor(cmd, false); got { - t.Error("resolveColor with NO_COLOR=1 should be false") - } -} - -func TestResolveColor_CLICOLORForceWithoutTTY(t *testing.T) { - t.Setenv("CLICOLOR_FORCE", "1") - t.Setenv("NO_COLOR", "") - cmd := newRootCmd() - cmd.SetOut(&bytes.Buffer{}) // not a *os.File so TTY path is false - if got := resolveColor(cmd, false); !got { - t.Error("CLICOLOR_FORCE=1 should override TTY check") - } -} - -func TestResolveColor_NoTTYNoColor(t *testing.T) { - t.Setenv("CLICOLOR_FORCE", "") - t.Setenv("NO_COLOR", "") - cmd := newRootCmd() - cmd.SetOut(&bytes.Buffer{}) // not a TTY - if got := resolveColor(cmd, false); got { - t.Error("buffer (non-TTY) without forces should disable color") +func TestResolveColor(t *testing.T) { + for _, tc := range []struct { + name string + noColorFlag bool + noColorEnv string + clicolorForce string + setNoColorEnv bool + setForceEnv bool + nonTTYOut bool + want bool + }{ + { + name: "no-color-flag-wins", + noColorFlag: true, + want: false, + }, + { + name: "no-color-env-disables", + setNoColorEnv: true, + noColorEnv: "1", + want: false, + }, + { + name: "clicolor-force-overrides-non-tty", + setForceEnv: true, + clicolorForce: "1", + setNoColorEnv: true, + noColorEnv: "", + nonTTYOut: true, + want: true, + }, + { + name: "non-tty-without-forces-disables", + setForceEnv: true, + clicolorForce: "", + setNoColorEnv: true, + noColorEnv: "", + nonTTYOut: true, + want: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.setNoColorEnv { + t.Setenv("NO_COLOR", tc.noColorEnv) + } + if tc.setForceEnv { + t.Setenv("CLICOLOR_FORCE", tc.clicolorForce) + } + cmd := newRootCmd() + if tc.nonTTYOut { + cmd.SetOut(&bytes.Buffer{}) + } + if got := resolveColor(cmd, tc.noColorFlag); got != tc.want { + t.Errorf("resolveColor = %v, want %v", got, tc.want) + } + }) } } func TestAtoi(t *testing.T) { - cases := map[string]struct { - want int - err bool + for _, tc := range []struct { + name string + in string + want int + wantErr bool }{ - "": {0, false}, - "0": {0, false}, - "123": {123, false}, - "abc": {0, true}, - "12x": {0, true}, - "99": {99, false}, - } - for in, tc := range cases { - got, err := atoi(in) - if tc.err { - if err == nil { - t.Errorf("atoi(%q): expected error", in) + {name: "empty-string-is-zero", in: "", want: 0}, + {name: "zero", in: "0", want: 0}, + {name: "three-digit", in: "123", want: 123}, + {name: "two-digit", in: "99", want: 99}, + {name: "all-letters-errors", in: "abc", wantErr: true}, + {name: "trailing-letter-errors", in: "12x", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := atoi(tc.in) + if tc.wantErr { + if err == nil { + t.Fatalf("atoi(%q): expected error", tc.in) + } + return } - continue - } - if err != nil { - t.Errorf("atoi(%q): unexpected error: %v", in, err) - continue - } - if got != tc.want { - t.Errorf("atoi(%q) = %d, want %d", in, got, tc.want) - } + if err != nil { + t.Fatalf("atoi(%q): unexpected error: %v", tc.in, err) + } + if got != tc.want { + t.Errorf("atoi(%q) = %d, want %d", tc.in, got, tc.want) + } + }) } } diff --git a/internal/analyze/analyze_test.go b/internal/analyze/analyze_test.go index e38b10e..aa721fd 100644 --- a/internal/analyze/analyze_test.go +++ b/internal/analyze/analyze_test.go @@ -7,8 +7,17 @@ import ( "testing" ) -func TestRun_ReturnsFindings(t *testing.T) { - in := strings.NewReader(` +func TestRun(t *testing.T) { + for _, tc := range []struct { + name string + yaml string + wantErr bool + wantWorkloads int + mustSeeIDs []string + }{ + { + name: "two-workloads-fire-cpu-and-missing-limit", + yaml: ` api: resources: requests: @@ -21,45 +30,50 @@ worker: resources: requests: memory: "1Gi" -`) - rep, err := Run(in, Options{Source: "test"}) - if err != nil { - t.Fatalf("Run: %v", err) - } - if rep.Workloads != 2 { - t.Errorf("workloads = %d, want 2", rep.Workloads) - } - if len(rep.Findings) == 0 { - t.Fatal("expected findings, got 0") - } - - var sawCPU, sawMissingLimit bool - for _, f := range rep.Findings { - switch f.DetectorID { - case "cpu-overprovisioned": - sawCPU = true - case "missing-memory-limit": - sawMissingLimit = true - } - } - if !sawCPU { - t.Error("missing cpu-overprovisioned finding") - } - if !sawMissingLimit { - t.Error("missing missing-memory-limit finding") - } -} - -func TestRun_BadYAML(t *testing.T) { - if _, err := Run(strings.NewReader("not: valid: yaml::"), Options{}); err == nil { - t.Fatal("expected error") +`, + wantWorkloads: 2, + mustSeeIDs: []string{"cpu-overprovisioned", "missing-memory-limit"}, + }, + { + name: "malformed-yaml-errors", + yaml: "not: valid: yaml::", + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + rep, err := Run(strings.NewReader(tc.yaml), Options{Source: "test"}) + if tc.wantErr { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatalf("Run: %v", err) + } + if rep.Workloads != tc.wantWorkloads { + t.Errorf("workloads = %d, want %d", rep.Workloads, tc.wantWorkloads) + } + if len(rep.Findings) == 0 { + t.Fatal("expected findings, got 0") + } + seen := map[string]bool{} + for _, f := range rep.Findings { + seen[f.DetectorID] = true + } + for _, id := range tc.mustSeeIDs { + if !seen[id] { + t.Errorf("missing detector %q in findings", id) + } + } + }) } } -func TestRunPath_File(t *testing.T) { +func TestRunPath(t *testing.T) { dir := t.TempDir() - values := filepath.Join(dir, "values.yaml") - if err := os.WriteFile(values, []byte(` + fileValues := filepath.Join(dir, "file-values.yaml") + if err := os.WriteFile(fileValues, []byte(` api: resources: requests: @@ -72,34 +86,53 @@ api: t.Fatal(err) } - rep, err := RunPath(values) - if err != nil { - t.Fatalf("RunPath: %v", err) - } - if rep.Workloads != 1 { - t.Errorf("workloads = %d, want 1", rep.Workloads) - } -} - -func TestRunPath_Directory(t *testing.T) { - dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "values.yaml"), []byte(`api: {resources: {requests: {cpu: 1, memory: 1Gi}, limits: {cpu: 1, memory: 1Gi}}}`), 0o600); err != nil { + dirChart := t.TempDir() + if err := os.WriteFile(filepath.Join(dirChart, "values.yaml"), + []byte(`api: {resources: {requests: {cpu: 1, memory: 1Gi}, limits: {cpu: 1, memory: 1Gi}}}`), 0o600); err != nil { t.Fatal(err) } - rep, err := RunPath(dir) - if err != nil { - t.Fatalf("RunPath dir: %v", err) - } - if rep.Workloads != 1 { - t.Errorf("workloads = %d", rep.Workloads) - } - if !strings.HasSuffix(rep.Source, "values.yaml") { - t.Errorf("source should point to values.yaml, got %q", rep.Source) - } -} -func TestRunPath_Missing(t *testing.T) { - if _, err := RunPath("/nonexistent/path/values.yaml"); err == nil { - t.Fatal("expected error") + for _, tc := range []struct { + name string + path string + wantErr bool + wantWorkloads int + wantSrcSuffix string + }{ + { + name: "file-path-loads-directly", + path: fileValues, + wantWorkloads: 1, + }, + { + name: "directory-resolves-to-values-yaml", + path: dirChart, + wantWorkloads: 1, + wantSrcSuffix: "values.yaml", + }, + { + name: "missing-path-errors", + path: "/nonexistent/path/values.yaml", + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + rep, err := RunPath(tc.path) + if tc.wantErr { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatalf("RunPath: %v", err) + } + if rep.Workloads != tc.wantWorkloads { + t.Errorf("workloads = %d, want %d", rep.Workloads, tc.wantWorkloads) + } + if tc.wantSrcSuffix != "" && !strings.HasSuffix(rep.Source, tc.wantSrcSuffix) { + t.Errorf("source = %q, want suffix %q", rep.Source, tc.wantSrcSuffix) + } + }) } } diff --git a/internal/analyze/diff.go b/internal/analyze/diff.go index a265ee1..8b71544 100644 --- a/internal/analyze/diff.go +++ b/internal/analyze/diff.go @@ -24,6 +24,7 @@ type DiffEntry struct { MonthlyUSDCentsDelta int64 // sandbox estimate; ±40% disclosure applies } +// DiffReport is the wire shape Diff returns. type DiffReport struct { A string `json:"a"` B string `json:"b"` @@ -90,6 +91,7 @@ func Diff(a, b io.Reader, aLabel, bLabel string) (DiffReport, error) { return DiffReport{A: aLabel, B: bLabel, Entries: entries}, nil } +// DiffPaths opens both files and runs Diff. func DiffPaths(a, b string) (DiffReport, error) { fa, err := openValues(a) if err != nil { diff --git a/internal/analyze/diff_render.go b/internal/analyze/diff_render.go index bb60b02..254c85d 100644 --- a/internal/analyze/diff_render.go +++ b/internal/analyze/diff_render.go @@ -59,6 +59,7 @@ func (r DiffReport) WriteText(w io.Writer, opts render.Options) error { return err } +// WriteJSON emits the report with the mandatory accuracy disclosure. func (r DiffReport) WriteJSON(w io.Writer) error { enc := json.NewEncoder(w) enc.SetIndent("", " ") diff --git a/internal/analyze/diff_test.go b/internal/analyze/diff_test.go index 4f119fe..642234d 100644 --- a/internal/analyze/diff_test.go +++ b/internal/analyze/diff_test.go @@ -1,6 +1,7 @@ package analyze import ( + "io" "strings" "testing" ) @@ -18,7 +19,6 @@ worker: const bOnly = ` api: - # halved CPU + memory request resources: requests: {cpu: "500m", memory: "512Mi"} limits: {cpu: "500m", memory: "512Mi"} @@ -27,89 +27,119 @@ worker: requests: {cpu: "500m", memory: "1Gi"} limits: {cpu: "500m", memory: "1Gi"} cache: - # net-new workload resources: requests: {cpu: "100m", memory: "256Mi"} limits: {cpu: "200m", memory: "512Mi"} ` -func TestDiff_DetectsAddedRemovedAndChanged(t *testing.T) { - rep, err := Diff(strings.NewReader(aOnly), strings.NewReader(bOnly), "before", "after") - if err != nil { - t.Fatalf("Diff: %v", err) - } - if rep.A != "before" || rep.B != "after" { - t.Errorf("labels lost: %+v", rep) - } - - byName := map[string]DiffEntry{} - for _, e := range rep.Entries { - byName[e.Name] = e - } - - api := byName["api"] - if api.NewWorkload || api.RemovedWorkload { - t.Errorf("api should be modified: %+v", api) - } - if api.CPURequestDelta != -500 { - t.Errorf("api cpu request delta = %d, want -500", api.CPURequestDelta) - } - if api.MemoryRequestDelta != -(512 * 1024 * 1024) { - t.Errorf("api memory request delta = %d, want -512Mi", api.MemoryRequestDelta) - } - if api.MonthlyUSDCentsDelta >= 0 { - t.Errorf("api should be a saving (negative cents); got %d", api.MonthlyUSDCentsDelta) - } - - worker := byName["worker"] - if worker.CPURequestDelta != 0 || worker.MonthlyUSDCentsDelta != 0 { - t.Errorf("worker unchanged should have 0 deltas: %+v", worker) - } - - cache := byName["cache"] - if !cache.NewWorkload { - t.Error("cache should be NewWorkload") - } - if cache.MonthlyUSDCentsDelta <= 0 { - t.Errorf("new workload should add cost: %d", cache.MonthlyUSDCentsDelta) - } -} - -func TestDiff_RemovedWorkload(t *testing.T) { - rep, err := Diff(strings.NewReader(aOnly), strings.NewReader(`api: {resources: {requests: {cpu: 1, memory: 1Gi}, limits: {cpu: 1, memory: 1Gi}}}`), "a", "b") - if err != nil { - t.Fatal(err) - } - var sawRemoved bool - for _, e := range rep.Entries { - if e.Name == "worker" && e.RemovedWorkload { - sawRemoved = true - } - } - if !sawRemoved { - t.Errorf("expected worker as RemovedWorkload, got entries=%+v", rep.Entries) - } -} - -func TestDiff_TotalSavingsNegative(t *testing.T) { - rep, err := Diff(strings.NewReader(aOnly), strings.NewReader(`api: {resources: {requests: {cpu: 500m, memory: 512Mi}, limits: {cpu: 500m, memory: 512Mi}}} -worker: {resources: {requests: {cpu: 500m, memory: 1Gi}, limits: {cpu: 500m, memory: 1Gi}}}`), "a", "b") - if err != nil { - t.Fatal(err) - } - if rep.MonthlyUSDCentsDelta() >= 0 { - t.Errorf("net should be negative (savings); got %d", rep.MonthlyUSDCentsDelta()) - } -} - -func TestDiff_StableEntryOrder(t *testing.T) { - rep, err := Diff(strings.NewReader(aOnly), strings.NewReader(bOnly), "a", "b") - if err != nil { - t.Fatal(err) - } - for i := 1; i < len(rep.Entries); i++ { - if rep.Entries[i-1].Name >= rep.Entries[i].Name { - t.Errorf("entries not sorted: %v", rep.Entries) - } +func TestDiff(t *testing.T) { + for _, tc := range []struct { + name string + a, b func() io.Reader + aLabel string + bLabel string + check func(t *testing.T, rep DiffReport) + }{ + { + name: "detects-added-removed-and-changed", + a: func() io.Reader { return strings.NewReader(aOnly) }, + b: func() io.Reader { return strings.NewReader(bOnly) }, + aLabel: "before", + bLabel: "after", + check: func(t *testing.T, rep DiffReport) { + t.Helper() + if rep.A != "before" || rep.B != "after" { + t.Errorf("labels lost: %+v", rep) + } + byName := map[string]DiffEntry{} + for _, e := range rep.Entries { + byName[e.Name] = e + } + api := byName["api"] + if api.NewWorkload || api.RemovedWorkload { + t.Errorf("api should be modified: %+v", api) + } + if api.CPURequestDelta != -500 { + t.Errorf("api cpu request delta = %d, want -500", api.CPURequestDelta) + } + if api.MemoryRequestDelta != -(512 * 1024 * 1024) { + t.Errorf("api memory request delta = %d, want -512Mi", api.MemoryRequestDelta) + } + if api.MonthlyUSDCentsDelta >= 0 { + t.Errorf("api should be a saving (negative cents); got %d", api.MonthlyUSDCentsDelta) + } + worker := byName["worker"] + if worker.CPURequestDelta != 0 || worker.MonthlyUSDCentsDelta != 0 { + t.Errorf("worker unchanged should have 0 deltas: %+v", worker) + } + cache := byName["cache"] + if !cache.NewWorkload { + t.Error("cache should be NewWorkload") + } + if cache.MonthlyUSDCentsDelta <= 0 { + t.Errorf("new workload should add cost: %d", cache.MonthlyUSDCentsDelta) + } + }, + }, + { + name: "removed-workload-flags-removal", + a: func() io.Reader { return strings.NewReader(aOnly) }, + b: func() io.Reader { + return strings.NewReader(`api: {resources: {requests: {cpu: 1, memory: 1Gi}, limits: {cpu: 1, memory: 1Gi}}}`) + }, + aLabel: "a", + bLabel: "b", + check: func(t *testing.T, rep DiffReport) { + t.Helper() + var sawRemoved bool + for _, e := range rep.Entries { + if e.Name == "worker" && e.RemovedWorkload { + sawRemoved = true + } + } + if !sawRemoved { + t.Errorf("expected worker as RemovedWorkload, got entries=%+v", rep.Entries) + } + }, + }, + { + name: "total-savings-net-negative-when-shrinking", + a: func() io.Reader { return strings.NewReader(aOnly) }, + b: func() io.Reader { + return strings.NewReader(`api: {resources: {requests: {cpu: 500m, memory: 512Mi}, limits: {cpu: 500m, memory: 512Mi}}} +worker: {resources: {requests: {cpu: 500m, memory: 1Gi}, limits: {cpu: 500m, memory: 1Gi}}}`) + }, + aLabel: "a", + bLabel: "b", + check: func(t *testing.T, rep DiffReport) { + t.Helper() + if rep.MonthlyUSDCentsDelta() >= 0 { + t.Errorf("net should be negative (savings); got %d", rep.MonthlyUSDCentsDelta()) + } + }, + }, + { + name: "entries-sorted-by-name", + a: func() io.Reader { return strings.NewReader(aOnly) }, + b: func() io.Reader { return strings.NewReader(bOnly) }, + aLabel: "a", + bLabel: "b", + check: func(t *testing.T, rep DiffReport) { + t.Helper() + for i := 1; i < len(rep.Entries); i++ { + if rep.Entries[i-1].Name >= rep.Entries[i].Name { + t.Errorf("entries not sorted: %v", rep.Entries) + } + } + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + rep, err := Diff(tc.a(), tc.b(), tc.aLabel, tc.bLabel) + if err != nil { + t.Fatalf("Diff: %v", err) + } + tc.check(t, rep) + }) } } diff --git a/internal/analyze/filter_test.go b/internal/analyze/filter_test.go index ef41637..747caf9 100644 --- a/internal/analyze/filter_test.go +++ b/internal/analyze/filter_test.go @@ -7,7 +7,8 @@ import ( "github.com/optiqor/optiqor-cli/pkg/rules" ) -func sample() render.Report { +func sampleReport(t *testing.T) render.Report { + t.Helper() return render.Report{ Source: "x", Workloads: 3, @@ -21,54 +22,75 @@ func sample() render.Report { } } -func TestFilter_NoOptionsPassthrough(t *testing.T) { - r := sample() - out := Filter(r, FilterOptions{}) - if len(out.Findings) != 5 { - t.Fatalf("len = %d, want 5", len(out.Findings)) - } -} - -func TestFilter_SecurityOnly(t *testing.T) { - out := Filter(sample(), FilterOptions{SecurityOnly: true}) - for _, f := range out.Findings { - if f.Category != rules.CategorySecurity { - t.Errorf("non-security finding leaked: %+v", f) - } - } - if len(out.Findings) != 3 { - t.Errorf("len = %d, want 3 (missing-memory-limit + missing-cpu-limit + image-pinned-latest)", len(out.Findings)) - } -} - -func TestFilter_MinSeverity(t *testing.T) { - out := Filter(sample(), FilterOptions{MinSeverity: rules.SeverityMed}) - for _, f := range out.Findings { - if severityRank(f.Severity) < severityRank(rules.SeverityMed) { - t.Errorf("LOW finding leaked: %+v", f) - } - } - if len(out.Findings) != 4 { - t.Errorf("len = %d, want 4", len(out.Findings)) - } -} - -func TestFilter_DetectorAllowList(t *testing.T) { - out := Filter(sample(), FilterOptions{DetectorIDs: []string{"cpu-overprovisioned", "image-pinned-latest"}}) - if len(out.Findings) != 2 { - t.Fatalf("len = %d, want 2", len(out.Findings)) - } - got := map[string]bool{} - for _, f := range out.Findings { - got[f.DetectorID] = true - } - if !got["cpu-overprovisioned"] || !got["image-pinned-latest"] { - t.Errorf("allow-list leaked or missed: %v", got) +func TestFilter(t *testing.T) { + for _, tc := range []struct { + name string + opts FilterOptions + wantLen int + wantIDs []string + mustOnly rules.Category + denyBelow rules.Severity + }{ + { + name: "zero-options-passthrough", + opts: FilterOptions{}, + wantLen: 5, + }, + { + name: "security-only-strips-cost", + opts: FilterOptions{SecurityOnly: true}, + wantLen: 3, + mustOnly: rules.CategorySecurity, + }, + { + name: "min-severity-med-drops-low", + opts: FilterOptions{MinSeverity: rules.SeverityMed}, + wantLen: 4, + denyBelow: rules.SeverityMed, + }, + { + name: "detector-allow-list-keeps-named-only", + opts: FilterOptions{DetectorIDs: []string{"cpu-overprovisioned", "image-pinned-latest"}}, + wantLen: 2, + wantIDs: []string{"cpu-overprovisioned", "image-pinned-latest"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + out := Filter(sampleReport(t), tc.opts) + if len(out.Findings) != tc.wantLen { + t.Fatalf("len = %d, want %d", len(out.Findings), tc.wantLen) + } + if tc.mustOnly != "" { + for _, f := range out.Findings { + if f.Category != tc.mustOnly { + t.Errorf("non-%s finding leaked: %+v", tc.mustOnly, f) + } + } + } + if tc.denyBelow != "" { + for _, f := range out.Findings { + if severityRank(f.Severity) < severityRank(tc.denyBelow) { + t.Errorf("below-threshold finding leaked: %+v", f) + } + } + } + if len(tc.wantIDs) > 0 { + got := map[string]bool{} + for _, f := range out.Findings { + got[f.DetectorID] = true + } + for _, id := range tc.wantIDs { + if !got[id] { + t.Errorf("missing detector %q in filtered set: %v", id, got) + } + } + } + }) } } -func TestFilter_OriginalReportUnchanged(t *testing.T) { - r := sample() +func TestFilter_DoesNotMutateInputReport(t *testing.T) { + r := sampleReport(t) before := len(r.Findings) _ = Filter(r, FilterOptions{SecurityOnly: true}) if len(r.Findings) != before { @@ -77,13 +99,20 @@ func TestFilter_OriginalReportUnchanged(t *testing.T) { } func TestSeverityRank(t *testing.T) { - if severityRank(rules.SeverityHigh) <= severityRank(rules.SeverityMed) { - t.Error("HIGH should outrank MED") - } - if severityRank(rules.SeverityMed) <= severityRank(rules.SeverityLow) { - t.Error("MED should outrank LOW") - } - if severityRank(rules.Severity("nonsense")) != 0 { - t.Error("unknown severity should rank 0") + for _, tc := range []struct { + name string + a, b rules.Severity + want bool + }{ + {name: "high-outranks-med", a: rules.SeverityHigh, b: rules.SeverityMed, want: true}, + {name: "med-outranks-low", a: rules.SeverityMed, b: rules.SeverityLow, want: true}, + {name: "unknown-ranks-zero", a: rules.Severity("nonsense"), b: rules.SeverityInfo, want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + got := severityRank(tc.a) > severityRank(tc.b) + if got != tc.want { + t.Errorf("rank(%s) > rank(%s) = %v, want %v", tc.a, tc.b, got, tc.want) + } + }) } } diff --git a/internal/analyze/score_test.go b/internal/analyze/score_test.go index 4a5d1cf..ad7df22 100644 --- a/internal/analyze/score_test.go +++ b/internal/analyze/score_test.go @@ -6,72 +6,154 @@ import ( "github.com/optiqor/optiqor-cli/pkg/rules" ) -func TestCompute_NoFindings_PerfectScore(t *testing.T) { - s := Compute("clean", 3, nil) - if s.Value != 100 { - t.Errorf("Value = %d, want 100", s.Value) - } - if s.Band != rules.ConfidenceHigh { - t.Errorf("Band = %s, want high", s.Band) - } -} - -func TestCompute_HighSeverityDrops(t *testing.T) { - s := Compute("dirty", 1, []rules.Finding{ - {DetectorID: "a", Severity: rules.SeverityHigh}, - }) - if s.Value != 100-penaltyHigh { - t.Errorf("Value = %d, want %d", s.Value, 100-penaltyHigh) - } -} - -func TestCompute_PenaltyCapsAt100(t *testing.T) { - finds := []rules.Finding{} - for i := 0; i < 10; i++ { - finds = append(finds, rules.Finding{DetectorID: "a", Severity: rules.SeverityHigh}) - } - s := Compute("worst", 1, finds) - if s.Value != 0 { - t.Errorf("Value = %d, want 0 (cap)", s.Value) - } - if s.Band != rules.ConfidenceLow { - t.Errorf("Band = %s, want low", s.Band) +func TestCompute(t *testing.T) { + manyHigh := func(n int) []rules.Finding { + out := make([]rules.Finding, 0, n) + for i := 0; i < n; i++ { + out = append(out, rules.Finding{DetectorID: "a", Severity: rules.SeverityHigh}) + } + return out } -} - -func TestCompute_BandThresholds(t *testing.T) { - cases := []struct { + for _, tc := range []struct { + name string + source string + workers int findings []rules.Finding - minScore int - maxScore int - band rules.Confidence + check func(t *testing.T, s Score) }{ - {nil, 100, 100, rules.ConfidenceHigh}, - {[]rules.Finding{{Severity: rules.SeverityLow}}, 95, 100, rules.ConfidenceHigh}, // 100 - 3 = 97 - {[]rules.Finding{{Severity: rules.SeverityMed}, {Severity: rules.SeverityMed}}, 75, 85, rules.ConfidenceMed}, // 100 - 20 = 80 - {[]rules.Finding{{Severity: rules.SeverityHigh}, {Severity: rules.SeverityHigh}}, 45, 60, rules.ConfidenceLow}, // 100 - 50 = 50 - } - for i, tc := range cases { - s := Compute("x", 1, tc.findings) - if s.Value < tc.minScore || s.Value > tc.maxScore { - t.Errorf("case %d: Value = %d, want [%d,%d]", i, s.Value, tc.minScore, tc.maxScore) - } - if s.Band != tc.band { - t.Errorf("case %d: Band = %s, want %s", i, s.Band, tc.band) - } - } -} - -func TestCompute_PenaltiesPerDetector(t *testing.T) { - s := Compute("x", 1, []rules.Finding{ - {DetectorID: "a", Severity: rules.SeverityHigh}, - {DetectorID: "a", Severity: rules.SeverityMed}, - {DetectorID: "b", Severity: rules.SeverityLow}, - }) - if got, want := s.Penalties["a"], penaltyHigh+penaltyMed; got != want { - t.Errorf("a penalty = %d, want %d", got, want) - } - if got, want := s.Penalties["b"], penaltyLow; got != want { - t.Errorf("b penalty = %d, want %d", got, want) + { + name: "no-findings-perfect-score", + source: "clean", + workers: 3, + check: func(t *testing.T, s Score) { + t.Helper() + if s.Value != 100 { + t.Errorf("Value = %d, want 100", s.Value) + } + if s.Band != rules.ConfidenceHigh { + t.Errorf("Band = %s, want high", s.Band) + } + }, + }, + { + name: "high-severity-drops", + source: "dirty", + workers: 1, + findings: []rules.Finding{ + {DetectorID: "a", Severity: rules.SeverityHigh}, + }, + check: func(t *testing.T, s Score) { + t.Helper() + if s.Value != 100-penaltyHigh { + t.Errorf("Value = %d, want %d", s.Value, 100-penaltyHigh) + } + }, + }, + { + name: "penalty-caps-at-100", + source: "worst", + workers: 1, + findings: manyHigh(10), + check: func(t *testing.T, s Score) { + t.Helper() + if s.Value != 0 { + t.Errorf("Value = %d, want 0 (cap)", s.Value) + } + if s.Band != rules.ConfidenceLow { + t.Errorf("Band = %s, want low", s.Band) + } + }, + }, + { + name: "band-threshold-perfect-high", + source: "x", + workers: 1, + check: func(t *testing.T, s Score) { + t.Helper() + if s.Value < 100 || s.Value > 100 { + t.Errorf("Value = %d, want [100,100]", s.Value) + } + if s.Band != rules.ConfidenceHigh { + t.Errorf("Band = %s, want %s", s.Band, rules.ConfidenceHigh) + } + }, + }, + { + name: "band-threshold-low-severity-still-high", + source: "x", + workers: 1, + findings: []rules.Finding{{Severity: rules.SeverityLow}}, + check: func(t *testing.T, s Score) { + t.Helper() + // 100 - 3 = 97 + if s.Value < 95 || s.Value > 100 { + t.Errorf("Value = %d, want [95,100]", s.Value) + } + if s.Band != rules.ConfidenceHigh { + t.Errorf("Band = %s, want %s", s.Band, rules.ConfidenceHigh) + } + }, + }, + { + name: "band-threshold-two-med-mid-band", + source: "x", + workers: 1, + findings: []rules.Finding{ + {Severity: rules.SeverityMed}, + {Severity: rules.SeverityMed}, + }, + check: func(t *testing.T, s Score) { + t.Helper() + // 100 - 20 = 80 + if s.Value < 75 || s.Value > 85 { + t.Errorf("Value = %d, want [75,85]", s.Value) + } + if s.Band != rules.ConfidenceMed { + t.Errorf("Band = %s, want %s", s.Band, rules.ConfidenceMed) + } + }, + }, + { + name: "band-threshold-two-high-low-band", + source: "x", + workers: 1, + findings: []rules.Finding{ + {Severity: rules.SeverityHigh}, + {Severity: rules.SeverityHigh}, + }, + check: func(t *testing.T, s Score) { + t.Helper() + // 100 - 50 = 50 + if s.Value < 45 || s.Value > 60 { + t.Errorf("Value = %d, want [45,60]", s.Value) + } + if s.Band != rules.ConfidenceLow { + t.Errorf("Band = %s, want %s", s.Band, rules.ConfidenceLow) + } + }, + }, + { + name: "penalties-per-detector", + source: "x", + workers: 1, + findings: []rules.Finding{ + {DetectorID: "a", Severity: rules.SeverityHigh}, + {DetectorID: "a", Severity: rules.SeverityMed}, + {DetectorID: "b", Severity: rules.SeverityLow}, + }, + check: func(t *testing.T, s Score) { + t.Helper() + if got, want := s.Penalties["a"], penaltyHigh+penaltyMed; got != want { + t.Errorf("a penalty = %d, want %d", got, want) + } + if got, want := s.Penalties["b"], penaltyLow; got != want { + t.Errorf("b penalty = %d, want %d", got, want) + } + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + tc.check(t, Compute(tc.source, tc.workers, tc.findings)) + }) } } diff --git a/internal/config/config.go b/internal/config/config.go index 1f99b70..0e45c39 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -28,6 +28,7 @@ type Config struct { NoColor bool `yaml:"no_color,omitempty"` } +// ConfigName is the default filename Load looks for in the cwd. const ConfigName = ".optiqor.yaml" // Load returns the zero Config when no file is present (users opt in @@ -60,6 +61,7 @@ func readFile(path string) (Config, error) { return Decode(f) } +// Decode parses YAML config bytes and validates the result. func Decode(r io.Reader) (Config, error) { raw, err := io.ReadAll(r) if err != nil { @@ -78,6 +80,7 @@ func Decode(r io.Reader) (Config, error) { return c, nil } +// Validate rejects unknown severity / fail-on values. func (c Config) Validate() error { for _, key := range []struct { name, value string diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 47ca2e6..1b2a391 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -8,115 +8,192 @@ import ( "testing" ) -func TestLoad_NoFileReturnsZero(t *testing.T) { - dir := t.TempDir() - wd, _ := os.Getwd() - defer os.Chdir(wd) - _ = os.Chdir(dir) - - c, err := Load("") - if err != nil { - t.Fatalf("expected zero config, got err: %v", err) - } - if !reflect.DeepEqual(c, Config{}) { - t.Errorf("expected zero, got %+v", c) - } -} - -func TestLoad_PicksUpDotOptiqor(t *testing.T) { - dir := t.TempDir() - wd, _ := os.Getwd() - defer os.Chdir(wd) - _ = os.Chdir(dir) - - body := `min_severity: med +func TestLoad(t *testing.T) { + for _, tc := range []struct { + name string + setup func(t *testing.T) (explicit string) + wantErr bool + wantZero bool + check func(t *testing.T, c Config) + }{ + { + name: "no file returns zero", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + wd, _ := os.Getwd() + t.Cleanup(func() { _ = os.Chdir(wd) }) + _ = os.Chdir(dir) + return "" + }, + wantZero: true, + }, + { + name: "picks up dot optiqor in cwd", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + wd, _ := os.Getwd() + t.Cleanup(func() { _ = os.Chdir(wd) }) + _ = os.Chdir(dir) + body := `min_severity: med detectors: - cpu-overprovisioned - missing-memory-limit fail_on: high no_color: true ` - if err := os.WriteFile(filepath.Join(dir, ConfigName), []byte(body), 0o600); err != nil { - t.Fatal(err) - } - c, err := Load("") - if err != nil { - t.Fatalf("Load: %v", err) - } - if c.MinSeverity != "med" || c.FailOn != "high" || !c.NoColor { - t.Errorf("config not loaded: %+v", c) - } - if len(c.Detectors) != 2 { - t.Errorf("detectors lost: %v", c.Detectors) + if err := os.WriteFile(filepath.Join(dir, ConfigName), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return "" + }, + check: func(t *testing.T, c Config) { + t.Helper() + if c.MinSeverity != "med" || c.FailOn != "high" || !c.NoColor { + t.Errorf("config not loaded: %+v", c) + } + if len(c.Detectors) != 2 { + t.Errorf("detectors lost: %v", c.Detectors) + } + }, + }, + { + name: "explicit path", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "custom.yaml") + if err := os.WriteFile(path, []byte("min_severity: high"), 0o600); err != nil { + t.Fatal(err) + } + return path + }, + check: func(t *testing.T, c Config) { + t.Helper() + if c.MinSeverity != "high" { + t.Errorf("MinSeverity = %q", c.MinSeverity) + } + }, + }, + { + name: "explicit missing errors", + setup: func(_ *testing.T) string { return "/no/such/path" }, + wantErr: true, + }, + { + name: "env var fallback", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "env-config.yaml") + if err := os.WriteFile(path, []byte("min_severity: low"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("OPTIQOR_CONFIG", path) + return "" + }, + check: func(t *testing.T, c Config) { + t.Helper() + if c.MinSeverity != "low" { + t.Errorf("MinSeverity = %q", c.MinSeverity) + } + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + explicit := tc.setup(t) + c, err := Load(explicit) + if tc.wantErr { + if err == nil { + t.Fatal("expected error") + } + return + } + if err != nil { + t.Fatalf("Load: %v", err) + } + if tc.wantZero && !reflect.DeepEqual(c, Config{}) { + t.Errorf("expected zero, got %+v", c) + } + if tc.check != nil { + tc.check(t, c) + } + }) } } -func TestLoad_ExplicitPath(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "custom.yaml") - if err := os.WriteFile(path, []byte("min_severity: high"), 0o600); err != nil { - t.Fatal(err) - } - c, err := Load(path) - if err != nil { - t.Fatal(err) - } - if c.MinSeverity != "high" { - t.Errorf("MinSeverity = %q", c.MinSeverity) - } -} - -func TestLoad_ExplicitMissingErrors(t *testing.T) { - if _, err := Load("/no/such/path"); err == nil { - t.Fatal("expected error on missing explicit path") - } -} - -func TestLoad_EnvVarFallback(t *testing.T) { - dir := t.TempDir() - path := filepath.Join(dir, "env-config.yaml") - if err := os.WriteFile(path, []byte("min_severity: low"), 0o600); err != nil { - t.Fatal(err) - } - t.Setenv("OPTIQOR_CONFIG", path) - c, err := Load("") - if err != nil { - t.Fatal(err) - } - if c.MinSeverity != "low" { - t.Errorf("MinSeverity = %q", c.MinSeverity) - } -} - -func TestValidate_BadSeverity(t *testing.T) { - c := Config{MinSeverity: "noisy"} - err := c.Validate() - if err == nil || !strings.Contains(err.Error(), "min_severity") { - t.Fatalf("expected min_severity validation error, got %v", err) - } -} - -func TestValidate_AcceptsAliases(t *testing.T) { - for _, v := range []string{"low", "med", "medium", "high", "LOW", "Med", "HIGH"} { - c := Config{MinSeverity: v} - if err := c.Validate(); err != nil { - t.Errorf("Validate(%q): unexpected error %v", v, err) - } - } -} - -func TestDecode_EmptyOK(t *testing.T) { - c, err := Decode(strings.NewReader("")) - if err != nil { - t.Fatal(err) - } - if !reflect.DeepEqual(c, Config{}) { - t.Errorf("empty body should yield zero, got %+v", c) +func TestValidate(t *testing.T) { + for _, tc := range []struct { + name string + cfg Config + wantErrIn string // substring; "" means expect nil + }{ + { + name: "bad severity", + cfg: Config{MinSeverity: "noisy"}, + wantErrIn: "min_severity", + }, + {name: "low", cfg: Config{MinSeverity: "low"}}, + {name: "med", cfg: Config{MinSeverity: "med"}}, + {name: "medium alias", cfg: Config{MinSeverity: "medium"}}, + {name: "high", cfg: Config{MinSeverity: "high"}}, + {name: "uppercase LOW", cfg: Config{MinSeverity: "LOW"}}, + {name: "mixed case Med", cfg: Config{MinSeverity: "Med"}}, + {name: "uppercase HIGH", cfg: Config{MinSeverity: "HIGH"}}, + } { + t.Run(tc.name, func(t *testing.T) { + err := tc.cfg.Validate() + if tc.wantErrIn != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErrIn) { + t.Fatalf("want error containing %q, got %v", tc.wantErrIn, err) + } + return + } + if err != nil { + t.Errorf("Validate(%q): unexpected error %v", tc.cfg.MinSeverity, err) + } + }) } } -func TestDecode_BadYAML(t *testing.T) { - if _, err := Decode(strings.NewReader("not: valid: yaml::")); err == nil { - t.Fatal("expected yaml error") +func TestDecode(t *testing.T) { + for _, tc := range []struct { + name string + body string + wantErr bool + check func(t *testing.T, c Config) + }{ + { + name: "empty body yields zero", + body: "", + check: func(t *testing.T, c Config) { + t.Helper() + if !reflect.DeepEqual(c, Config{}) { + t.Errorf("got %+v", c) + } + }, + }, + { + name: "bad yaml errors", + body: "not: valid: yaml::", + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + c, err := Decode(strings.NewReader(tc.body)) + if tc.wantErr { + if err == nil { + t.Fatal("expected yaml error") + } + return + } + if err != nil { + t.Fatal(err) + } + if tc.check != nil { + tc.check(t, c) + } + }) } } diff --git a/internal/render/style/style.go b/internal/render/style/style.go index 680976d..9dfa523 100644 --- a/internal/render/style/style.go +++ b/internal/render/style/style.go @@ -172,6 +172,7 @@ func (t Theme) Hyperlink(label, url string) string { return fmt.Sprintf("\x1b]8;;%s\x1b\\%s\x1b]8;;\x1b\\", url, label) } +// DividerLine renders a width-many-rune box-drawing divider. func (t Theme) DividerLine(width int) string { if width <= 0 { width = 64 @@ -222,6 +223,7 @@ func (t Theme) SectionRule(label string, width int, accent lipgloss.Style) strin return rendered + accent.Render(repeat("━", remaining)) } +// SeverityBadge renders a fixed-width coloured chip for the severity. func (t Theme) SeverityBadge(sev string) string { switch sev { case "HIGH": @@ -235,6 +237,7 @@ func (t Theme) SeverityBadge(sev string) string { } } +// ConfidenceDots renders the high/medium/low band as three glyphs. func (t Theme) ConfidenceDots(conf string) string { switch conf { case "high": diff --git a/internal/render/text.go b/internal/render/text.go index 6ddf442..d496da0 100644 --- a/internal/render/text.go +++ b/internal/render/text.go @@ -21,6 +21,7 @@ import ( // (hard rule per CLAUDE.md — never soften, never make dismissible). const AccuracyDisclosure = "Sandbox accuracy: ±40%. Install the Optiqor agent for exact numbers (optiqor.dev/get)." +// Brand strings surfaced in the header banner. const ( BrandName = "optiqor" BrandTagline = "Helm chart cost optimization · security as a bonus" @@ -71,6 +72,8 @@ func (r Report) MonthlySavingsUSDCents() int64 { return sum } +// Text writes the terminal-friendly report. Always includes the +// AccuracyDisclosure footer (hard rule per CLAUDE.md). func Text(w io.Writer, r Report, opts Options) error { t := style.NewTheme(opts.Color) width := opts.Width diff --git a/internal/roast/roast_test.go b/internal/roast/roast_test.go index df310e4..5a8a1b2 100644 --- a/internal/roast/roast_test.go +++ b/internal/roast/roast_test.go @@ -8,84 +8,106 @@ import ( "github.com/optiqor/optiqor-cli/pkg/rules" ) -func TestApply_RewritesKnownTitles(t *testing.T) { - in := render.Report{ - Findings: []rules.Finding{ - {DetectorID: "cpu-overprovisioned", Title: "CPU request appears overprovisioned"}, - {DetectorID: "missing-memory-limit", Title: "Memory limit not set"}, +func TestApply(t *testing.T) { + for _, tc := range []struct { + name string + in render.Report + check func(t *testing.T, in, out render.Report) + }{ + { + name: "rewrites-known-titles", + in: render.Report{ + Findings: []rules.Finding{ + {DetectorID: "cpu-overprovisioned", Title: "CPU request appears overprovisioned"}, + {DetectorID: "missing-memory-limit", Title: "Memory limit not set"}, + }, + }, + check: func(t *testing.T, in, out render.Report) { + t.Helper() + if out.Findings[0].Title == in.Findings[0].Title { + t.Errorf("cpu-overprovisioned title should have been rewritten") + } + if out.Findings[1].Title == in.Findings[1].Title { + t.Errorf("missing-memory-limit title should have been rewritten") + } + }, }, - } - out := Apply(in) - if out.Findings[0].Title == in.Findings[0].Title { - t.Errorf("cpu-overprovisioned title should have been rewritten") - } - if out.Findings[1].Title == in.Findings[1].Title { - t.Errorf("missing-memory-limit title should have been rewritten") - } -} - -func TestApply_LeavesUnknownTitleAlone(t *testing.T) { - in := render.Report{ - Findings: []rules.Finding{ - {DetectorID: "future-detector-not-yet-roasted", Title: "Some title"}, + { + name: "leaves-unknown-title-alone", + in: render.Report{ + Findings: []rules.Finding{ + {DetectorID: "future-detector-not-yet-roasted", Title: "Some title"}, + }, + }, + check: func(t *testing.T, _, out render.Report) { + t.Helper() + if out.Findings[0].Title != "Some title" { + t.Errorf("unknown detector title was rewritten: %q", out.Findings[0].Title) + } + }, }, - } - out := Apply(in) - if out.Findings[0].Title != "Some title" { - t.Errorf("unknown detector title was rewritten: %q", out.Findings[0].Title) - } -} - -func TestApply_DoesNotMutateInput(t *testing.T) { - in := render.Report{ - Findings: []rules.Finding{ - {DetectorID: "cpu-overprovisioned", Title: "original"}, + { + name: "does-not-mutate-input", + in: render.Report{ + Findings: []rules.Finding{ + {DetectorID: "cpu-overprovisioned", Title: "original"}, + }, + }, + check: func(t *testing.T, in, _ render.Report) { + t.Helper() + if in.Findings[0].Title != "original" { + t.Errorf("input mutated: %q != %q", in.Findings[0].Title, "original") + } + }, }, - } - original := in.Findings[0].Title - _ = Apply(in) - if in.Findings[0].Title != original { - t.Errorf("input mutated: %q != %q", in.Findings[0].Title, original) - } -} - -func TestApply_PreservesMaterialFields(t *testing.T) { - // Hard rule: only Title changes. Detail, MonthlyUSDCents, - // Severity, Confidence, Category, Signal must round-trip. - in := render.Report{ - Findings: []rules.Finding{{ - DetectorID: "cpu-overprovisioned", - Workload: "api", - Title: "CPU request appears overprovisioned", - Detail: "Request 2 vs limit 2.5", - MonthlyUSDCents: 12345, - Severity: rules.SeverityMed, - Confidence: rules.ConfidenceMed, - Category: rules.CategoryCost, - Signal: &rules.Signal{ - Label: "CPU", Have: 2, Want: 2.5, - HaveDisplay: "2", WantDisplay: "2.5", Note: "80% of limit", + { + // Hard rule: only Title changes. Detail, MonthlyUSDCents, + // Severity, Confidence, Category, Signal must round-trip. + name: "preserves-material-fields", + in: render.Report{ + Findings: []rules.Finding{{ + DetectorID: "cpu-overprovisioned", + Workload: "api", + Title: "CPU request appears overprovisioned", + Detail: "Request 2 vs limit 2.5", + MonthlyUSDCents: 12345, + Severity: rules.SeverityMed, + Confidence: rules.ConfidenceMed, + Category: rules.CategoryCost, + Signal: &rules.Signal{ + Label: "CPU", Have: 2, Want: 2.5, + HaveDisplay: "2", WantDisplay: "2.5", Note: "80% of limit", + }, + }}, }, - }}, - } - out := Apply(in).Findings[0] - if out.Detail != "Request 2 vs limit 2.5" { - t.Errorf("Detail rewritten") - } - if out.MonthlyUSDCents != 12345 { - t.Errorf("MonthlyUSDCents changed") - } - if out.Severity != rules.SeverityMed { - t.Errorf("Severity changed") - } - if out.Confidence != rules.ConfidenceMed { - t.Errorf("Confidence changed") - } - if out.Category != rules.CategoryCost { - t.Errorf("Category changed") - } - if out.Signal == nil || out.Signal.Note != "80% of limit" { - t.Errorf("Signal lost or rewritten") + check: func(t *testing.T, _, out render.Report) { + t.Helper() + f := out.Findings[0] + if f.Detail != "Request 2 vs limit 2.5" { + t.Errorf("Detail rewritten") + } + if f.MonthlyUSDCents != 12345 { + t.Errorf("MonthlyUSDCents changed") + } + if f.Severity != rules.SeverityMed { + t.Errorf("Severity changed") + } + if f.Confidence != rules.ConfidenceMed { + t.Errorf("Confidence changed") + } + if f.Category != rules.CategoryCost { + t.Errorf("Category changed") + } + if f.Signal == nil || f.Signal.Note != "80% of limit" { + t.Errorf("Signal lost or rewritten") + } + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + out := Apply(tc.in) + tc.check(t, tc.in, out) + }) } } diff --git a/internal/share/share.go b/internal/share/share.go index 8baa513..1c1b910 100644 --- a/internal/share/share.go +++ b/internal/share/share.go @@ -20,6 +20,7 @@ import ( "time" ) +// BaseURL is the public share-page prefix. const BaseURL = "https://optiqor.dev/r/" // HashLen of 12 hex chars (48 bits) is collision-safe at Phase 1 @@ -72,6 +73,7 @@ func Hash(report any) (string, error) { return hex.EncodeToString(sum[:])[:HashLen], nil } +// URL is the canonical share URL: BaseURL + Hash(report). func URL(report any) (string, error) { h, err := Hash(report) if err != nil { @@ -101,8 +103,7 @@ const UploadEndpoint = "https://sandbox.optiqor.dev/api/v1/share" // uploadTimeout keeps tight — the CLI is interactive. const uploadTimeout = 5 * time.Second -// UploadResult: Hash + URL are always populated; Posted reports -// whether the HTTP POST returned 2xx. +// UploadResult always populates Hash + URL; Posted reports HTTP 2xx. type UploadResult struct { Hash string URL string diff --git a/internal/share/share_test.go b/internal/share/share_test.go index 7811fba..faef547 100644 --- a/internal/share/share_test.go +++ b/internal/share/share_test.go @@ -14,45 +14,50 @@ type sample struct { Findings []any `json:"findings"` } -func TestHash_Stable(t *testing.T) { - a := sample{Source: "/tmp/x", Workloads: 3, Findings: []any{ - map[string]any{"DetectorID": "cpu", "Severity": "MED"}, - }} - h1, err := Hash(a) - if err != nil { - t.Fatal(err) - } - h2, err := Hash(a) - if err != nil { - t.Fatal(err) - } - if h1 != h2 { - t.Errorf("hash unstable: %q vs %q", h1, h2) - } - if len(h1) != HashLen { - t.Errorf("hash len = %d, want %d", len(h1), HashLen) - } -} - -func TestHash_IgnoresSourcePath(t *testing.T) { - // Source-path differences must not change the hash — that's the - // point of sanitisation. - a := sample{Source: "/home/alice/chart", Workloads: 1} - b := sample{Source: "/home/bob/chart", Workloads: 1} - ha, _ := Hash(a) - hb, _ := Hash(b) - if ha != hb { - t.Errorf("source path leaked into hash: %q vs %q", ha, hb) - } -} - -func TestHash_WorkloadsAffectsHash(t *testing.T) { - a := sample{Workloads: 3} - b := sample{Workloads: 5} - ha, _ := Hash(a) - hb, _ := Hash(b) - if ha == hb { - t.Errorf("workload count should change the hash") +func TestHash(t *testing.T) { + for _, tc := range []struct { + name string + a, b sample + // equal=true asserts Hash(a) == Hash(b); false asserts they differ. + equal bool + }{ + { + name: "stable across calls", + a: sample{Source: "/tmp/x", Workloads: 3, Findings: []any{map[string]any{"DetectorID": "cpu", "Severity": "MED"}}}, + b: sample{Source: "/tmp/x", Workloads: 3, Findings: []any{map[string]any{"DetectorID": "cpu", "Severity": "MED"}}}, + equal: true, + }, + { + // Source-path differences must not change the hash — that's + // the point of sanitisation. + name: "ignores source path", + a: sample{Source: "/home/alice/chart", Workloads: 1}, + b: sample{Source: "/home/bob/chart", Workloads: 1}, + equal: true, + }, + { + name: "workload count affects hash", + a: sample{Workloads: 3}, + b: sample{Workloads: 5}, + equal: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ha, err := Hash(tc.a) + if err != nil { + t.Fatal(err) + } + hb, err := Hash(tc.b) + if err != nil { + t.Fatal(err) + } + if len(ha) != HashLen { + t.Errorf("hash len = %d, want %d", len(ha), HashLen) + } + if (ha == hb) != tc.equal { + t.Errorf("equal=%v: ha=%q hb=%q", tc.equal, ha, hb) + } + }) } } @@ -93,96 +98,121 @@ func TestSanitise_TruncatesLongDetail(t *testing.T) { } } -func TestUpload_PostsSanitisedJSON(t *testing.T) { - var ( - gotMethod string - gotHash string - gotContent string - gotBody []byte - ) - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotMethod = r.Method - gotHash = r.Header.Get("X-Optiqor-Hash") - gotContent = r.Header.Get("Content-Type") - gotBody, _ = io.ReadAll(r.Body) - w.WriteHeader(http.StatusAccepted) - })) - defer srv.Close() - - rep := sample{Source: "/tmp/x", Workloads: 2} - res := Upload(rep, srv.URL) - - if !res.Posted { - t.Fatalf("Posted = false, error = %q", res.Error) - } - if !IsHash(res.Hash) { - t.Errorf("Hash invalid: %q", res.Hash) - } - if !strings.HasPrefix(res.URL, BaseURL) { - t.Errorf("URL = %q", res.URL) - } - if gotMethod != http.MethodPost { - t.Errorf("method = %q", gotMethod) - } - if gotContent != "application/json" { - t.Errorf("content-type = %q", gotContent) - } - if gotHash != res.Hash { - t.Errorf("X-Optiqor-Hash = %q, want %q", gotHash, res.Hash) - } - // Body must NOT contain the source path (PII sanitisation). - if strings.Contains(string(gotBody), "/tmp/x") { - t.Errorf("source path leaked into upload body: %s", gotBody) - } -} - -func TestUpload_RejectsNon2xxAsError(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - })) - defer srv.Close() +func TestUpload(t *testing.T) { + for _, tc := range []struct { + name string + handler http.HandlerFunc + closeSrv bool // close the server before Upload — simulates network error + input sample + wantPost bool + wantErrIn string // substring expected in res.Error when wantPost=false + check func(t *testing.T, res UploadResult, captured map[string]string, body []byte) + }{ + { + name: "posts sanitised json", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusAccepted) + }, + input: sample{Source: "/tmp/x", Workloads: 2}, + wantPost: true, + check: func(t *testing.T, res UploadResult, captured map[string]string, body []byte) { + t.Helper() + if !IsHash(res.Hash) { + t.Errorf("Hash invalid: %q", res.Hash) + } + if !strings.HasPrefix(res.URL, BaseURL) { + t.Errorf("URL = %q", res.URL) + } + if captured["method"] != http.MethodPost { + t.Errorf("method = %q", captured["method"]) + } + if captured["content-type"] != "application/json" { + t.Errorf("content-type = %q", captured["content-type"]) + } + if captured["x-optiqor-hash"] != res.Hash { + t.Errorf("X-Optiqor-Hash = %q, want %q", captured["x-optiqor-hash"], res.Hash) + } + // Sanitisation contract: source path must not appear in + // the wire body. + if strings.Contains(string(body), "/tmp/x") { + t.Errorf("source path leaked into upload body: %s", body) + } + }, + }, + { + name: "non-2xx becomes error", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + input: sample{Workloads: 1}, + wantPost: false, + wantErrIn: "500", + }, + { + // Closed server → connection-refused style error. Hash/URL + // must still be populated so callers can show the local hash. + name: "network error graceful", + handler: func(http.ResponseWriter, *http.Request) {}, + closeSrv: true, + input: sample{Workloads: 1}, + wantPost: false, + wantErrIn: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + captured := map[string]string{} + var body []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured["method"] = r.Method + captured["content-type"] = r.Header.Get("Content-Type") + captured["x-optiqor-hash"] = r.Header.Get("X-Optiqor-Hash") + body, _ = io.ReadAll(r.Body) + tc.handler(w, r) + })) + if tc.closeSrv { + srv.Close() + } else { + defer srv.Close() + } - res := Upload(sample{Workloads: 1}, srv.URL) - if res.Posted { - t.Fatal("non-2xx must be reported as not-posted") - } - if !strings.Contains(res.Error, "500") { - t.Errorf("Error = %q, want it to mention status 500", res.Error) - } - if !IsHash(res.Hash) || res.URL == "" { - t.Errorf("hash/url should still be populated on failure: %+v", res) - } -} + res := Upload(tc.input, srv.URL) -func TestUpload_NetworkErrorGraceful(t *testing.T) { - // Closed server → connection-refused style error - srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) - srv.Close() - - res := Upload(sample{Workloads: 1}, srv.URL) - if res.Posted { - t.Fatal("upload to closed server should not be Posted") - } - if res.Error == "" { - t.Error("Error should describe the failure") - } - if !IsHash(res.Hash) { - t.Errorf("Hash should be populated even on network failure: %q", res.Hash) + if res.Posted != tc.wantPost { + t.Fatalf("Posted = %v want %v; Error=%q", res.Posted, tc.wantPost, res.Error) + } + if !IsHash(res.Hash) { + t.Errorf("Hash should be populated even on failure: %q", res.Hash) + } + if !tc.wantPost && tc.wantErrIn != "" && !strings.Contains(res.Error, tc.wantErrIn) { + t.Errorf("Error %q does not contain %q", res.Error, tc.wantErrIn) + } + if !tc.wantPost && tc.wantErrIn == "" && res.Error == "" { + t.Error("Error should describe the failure") + } + if tc.check != nil { + tc.check(t, res, captured, body) + } + }) } } func TestIsHash(t *testing.T) { - cases := map[string]bool{ - "": false, - "abc": false, - "abcdef012345": true, - "ABCDEF012345": true, - "abcdef01234g": false, - "abcdef0123456": false, // too long - } - for in, want := range cases { - if got := IsHash(in); got != want { - t.Errorf("IsHash(%q) = %v, want %v", in, got, want) - } + for _, tc := range []struct { + name string + in string + want bool + }{ + {"empty", "", false}, + {"too short", "abc", false}, + {"lowercase hex", "abcdef012345", true}, + {"uppercase hex", "ABCDEF012345", true}, + {"non-hex char", "abcdef01234g", false}, + {"too long", "abcdef0123456", false}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := IsHash(tc.in); got != tc.want { + t.Errorf("IsHash(%q) = %v, want %v", tc.in, got, tc.want) + } + }) } } diff --git a/pkg/htmlrender/htmlrender_test.go b/pkg/htmlrender/htmlrender_test.go index 1308f00..bcbe0ac 100644 --- a/pkg/htmlrender/htmlrender_test.go +++ b/pkg/htmlrender/htmlrender_test.go @@ -47,115 +47,191 @@ func sample() Data { } } -func TestRender_HappyPath(t *testing.T) { - out, err := RenderString(sample()) - if err != nil { - t.Fatal(err) - } - for _, want := range []string{ - "", - "Optiqor", - "Cost optimizations", - "Security findings", - "bonus", - "CPU request appears overprovisioned", - "Container runs as root", - "$316.80", // 22040 + 9640 cents = $316.80 - "±40%", - "share", +func TestRender(t *testing.T) { + for _, tc := range []struct { + name string + data func() Data + check func(t *testing.T, d Data) + }{ + { + name: "happy-path-contains-expected-markers", + data: sample, + check: func(t *testing.T, d Data) { + t.Helper() + out, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + for _, want := range []string{ + "", + "Optiqor", + "Cost optimizations", + "Security findings", + "bonus", + "CPU request appears overprovisioned", + "Container runs as root", + "$316.80", // 22040 + 9640 cents = $316.80 + "±40%", + "share", + } { + if !strings.Contains(out, want) { + t.Errorf("missing %q in output", want) + } + } + }, + }, + { + name: "no-share-url-omits-share-row", + data: func() Data { + d := sample() + d.ShareURL = "" + return d + }, + check: func(t *testing.T, d Data) { + t.Helper() + out, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + if strings.Contains(out, "data-copy=") { + t.Errorf("share UI should not render when ShareURL is empty") + } + }, + }, + { + name: "no-findings-clean-state", + data: func() Data { + d := sample() + d.Findings = nil + return d + }, + check: func(t *testing.T, d Data) { + t.Helper() + out, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, "Clean. No findings.") { + t.Errorf("clean state copy missing:\n%s", out) + } + }, + }, + { + name: "truncates-timestamp-to-minute", + data: sample, + check: func(t *testing.T, d Data) { + t.Helper() + t1 := time.Date(2026, 5, 11, 14, 30, 0, 0, time.UTC) + t2 := time.Date(2026, 5, 11, 14, 30, 45, 0, time.UTC) + d.GeneratedAt = t1 + a, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + d.GeneratedAt = t2 + b, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + if a != b { + t.Errorf("renders within the same minute must be byte-equal") + } + }, + }, + { + name: "cost-findings-lead-biggest-dollar", + data: sample, + check: func(t *testing.T, d Data) { + t.Helper() + out, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + cpuIdx := strings.Index(out, "CPU request appears overprovisioned") + memIdx := strings.Index(out, "Memory request appears overprovisioned") + if cpuIdx == -1 || memIdx == -1 { + t.Fatalf("missing finding lines") + } + if cpuIdx > memIdx { + t.Errorf("CPU ($220.40) should render before Memory ($96.40)") + } + }, + }, + { + name: "writes-to-writer", + data: sample, + check: func(t *testing.T, d Data) { + t.Helper() + var buf bytes.Buffer + if err := Render(&buf, d); err != nil { + t.Fatal(err) + } + if buf.Len() < 1024 { + t.Errorf("output suspiciously short: %d bytes", buf.Len()) + } + }, + }, } { - if !strings.Contains(out, want) { - t.Errorf("missing %q in output", want) - } - } -} - -func TestRender_DisclosureMandatory(t *testing.T) { - out, err := RenderString(sample()) - if err != nil { - t.Fatal(err) - } - if !strings.Contains(out, AccuracyDisclosure) { - t.Errorf("sandbox-mode output must contain the verbatim disclosure") - } -} - -func TestRender_AgentModeAccuracyLine(t *testing.T) { - d := sample() - d.Mode = ModeAgent - out, _ := RenderString(d) - if !strings.Contains(out, "Agent accuracy: ±15%") { - t.Errorf("agent mode must show ±15%% line") - } - if strings.Contains(out, AccuracyDisclosure) { - t.Errorf("agent mode should not also show sandbox disclosure") - } -} - -func TestRender_NoShareURL_OmitsShareRow(t *testing.T) { - d := sample() - d.ShareURL = "" - out, _ := RenderString(d) - if strings.Contains(out, "data-copy=") { - t.Errorf("share UI should not render when ShareURL is empty") - } -} - -func TestRender_NoFindings_CleanState(t *testing.T) { - d := sample() - d.Findings = nil - out, _ := RenderString(d) - if !strings.Contains(out, "Clean. No findings.") { - t.Errorf("clean state copy missing:\n%s", out) - } -} - -func TestRender_DeterministicAcrossRuns(t *testing.T) { - t1 := time.Date(2026, 5, 11, 14, 30, 0, 0, time.UTC) - t2 := time.Date(2026, 5, 11, 14, 30, 45, 0, time.UTC) - d := sample() - d.GeneratedAt = t1 - a, _ := RenderString(d) - d.GeneratedAt = t2 - b, _ := RenderString(d) - if a != b { - t.Errorf("renders within the same minute must be byte-equal") - } -} - -func TestRender_CostFindingsLeadBiggestDollar(t *testing.T) { - d := sample() - out, _ := RenderString(d) - cpuIdx := strings.Index(out, "CPU request appears overprovisioned") - memIdx := strings.Index(out, "Memory request appears overprovisioned") - if cpuIdx == -1 || memIdx == -1 { - t.Fatalf("missing finding lines") - } - if cpuIdx > memIdx { - t.Errorf("CPU ($220.40) should render before Memory ($96.40)") + t.Run(tc.name, func(t *testing.T) { + tc.check(t, tc.data()) + }) } } -func TestRender_WritesToWriter(t *testing.T) { - var buf bytes.Buffer - if err := Render(&buf, sample()); err != nil { - t.Fatal(err) - } - if buf.Len() < 1024 { - t.Errorf("output suspiciously short: %d bytes", buf.Len()) +func TestRender_AccuracyByMode(t *testing.T) { + for _, tc := range []struct { + name string + mode Mode + wantSubstr string + wantDisclosure bool + }{ + { + name: "sandbox-shows-mandatory-disclosure", + mode: ModeSandbox, + wantSubstr: AccuracyDisclosure, + wantDisclosure: true, + }, + { + name: "agent-shows-tighter-line-and-no-sandbox-disclosure", + mode: ModeAgent, + wantSubstr: "Agent accuracy: ±15%", + wantDisclosure: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + d := sample() + d.Mode = tc.mode + out, err := RenderString(d) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, tc.wantSubstr) { + t.Errorf("missing %q in %s output", tc.wantSubstr, tc.name) + } + hasDisclosure := strings.Contains(out, AccuracyDisclosure) + if hasDisclosure != tc.wantDisclosure { + t.Errorf("disclosure presence = %v, want %v", hasDisclosure, tc.wantDisclosure) + } + }) } } func TestFmtUSD(t *testing.T) { - for cents, want := range map[int64]string{ - 0: "$0", - 1: "$0.01", - 100: "$1", - 12345: "$123.45", - 12_345_678: "$123,456.78", + for _, tc := range []struct { + name string + cents int64 + want string + }{ + {name: "zero", cents: 0, want: "$0"}, + {name: "one-cent", cents: 1, want: "$0.01"}, + {name: "exact-dollar", cents: 100, want: "$1"}, + {name: "dollars-and-cents", cents: 12345, want: "$123.45"}, + {name: "thousands-separator", cents: 12_345_678, want: "$123,456.78"}, } { - if got := fmtUSD(cents); got != want { - t.Errorf("fmtUSD(%d) = %q, want %q", cents, got, want) - } + t.Run(tc.name, func(t *testing.T) { + if got := fmtUSD(tc.cents); got != tc.want { + t.Errorf("fmtUSD(%d) = %q, want %q", tc.cents, got, tc.want) + } + }) } } diff --git a/pkg/parser/helm_test.go b/pkg/parser/helm_test.go index b499d60..221fe40 100644 --- a/pkg/parser/helm_test.go +++ b/pkg/parser/helm_test.go @@ -5,8 +5,16 @@ import ( "testing" ) -func TestParseValues_Flat(t *testing.T) { - in := ` +func TestParseValues(t *testing.T) { + for _, tc := range []struct { + name string + in string + wantErr bool + check func(t *testing.T, wls []Workload) + }{ + { + name: "flat-workloads", + in: ` api: resources: requests: @@ -21,90 +29,81 @@ worker: cpu: 500m limits: cpu: 1 -` - wls, err := ParseValues(strings.NewReader(in)) - if err != nil { - t.Fatalf("ParseValues: %v", err) - } - if len(wls) != 2 { - t.Fatalf("expected 2 workloads, got %d: %+v", len(wls), wls) - } - - if wls[0].Name != "api" { - t.Errorf("wls[0].Name = %q, want api", wls[0].Name) - } - if wls[0].Requests.CPU.Value != 1000 { - t.Errorf("api requests cpu = %d, want 1000", wls[0].Requests.CPU.Value) - } - if wls[0].Limits.Memory.Value != 4*1024*1024*1024 { - t.Errorf("api limits memory = %d, want %d", wls[0].Limits.Memory.Value, 4*1024*1024*1024) - } - - if wls[1].Name != "worker" { - t.Errorf("wls[1].Name = %q, want worker", wls[1].Name) - } - if wls[1].Requests.Memory.Set { - t.Errorf("worker requests memory should be unset") - } - if wls[1].Limits.Memory.Set { - t.Errorf("worker limits memory should be unset") - } -} - -func TestParseValues_NestedSubchart(t *testing.T) { - in := ` +`, + check: func(t *testing.T, wls []Workload) { + t.Helper() + if len(wls) != 2 { + t.Fatalf("expected 2 workloads, got %d: %+v", len(wls), wls) + } + if wls[0].Name != "api" { + t.Errorf("wls[0].Name = %q, want api", wls[0].Name) + } + if wls[0].Requests.CPU.Value != 1000 { + t.Errorf("api requests cpu = %d, want 1000", wls[0].Requests.CPU.Value) + } + if wls[0].Limits.Memory.Value != 4*1024*1024*1024 { + t.Errorf("api limits memory = %d, want %d", wls[0].Limits.Memory.Value, 4*1024*1024*1024) + } + if wls[1].Name != "worker" { + t.Errorf("wls[1].Name = %q, want worker", wls[1].Name) + } + if wls[1].Requests.Memory.Set { + t.Errorf("worker requests memory should be unset") + } + if wls[1].Limits.Memory.Set { + t.Errorf("worker limits memory should be unset") + } + }, + }, + { + name: "nested-subchart-flattens-dotted", + in: ` postgresql: primary: resources: requests: cpu: 2 memory: 8Gi -` - wls, err := ParseValues(strings.NewReader(in)) - if err != nil { - t.Fatalf("ParseValues: %v", err) - } - if len(wls) != 1 { - t.Fatalf("expected 1 workload, got %+v", wls) - } - if wls[0].Name != "postgresql.primary" { - t.Errorf("nested name = %q, want postgresql.primary", wls[0].Name) - } -} - -func TestParseValues_NoWorkloads(t *testing.T) { - in := ` +`, + check: func(t *testing.T, wls []Workload) { + t.Helper() + if len(wls) != 1 { + t.Fatalf("expected 1 workload, got %+v", wls) + } + if wls[0].Name != "postgresql.primary" { + t.Errorf("nested name = %q, want postgresql.primary", wls[0].Name) + } + }, + }, + { + name: "no-resource-blocks-yields-empty", + in: ` config: level: info features: - one - two -` - wls, err := ParseValues(strings.NewReader(in)) - if err != nil { - t.Fatalf("ParseValues: %v", err) - } - if len(wls) != 0 { - t.Errorf("expected 0 workloads, got %+v", wls) - } -} - -func TestParseValues_BadYAML(t *testing.T) { - in := "this is: not: valid: yaml::" - if _, err := ParseValues(strings.NewReader(in)); err == nil { - t.Fatal("expected error on malformed yaml") - } -} - -func TestParseValues_TopLevelNotMap(t *testing.T) { - in := "- a\n- b\n" - if _, err := ParseValues(strings.NewReader(in)); err == nil { - t.Fatal("expected error when top level is a sequence") - } -} - -func TestParseValues_DeterministicOrder(t *testing.T) { - in := ` +`, + check: func(t *testing.T, wls []Workload) { + t.Helper() + if len(wls) != 0 { + t.Errorf("expected 0 workloads, got %+v", wls) + } + }, + }, + { + name: "malformed-yaml-errors", + in: "this is: not: valid: yaml::", + wantErr: true, + }, + { + name: "top-level-sequence-errors", + in: "- a\n- b\n", + wantErr: true, + }, + { + name: "deterministic-alpha-order", + in: ` zeta: resources: requests: {cpu: 1} @@ -114,15 +113,32 @@ alpha: mike: resources: requests: {cpu: 1} -` - wls, err := ParseValues(strings.NewReader(in)) - if err != nil { - t.Fatal(err) - } - want := []string{"alpha", "mike", "zeta"} - for i := range want { - if wls[i].Name != want[i] { - t.Errorf("wls[%d].Name = %q, want %q", i, wls[i].Name, want[i]) - } +`, + check: func(t *testing.T, wls []Workload) { + t.Helper() + want := []string{"alpha", "mike", "zeta"} + for i := range want { + if wls[i].Name != want[i] { + t.Errorf("wls[%d].Name = %q, want %q", i, wls[i].Name, want[i]) + } + } + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + wls, err := ParseValues(strings.NewReader(tc.in)) + if tc.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("ParseValues: %v", err) + } + if tc.check != nil { + tc.check(t, wls) + } + }) } } diff --git a/pkg/parser/quantity_test.go b/pkg/parser/quantity_test.go index 5d954cf..5cf6c54 100644 --- a/pkg/parser/quantity_test.go +++ b/pkg/parser/quantity_test.go @@ -3,119 +3,132 @@ package parser import "testing" func TestParseCPU(t *testing.T) { - cases := []struct { + for _, tc := range []struct { + name string in string want int64 set bool wantErr bool }{ - {"", 0, false, false}, - {"500m", 500, true, false}, - {"1", 1000, true, false}, - {"2.5", 2500, true, false}, - {"0", 0, true, false}, - {"-1", 0, false, true}, - {"100x", 0, false, true}, - {"abc", 0, false, true}, - } - for _, tc := range cases { - q, err := ParseCPU(tc.in) - if tc.wantErr { - if err == nil { - t.Errorf("ParseCPU(%q): expected error", tc.in) + {name: "empty-yields-unset", in: "", want: 0, set: false}, + {name: "millicore-suffix", in: "500m", want: 500, set: true}, + {name: "whole-core", in: "1", want: 1000, set: true}, + {name: "fractional-core", in: "2.5", want: 2500, set: true}, + {name: "zero-is-set", in: "0", want: 0, set: true}, + {name: "negative-errors", in: "-1", wantErr: true}, + {name: "trailing-junk-errors", in: "100x", wantErr: true}, + {name: "non-numeric-errors", in: "abc", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + q, err := ParseCPU(tc.in) + if tc.wantErr { + if err == nil { + t.Errorf("ParseCPU(%q): expected error", tc.in) + } + return + } + if err != nil { + t.Fatalf("ParseCPU(%q): unexpected error: %v", tc.in, err) + } + if q.Value != tc.want || q.Set != tc.set { + t.Errorf("ParseCPU(%q) = {%d %v}, want {%d %v}", tc.in, q.Value, q.Set, tc.want, tc.set) } - continue - } - if err != nil { - t.Errorf("ParseCPU(%q): unexpected error: %v", tc.in, err) - continue - } - if q.Value != tc.want || q.Set != tc.set { - t.Errorf("ParseCPU(%q) = {%d %v}, want {%d %v}", tc.in, q.Value, q.Set, tc.want, tc.set) - } + }) } } func TestParseMemory(t *testing.T) { - cases := []struct { + for _, tc := range []struct { + name string in string want int64 set bool wantErr bool }{ - {"", 0, false, false}, - {"1024", 1024, true, false}, - {"1Ki", 1024, true, false}, - {"1Mi", 1024 * 1024, true, false}, - {"512Mi", 512 * 1024 * 1024, true, false}, - {"2Gi", 2 * 1024 * 1024 * 1024, true, false}, - {"1G", 1_000_000_000, true, false}, - {"100M", 100_000_000, true, false}, - {"-1Mi", 0, false, true}, - {"abcMi", 0, false, true}, - } - for _, tc := range cases { - q, err := ParseMemory(tc.in) - if tc.wantErr { - if err == nil { - t.Errorf("ParseMemory(%q): expected error", tc.in) + {name: "empty-yields-unset", in: "", want: 0, set: false}, + {name: "plain-bytes", in: "1024", want: 1024, set: true}, + {name: "Ki-binary", in: "1Ki", want: 1024, set: true}, + {name: "Mi-binary", in: "1Mi", want: 1024 * 1024, set: true}, + {name: "512Mi", in: "512Mi", want: 512 * 1024 * 1024, set: true}, + {name: "Gi-binary", in: "2Gi", want: 2 * 1024 * 1024 * 1024, set: true}, + {name: "G-decimal", in: "1G", want: 1_000_000_000, set: true}, + {name: "M-decimal", in: "100M", want: 100_000_000, set: true}, + {name: "negative-errors", in: "-1Mi", wantErr: true}, + {name: "non-numeric-errors", in: "abcMi", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + q, err := ParseMemory(tc.in) + if tc.wantErr { + if err == nil { + t.Errorf("ParseMemory(%q): expected error", tc.in) + } + return + } + if err != nil { + t.Fatalf("ParseMemory(%q): unexpected error: %v", tc.in, err) } - continue - } - if err != nil { - t.Errorf("ParseMemory(%q): unexpected error: %v", tc.in, err) - continue - } - if q.Value != tc.want || q.Set != tc.set { - t.Errorf("ParseMemory(%q) = {%d %v}, want {%d %v}", tc.in, q.Value, q.Set, tc.want, tc.set) - } + if q.Value != tc.want || q.Set != tc.set { + t.Errorf("ParseMemory(%q) = {%d %v}, want {%d %v}", tc.in, q.Value, q.Set, tc.want, tc.set) + } + }) } } func TestFormatCPU(t *testing.T) { - cases := map[int64]string{ - 0: "0", - 500: "500m", - 1000: "1", - 1500: "1500m", - 2000: "2", - } - for in, want := range cases { - got := FormatCPU(Quantity{Value: in, Set: true}) - if got != want { - t.Errorf("FormatCPU(%d) = %q, want %q", in, got, want) - } - } - if got := FormatCPU(Quantity{}); got != "(unset)" { - t.Errorf("FormatCPU(unset) = %q", got) + for _, tc := range []struct { + name string + in Quantity + want string + }{ + {name: "zero", in: Quantity{Value: 0, Set: true}, want: "0"}, + {name: "sub-core-millis", in: Quantity{Value: 500, Set: true}, want: "500m"}, + {name: "whole-core", in: Quantity{Value: 1000, Set: true}, want: "1"}, + {name: "fractional-stays-millis", in: Quantity{Value: 1500, Set: true}, want: "1500m"}, + {name: "two-cores", in: Quantity{Value: 2000, Set: true}, want: "2"}, + {name: "unset-sentinel", in: Quantity{}, want: "(unset)"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := FormatCPU(tc.in); got != tc.want { + t.Errorf("FormatCPU(%+v) = %q, want %q", tc.in, got, tc.want) + } + }) } } func TestFormatMemory(t *testing.T) { - cases := []struct { + for _, tc := range []struct { + name string v int64 want string }{ - {0, "0B"}, - {1024, "1.0Ki"}, - {1024 * 1024, "1.0Mi"}, - {2 * 1024 * 1024 * 1024, "2.0Gi"}, - {1024 * 1024 * 1024 * 1024, "1.0Ti"}, - } - for _, tc := range cases { - got := FormatMemory(Quantity{Value: tc.v, Set: true}) - if got != tc.want { - t.Errorf("FormatMemory(%d) = %q, want %q", tc.v, got, tc.want) - } + {name: "zero-bytes", v: 0, want: "0B"}, + {name: "one-Ki", v: 1024, want: "1.0Ki"}, + {name: "one-Mi", v: 1024 * 1024, want: "1.0Mi"}, + {name: "two-Gi", v: 2 * 1024 * 1024 * 1024, want: "2.0Gi"}, + {name: "one-Ti", v: 1024 * 1024 * 1024 * 1024, want: "1.0Ti"}, + } { + t.Run(tc.name, func(t *testing.T) { + got := FormatMemory(Quantity{Value: tc.v, Set: true}) + if got != tc.want { + t.Errorf("FormatMemory(%d) = %q, want %q", tc.v, got, tc.want) + } + }) } } func TestQuantityString(t *testing.T) { - q := Quantity{Original: "500m", Set: true} - if q.String() != "500m" { - t.Errorf("String() = %q", q.String()) - } - if (Quantity{}).String() != "(unset)" { - t.Error("unset String() should be (unset)") + for _, tc := range []struct { + name string + in Quantity + want string + }{ + {name: "set-returns-original", in: Quantity{Original: "500m", Set: true}, want: "500m"}, + {name: "unset-returns-sentinel", in: Quantity{}, want: "(unset)"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := tc.in.String(); got != tc.want { + t.Errorf("String() = %q, want %q", got, tc.want) + } + }) } } diff --git a/pkg/rules/new_detectors_test.go b/pkg/rules/new_detectors_test.go index 07f6eb3..d86ba24 100644 --- a/pkg/rules/new_detectors_test.go +++ b/pkg/rules/new_detectors_test.go @@ -7,103 +7,143 @@ import ( "github.com/optiqor/optiqor-cli/pkg/parser" ) -func TestMemoryOverprovisioned_Triggers(t *testing.T) { - w := parser.Workload{ - Name: "api", - Requests: parser.ResourceList{Memory: memQ(2 * 1024 * 1024 * 1024)}, // 2GiB - Limits: parser.ResourceList{Memory: memQ(int64(2.5 * 1024 * 1024 * 1024))}, - } - f := newMemoryOverprovisioned().Run(w) - if len(f) != 1 { - t.Fatalf("expected 1 finding, got %d", len(f)) - } - if f[0].Severity != SeverityMed { - t.Errorf("severity = %s", f[0].Severity) - } - if f[0].MonthlyUSDCents <= 0 { - t.Errorf("expected positive savings, got %d", f[0].MonthlyUSDCents) - } -} - -func TestMemoryOverprovisioned_DoesNotTriggerBelowRatio(t *testing.T) { - w := parser.Workload{ - Requests: parser.ResourceList{Memory: memQ(256 * 1024 * 1024)}, - Limits: parser.ResourceList{Memory: memQ(2 * 1024 * 1024 * 1024)}, - } - if f := newMemoryOverprovisioned().Run(w); len(f) != 0 { - t.Fatalf("ratio 0.125 should not fire; got %v", f) - } -} - -func TestMissingCPULimit_Triggers(t *testing.T) { - w := parser.Workload{ - Requests: parser.ResourceList{CPU: cpuQ(500)}, - Limits: parser.ResourceList{}, - } - f := newMissingCPULimit().Run(w) - if len(f) != 1 { - t.Fatalf("expected 1 finding") - } - if f[0].Severity != SeverityLow { - t.Errorf("severity = %s", f[0].Severity) - } -} - -func TestMissingCPULimit_NoTrigger(t *testing.T) { - w := parser.Workload{ - Requests: parser.ResourceList{CPU: cpuQ(500)}, - Limits: parser.ResourceList{CPU: cpuQ(1000)}, - } - if f := newMissingCPULimit().Run(w); len(f) != 0 { - t.Fatalf("limit set → no finding; got %v", f) +func TestMemoryOverprovisioned(t *testing.T) { + for _, tc := range []struct { + name string + in parser.Workload + wantCount int + wantSev Severity + wantSavings bool + }{ + { + name: "request-near-limit-triggers", + in: parser.Workload{ + Name: "api", + Requests: parser.ResourceList{Memory: memQ(2 * 1024 * 1024 * 1024)}, // 2GiB + Limits: parser.ResourceList{Memory: memQ(int64(2.5 * 1024 * 1024 * 1024))}, + }, + wantCount: 1, + wantSev: SeverityMed, + wantSavings: true, + }, + { + name: "ratio-below-threshold-quiet", + in: parser.Workload{ + Requests: parser.ResourceList{Memory: memQ(256 * 1024 * 1024)}, + Limits: parser.ResourceList{Memory: memQ(2 * 1024 * 1024 * 1024)}, + }, + wantCount: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := newMemoryOverprovisioned().Run(tc.in) + if len(f) != tc.wantCount { + t.Fatalf("findings = %d, want %d: %+v", len(f), tc.wantCount, f) + } + if tc.wantCount == 0 { + return + } + if f[0].Severity != tc.wantSev { + t.Errorf("severity = %s, want %s", f[0].Severity, tc.wantSev) + } + if tc.wantSavings && f[0].MonthlyUSDCents <= 0 { + t.Errorf("expected positive savings, got %d", f[0].MonthlyUSDCents) + } + }) } } -func TestImagePinnedLatest_TriggersOnLiteralLatest(t *testing.T) { - w := parser.Workload{ - Name: "cache", - Image: parser.ImageRef{Repository: "redis", Tag: "latest", Set: true}, - } - f := newImagePinnedLatest().Run(w) - if len(f) != 1 { - t.Fatalf("expected 1 finding") - } - if !strings.Contains(f[0].Detail, ":latest") { - t.Errorf("detail should mention :latest: %q", f[0].Detail) - } -} - -func TestImagePinnedLatest_TriggersOnMissingTag(t *testing.T) { - w := parser.Workload{ - Name: "cache", - Image: parser.ImageRef{Repository: "redis", Tag: "", Set: true}, - } - f := newImagePinnedLatest().Run(w) - if len(f) != 1 { - t.Fatalf("expected 1 finding for missing tag") - } -} - -func TestImagePinnedLatest_DoesNotTriggerOnVersionTag(t *testing.T) { - w := parser.Workload{ - Image: parser.ImageRef{Repository: "redis", Tag: "7.2.4", Set: true}, - } - if f := newImagePinnedLatest().Run(w); len(f) != 0 { - t.Fatalf("version tag should not fire; got %v", f) +func TestMissingCPULimit(t *testing.T) { + for _, tc := range []struct { + name string + in parser.Workload + wantCount int + }{ + { + name: "missing-limit-triggers-low", + in: parser.Workload{ + Requests: parser.ResourceList{CPU: cpuQ(500)}, + Limits: parser.ResourceList{}, + }, + wantCount: 1, + }, + { + name: "limit-present-quiet", + in: parser.Workload{ + Requests: parser.ResourceList{CPU: cpuQ(500)}, + Limits: parser.ResourceList{CPU: cpuQ(1000)}, + }, + wantCount: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := newMissingCPULimit().Run(tc.in) + if len(f) != tc.wantCount { + t.Fatalf("findings = %d, want %d: %+v", len(f), tc.wantCount, f) + } + if tc.wantCount == 0 { + return + } + if f[0].Severity != SeverityLow { + t.Errorf("severity = %s, want LOW", f[0].Severity) + } + }) } } -func TestImagePinnedLatest_NoImage(t *testing.T) { - w := parser.Workload{Name: "x"} - if f := newImagePinnedLatest().Run(w); len(f) != 0 { - t.Fatalf("no image → no finding; got %v", f) +func TestImagePinnedLatest(t *testing.T) { + for _, tc := range []struct { + name string + in parser.Workload + wantCount int + wantDetailHint string + }{ + { + name: "explicit-latest-tag-triggers", + in: parser.Workload{ + Name: "cache", + Image: parser.ImageRef{Repository: "redis", Tag: "latest", Set: true}, + }, + wantCount: 1, + wantDetailHint: ":latest", + }, + { + name: "missing-tag-defaults-to-latest", + in: parser.Workload{ + Name: "cache", + Image: parser.ImageRef{Repository: "redis", Tag: "", Set: true}, + }, + wantCount: 1, + }, + { + name: "semver-tag-quiet", + in: parser.Workload{ + Image: parser.ImageRef{Repository: "redis", Tag: "7.2.4", Set: true}, + }, + wantCount: 0, + }, + { + name: "no-image-quiet", + in: parser.Workload{Name: "x"}, + wantCount: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := newImagePinnedLatest().Run(tc.in) + if len(f) != tc.wantCount { + t.Fatalf("findings = %d, want %d: %+v", len(f), tc.wantCount, f) + } + if tc.wantDetailHint != "" && !strings.Contains(f[0].Detail, tc.wantDetailHint) { + t.Errorf("detail should mention %q: %q", tc.wantDetailHint, f[0].Detail) + } + }) } } // TestAll_DetectorCountAndUniqueIDs guards the wire-stable detector -// IDs and the Phase 3 count (15 cost + 15 security). +// IDs and the running Phase 3+ count. func TestAll_DetectorCountAndUniqueIDs(t *testing.T) { - const want = 31 // 16 cost + 15 security (idle-workload added 2026-05-18) + const want = 31 // 15 cost + 15 security + idle-workload (#32) dets := All() if len(dets) != want { t.Fatalf("All() returned %d detectors, want %d", len(dets), want) diff --git a/pkg/rules/rules_test.go b/pkg/rules/rules_test.go index 83b62b5..9ad6cdd 100644 --- a/pkg/rules/rules_test.go +++ b/pkg/rules/rules_test.go @@ -10,81 +10,109 @@ import ( func cpuQ(v int64) parser.Quantity { return parser.Quantity{Value: v, Set: true, Original: "x"} } func memQ(v int64) parser.Quantity { return parser.Quantity{Value: v, Set: true, Original: "x"} } -func TestCPUOverprovisioned_Triggers(t *testing.T) { - w := parser.Workload{ - Name: "api", - Requests: parser.ResourceList{CPU: cpuQ(2000), Memory: memQ(1024)}, - Limits: parser.ResourceList{CPU: cpuQ(2500), Memory: memQ(2048)}, - } - f := newCPUOverprovisioned().Run(w) - if len(f) != 1 { - t.Fatalf("expected 1 finding, got %d", len(f)) - } - if f[0].Severity != SeverityMed { - t.Errorf("severity = %s", f[0].Severity) - } - if f[0].MonthlyUSDCents <= 0 { - t.Errorf("expected positive savings, got %d", f[0].MonthlyUSDCents) - } -} - -func TestCPUOverprovisioned_DoesNotTriggerBelowRatio(t *testing.T) { - w := parser.Workload{ - Name: "api", - Requests: parser.ResourceList{CPU: cpuQ(500)}, - Limits: parser.ResourceList{CPU: cpuQ(2000)}, - } - if f := newCPUOverprovisioned().Run(w); len(f) != 0 { - t.Fatalf("expected no findings (ratio 0.25); got %v", f) - } -} - -func TestCPUOverprovisioned_NoLimit(t *testing.T) { - w := parser.Workload{ - Name: "api", - Requests: parser.ResourceList{CPU: cpuQ(500)}, - } - if f := newCPUOverprovisioned().Run(w); len(f) != 0 { - t.Fatalf("no limit -> no finding; got %v", f) - } -} - -func TestCPUOverprovisioned_NoRequest(t *testing.T) { - w := parser.Workload{ - Name: "api", - Limits: parser.ResourceList{CPU: cpuQ(1000)}, - } - if f := newCPUOverprovisioned().Run(w); len(f) != 0 { - t.Fatalf("no request -> no finding; got %v", f) - } -} - -func TestMissingMemoryLimit_Triggers(t *testing.T) { - w := parser.Workload{ - Name: "worker", - Requests: parser.ResourceList{Memory: memQ(1024 * 1024 * 1024)}, - Limits: parser.ResourceList{}, - } - f := newMissingMemoryLimit().Run(w) - if len(f) != 1 { - t.Fatalf("expected 1 finding, got %d: %+v", len(f), f) - } - if f[0].Severity != SeverityHigh { - t.Errorf("severity = %s", f[0].Severity) - } - if !strings.Contains(f[0].Detail, "P95") { - t.Errorf("detail should mention P95: %q", f[0].Detail) +func TestCPUOverprovisioned(t *testing.T) { + for _, tc := range []struct { + name string + in parser.Workload + wantCount int + wantSev Severity + wantSavings bool + }{ + { + name: "request-near-limit-triggers", + in: parser.Workload{ + Name: "api", + Requests: parser.ResourceList{CPU: cpuQ(2000), Memory: memQ(1024)}, + Limits: parser.ResourceList{CPU: cpuQ(2500), Memory: memQ(2048)}, + }, + wantCount: 1, + wantSev: SeverityMed, + wantSavings: true, + }, + { + name: "ratio-below-threshold-quiet", + in: parser.Workload{ + Name: "api", + Requests: parser.ResourceList{CPU: cpuQ(500)}, + Limits: parser.ResourceList{CPU: cpuQ(2000)}, + }, + wantCount: 0, + }, + { + name: "no-limit-cannot-compare", + in: parser.Workload{ + Name: "api", + Requests: parser.ResourceList{CPU: cpuQ(500)}, + }, + wantCount: 0, + }, + { + name: "no-request-cannot-compare", + in: parser.Workload{ + Name: "api", + Limits: parser.ResourceList{CPU: cpuQ(1000)}, + }, + wantCount: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := newCPUOverprovisioned().Run(tc.in) + if len(f) != tc.wantCount { + t.Fatalf("findings = %d, want %d: %+v", len(f), tc.wantCount, f) + } + if tc.wantCount == 0 { + return + } + if f[0].Severity != tc.wantSev { + t.Errorf("severity = %s, want %s", f[0].Severity, tc.wantSev) + } + if tc.wantSavings && f[0].MonthlyUSDCents <= 0 { + t.Errorf("expected positive savings, got %d", f[0].MonthlyUSDCents) + } + }) } } -func TestMissingMemoryLimit_DoesNotTriggerWhenLimitSet(t *testing.T) { - w := parser.Workload{ - Name: "worker", - Requests: parser.ResourceList{Memory: memQ(1024 * 1024 * 1024)}, - Limits: parser.ResourceList{Memory: memQ(2 * 1024 * 1024 * 1024)}, - } - if f := newMissingMemoryLimit().Run(w); len(f) != 0 { - t.Fatalf("limit set -> no finding; got %v", f) +func TestMissingMemoryLimit(t *testing.T) { + for _, tc := range []struct { + name string + in parser.Workload + wantCount int + }{ + { + name: "missing-limit-triggers-high", + in: parser.Workload{ + Name: "worker", + Requests: parser.ResourceList{Memory: memQ(1024 * 1024 * 1024)}, + Limits: parser.ResourceList{}, + }, + wantCount: 1, + }, + { + name: "limit-present-quiet", + in: parser.Workload{ + Name: "worker", + Requests: parser.ResourceList{Memory: memQ(1024 * 1024 * 1024)}, + Limits: parser.ResourceList{Memory: memQ(2 * 1024 * 1024 * 1024)}, + }, + wantCount: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + f := newMissingMemoryLimit().Run(tc.in) + if len(f) != tc.wantCount { + t.Fatalf("findings = %d, want %d: %+v", len(f), tc.wantCount, f) + } + if tc.wantCount == 0 { + return + } + if f[0].Severity != SeverityHigh { + t.Errorf("severity = %s, want HIGH", f[0].Severity) + } + if !strings.Contains(f[0].Detail, "P95") { + t.Errorf("detail should mention P95: %q", f[0].Detail) + } + }) } } @@ -102,7 +130,7 @@ func TestRun_StableOrder(t *testing.T) { } got := Run(wls, All()) if len(got) < 2 { - t.Fatalf("expected ≥2 findings, got %d: %+v", len(got), got) + t.Fatalf("expected >=2 findings, got %d: %+v", len(got), got) } if got[0].Workload != "alpha" { t.Errorf("first workload = %q, want alpha", got[0].Workload)