Skip to content

test(cli): cover formatLintFindings and normalizeErrorMessage#1208

Merged
vanceingalls merged 1 commit into
heygen-com:mainfrom
calcarazgre646:test/cli-format-helpers
Jun 17, 2026
Merged

test(cli): cover formatLintFindings and normalizeErrorMessage#1208
vanceingalls merged 1 commit into
heygen-com:mainfrom
calcarazgre646:test/cli-format-helpers

Conversation

@calcarazgre646

Copy link
Copy Markdown
Contributor

What

Test coverage for two pure cli helpers that had none:

  • utils/lintFormat.ts (formatLintFindings): the shared console formatter behind lint, render, preview, and publish. 12 tests covering the file-label threshold (single vs multiple files), showElementId, info suppression by default and inclusion under verbose, input-order default vs errorsFirst grouping (info last), the indented Fix: hint line, and the showSummary footer including the verbose-only info count.
  • utils/errorMessage.ts (normalizeErrorMessage): the fallback cascade used by render error paths. 10 tests covering Error instances, strings, objects with and without a string message, the JSON fallback, the key-list fallback for cyclic objects, the String() last resort for an object that throws from both JSON.stringify and Object.keys (Proxy with a throwing ownKeys trap), and the null/undefined/primitive paths.

No source changes.

Notes

  • Fixtures build the full ProjectLintResult shape (per-file counts plus totals) rather than only the fields the formatter happens to read, so they stay valid if the formatter starts consuming more of the result.
  • Output assertions target plain substrings. ui/colors.ts disables ANSI when stdout is not a TTY, which holds for vitest workers locally and in CI.
  • Each assertion group was checked against a mutated implementation before committing (disabling errorsFirst grouping and removing the message-property preference both produce failures).

Suite: 710/710 cli.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: clear — ready for stamp

A clean, source-touch-free test contribution; both new files pin pure helpers that previously had no coverage, and the assertions match the implementations at 3279ea7f line for line. The PR body's promise — "each assertion group was checked against a mutated implementation before committing" — is the very discipline that protects this kind of work from the tests-pass-by-coincidence failure mode, and I have no quarrel with the result.

Verification of errorMessage.test.ts against errorMessage.ts

Walking the cascade in source order and matching each branch to a test:

  • error instanceof Errorerror.message. Tests cover both Error and TypeError (subclass) — symmetric and sufficient.
  • typeof error === "string" → pass-through. Covered.
  • typeof error === "object" && error !== null with typeof msg === "string" → returned. Covered by the "prefers a string message property" test.
  • Same branch but msg = 42 (non-string) → falls through to JSON.stringify(error), which serializes the whole object including the message key. The test pins exactly '{"message":42}' — the actual behaviour, not an idealised one. Good fidelity.
  • JSON.stringify throws on a cycle → Object.keys(error).join(", ") joined into "{code, self}". The cyclic fixture (cyclic.self = cyclic) is the canonical shape, and Object.keys returns insertion order, so the assertion is stable.
  • Both fallbacks fail → String(error ?? "unknown error"). The hostile-Proxy fixture is the only way to reach this branch, and new Proxy({}, { ownKeys: () => { throw } }) causes JSON.stringify's internal ownKeys invocation to throw and Object.keys(error) to throw on the second attempt — both catches fire, and String(<proxy of {}>) is "[object Object]". Genuinely covers the truly opaque object comment in the source.
  • null / undefinedString(null ?? "unknown error") = "unknown error". Both ends of the nullish-coalescing covered.
  • Non-object primitives → String(false ?? "unknown error") = "false" (nullish coalescing only catches null/undefined, not falsy false). The test's choice of 42 and false deliberately exercises the truthy primitive and falsy-but-not-nullish axes — that's the right two values to pick for one paired assertion.

Every branch reachable; no dead arms; no over-mocking.

Verification of lintFormat.test.ts against lintFormat.ts

The single-file-vs-multi-file pivot (multiFile = results.length > 1) is exercised on both sides — "no file label for a single file" and "labels each finding with its file when more than one file" — and the assertions correctly invert: the first asserts not.toContain("index.html"), the second asserts both labels appear. That asymmetric pairing is what proves the conditional, not just its true branch.

The errorsFirst ordering trio is the most interesting bit: input order on [warning, error], then errors-first on the same input, then errors-first-with-info-last on [info, warning, error]. The three together pin the three loop passes in the source. The order of the three passes (error → warning → info) is what the source does only when verbose && errorsFirst; the test's last assertion (info message at position 2) is the only one that proves the info-last convention is intentional rather than accidental. Worth keeping.

fixHint test pins both appended immediately after its parent finding and indentation prefix on the hint line. Both are pinned by expect(lines).toHaveLength(3) paired with positional toContain, which would break under either rewrite (re-ordering or re-indenting). Defensible.

Summary test asserts lines.at(-2) === "" and lines.at(-1) contains the count string. The source pushes the blank line unconditionally under showSummary, so the at(-2) === "" is load-bearing — if a future author conditionalises the blank line, this fails loudly. Good.

verbose && totalInfos > 0 in the summary is the source's gate. The test pairs quiet (asserts not.toContain("info(s)")) with verbose (asserts "1 error(s), 0 warning(s), 1 info(s)"). Both sides — never a dead branch.

The substring-match decision is correct

ui/colors.ts (isColorSupported = process.stdout.isTTY === true && ...) — in vitest workers process.stdout.isTTY is undefined, so every c.* wrap collapses to identity and the substring assertions ("✗", "missing-timeline", "[main]") land verbatim. The PR body cites this explicitly; I verified it at HEAD. If colors.ts ever flips that condition to force colour even in CI, the test failures would name the cause precisely ([main] vs [\x1b[38;2;…m[main]\x1b[39m]) rather than going green-with-wrong-coverage. Defensible posture.

Band-aid-bar audit

Nothing trips. The PR is source-untouched; there is no duplicate validation at boundaries to drift, no contradictory rule, no silent scope gap (the helpers are pure functions, the tests cover their public surface exhaustively), no defensive code swallowing its own throw, no telemetry-without-user-signal hazard, no extracted-helper inlined back, no rules-of-hooks violation. The closest thing to a band-aid concern would be brittle assertions that pin implementation detail — but I cannot find any. The hostile-Proxy assertion comes nearest (it pins "[object Object]", which is Object.prototype.toString's default), yet that is the contract normalizeErrorMessage offers as its last resort, and a future change to that contract should announce itself by failing this test.

Stack note

Despite the consecutive numbers, #1206 / #1207 / #1208 are not a Graphite stack — every PR's baseRefName is main, each headRefName is an independent feature branch (fix/core-timing-nan-validation, fix/init-validate-before-mkdir, test/cli-format-helpers). Same author, different surfaces. The final-state-vs-base reading rule that applies to a real stack does not apply here; each PR can and should be reviewed in isolation. No cross-PR rebase-revert hazard for this one given it's pure test additions to two new files.

CI

Green across every required check at 3279ea7f — Format, Lint, Build, Typecheck, Test, Test: runtime contract, Studio: load smoke, CLI smoke, Smoke: global install, Fallow audit, plus all the conditional gates (Player perf, regression, preview-regression, Windows render). The 710/710 cli suite claim in the body matches the green Test job.

Verdict

Clear at 3279ea7f. No nits substantive enough to hold for, no band-aid-bar trips, no scope gaps. Hand to the stamp queue when ready; re-verify if HEAD moves before stamp.

Review by Via

@vanceingalls vanceingalls merged commit 8306b2c into heygen-com:main Jun 17, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants