test(processor): multi-line golden fixtures; route .sass through SCSS lexer
This commit is contained in:
@@ -0,0 +1,64 @@
|
||||
# Task 3 & 4 Report — Rewire mutation and preview paths through StripComments
|
||||
|
||||
## Files changed
|
||||
|
||||
- `internal/processor/processor.go`
|
||||
- `processFileInMemory`: rewritten to read the whole file with `os.ReadFile`, call `StripComments`, and write back with `os.WriteFile` only when `changed(changes)` is true. Signature unchanged (`language` param kept but now unused, which is legal for a Go function parameter).
|
||||
- `showPreview`: rewritten to read the whole file, call `StripComments` (no `InRange`), map results via the new `changesToInfos` helper, and render exactly as before.
|
||||
- Removed unused `"bufio"` import (both scanner-based read loops were replaced by `os.ReadFile`). Kept `"io"` (still used by `createBackup`'s `io.Copy`).
|
||||
|
||||
- `internal/processor/git_processor.go`
|
||||
- `processFileWithLineRanges`: rewritten to read the whole file, build an `InRange` predicate from `git.IsInLineRanges` when `lineRanges` is non-empty, call `StripComments`, write back conditionally.
|
||||
- `showGitPreviewWithTotals`: rewritten to read the whole file, build the same `InRange` predicate, call `StripComments`, map via `changesToInfos`, render with the existing blue "Processing" header, accumulate into `totals`.
|
||||
- Removed unused `"bufio"` import.
|
||||
- Kept `removeCommentsFromLine`, `findCommentIndex`, `lineHasComment` — all three are still referenced (fallback path in `legacyStrip`, plus `TestRemoveCommentsFromLine`, `TestFindCommentIndex`, `TestLineHasComment`).
|
||||
|
||||
- `internal/processor/engine.go`
|
||||
- Added `changesToInfos(changes []LineChange) ([]changeInfo, int, int, int)` per the plan (Task 4 Step 1), consumed by both preview functions.
|
||||
- **Bug fix (not in the plan's literal code) required to keep the regression gate green:** in `StripComments`, the `endLine` computed for a token's `InRange` straddle check was `startLine + strings.Count(t.Value, "\n")`. Chroma's single-line-comment tokens include the trailing `\n` in `t.Value` (verified directly against the JS lexer: `CommentSingle "// one\n"`), so a same-line `//` comment was computed as spanning two lines, which made the git-range straddle check incorrectly keep it. Fixed by excluding the token's own trailing newline from the span count:
|
||||
```go
|
||||
endLine := startLine + strings.Count(strings.TrimSuffix(t.Value, "\n"), "\n")
|
||||
```
|
||||
This is scoped entirely inside `StripComments`'s per-token loop; no other behavior changed. Confirmed the existing `TestStripComments` straddle/range-block subtests (which use multi-line `/* */` tokens that don't end in `\n`) are unaffected, and the fix is exactly what turned the `TestWholeFile/git_range_limits_stripping_to_changed_lines` regression from FAIL to PASS.
|
||||
|
||||
## Imports removed
|
||||
|
||||
- `internal/processor/processor.go`: `"bufio"`
|
||||
- `internal/processor/git_processor.go`: `"bufio"`
|
||||
|
||||
No other imports were flagged by `go build ./...`.
|
||||
|
||||
## Test output
|
||||
|
||||
`go test ./...` (final, after both commits):
|
||||
```
|
||||
ok github.com/carlosarraes/shush/cmd/shush
|
||||
ok github.com/carlosarraes/shush/internal/git
|
||||
ok github.com/carlosarraes/shush/internal/processor
|
||||
? github.com/carlosarraes/shush/internal/cli [no test files]
|
||||
? github.com/carlosarraes/shush/internal/commands [no test files]
|
||||
? github.com/carlosarraes/shush/internal/config [no test files]
|
||||
? github.com/carlosarraes/shush/internal/guide [no test files]
|
||||
? github.com/carlosarraes/shush/internal/hooks [no test files]
|
||||
? github.com/carlosarraes/shush/internal/ignore [no test files]
|
||||
? github.com/carlosarraes/shush/internal/types [no test files]
|
||||
```
|
||||
|
||||
`TestWholeFile` (regression guard) — all 17 subtests PASS, including `git_range_limits_stripping_to_changed_lines` (the one that initially failed and drove the fix above).
|
||||
|
||||
`TestStripComments`, `TestResolveLexer`, `TestShouldDrop` — all PASS (unaffected by the endLine fix except in the way intended).
|
||||
|
||||
## make check
|
||||
|
||||
Ran twice (once per task, gate before each commit): `go fmt ./...` clean, `go vet ./...` clean, `go test -v ./...` all PASS both times. Exit code 0 both times.
|
||||
|
||||
## Commits (jj, in order)
|
||||
|
||||
1. `feat(processor): route mutation paths through StripComments` — commit `68a366c7` (`processFileInMemory`, `processFileWithLineRanges` rewired; includes the `endLine` fix in `engine.go` since it was required to pass the regression gate at this step).
|
||||
2. `feat(processor): route preview paths through StripComments` — commit `99bc7c3d` (`showPreview`, `showGitPreviewWithTotals` rewired; `changesToInfos` added).
|
||||
|
||||
Working copy is now a fresh empty commit (`dcf0169d`) on top, per the jj-per-task workflow (`jj new` after each `jj describe`).
|
||||
|
||||
## No-comments check
|
||||
|
||||
Ran `jj diff | grep '^+' | grep -E '//|/\*|\*/'` after both edits — no matches. No Go comments were introduced.
|
||||
@@ -0,0 +1,146 @@
|
||||
# Task 5 Report — Multi-line golden fixtures + driver
|
||||
|
||||
## Status: BLOCKED (1 of 39 subtests fails; `make check` not clean; not committed)
|
||||
|
||||
## Files created
|
||||
|
||||
- `internal/processor/multiline_test.go` — driver, exactly per plan (`multilineCase`, `runMultilineCase`, `TestMultiline`), no code comments.
|
||||
- `internal/processor/testdata/multiline/` — 78 fixture files (39 inputs + 39 goldens):
|
||||
- Block fixtures, one pair per extension in `blockExts` (js, ts, jsx, tsx, go, c, cpp, cc, cxx, h, hpp, java, cs, rs, swift, kt, kts, dart, scala, php, css, scss, sass, less, sql) and `xmlExts` (html, htm, xml, svg).
|
||||
- `php` uses the `<?php` wrapper variant per plan.
|
||||
- Special-case fixtures: `preproc.c`, `rawstring.go`, `template.js`, `lifetime.rs`, `mixed.c`, `marker_in_line.c`, `unterminated.c`, `fallback.conf`, `straddle.js`, `range_inside.js`.
|
||||
|
||||
All fixture bytes were typed exactly as specified in the plan (via the Write tool), except one golden correction described below.
|
||||
|
||||
## Test run: `go test ./internal/processor -run TestMultiline -v`
|
||||
|
||||
```
|
||||
=== RUN TestMultiline
|
||||
--- FAIL: TestMultiline (0.15s)
|
||||
--- PASS: TestMultiline/block_js (0.00s)
|
||||
--- PASS: TestMultiline/block_ts (0.00s)
|
||||
--- PASS: TestMultiline/block_jsx (0.00s)
|
||||
--- PASS: TestMultiline/block_tsx (0.00s)
|
||||
--- PASS: TestMultiline/block_go (0.00s)
|
||||
--- PASS: TestMultiline/block_c (0.00s)
|
||||
--- PASS: TestMultiline/block_cpp (0.00s)
|
||||
--- PASS: TestMultiline/block_cc (0.00s)
|
||||
--- PASS: TestMultiline/block_cxx (0.00s)
|
||||
--- PASS: TestMultiline/block_h (0.00s)
|
||||
--- PASS: TestMultiline/block_hpp (0.00s)
|
||||
--- PASS: TestMultiline/block_java (0.00s)
|
||||
--- PASS: TestMultiline/block_cs (0.00s)
|
||||
--- PASS: TestMultiline/block_rs (0.00s)
|
||||
--- PASS: TestMultiline/block_swift (0.00s)
|
||||
--- PASS: TestMultiline/block_kt (0.00s)
|
||||
--- PASS: TestMultiline/block_kts (0.00s)
|
||||
--- PASS: TestMultiline/block_dart (0.00s)
|
||||
--- PASS: TestMultiline/block_scala (0.01s)
|
||||
--- PASS: TestMultiline/block_php (0.00s)
|
||||
--- PASS: TestMultiline/block_css (0.01s)
|
||||
--- PASS: TestMultiline/block_scss (0.02s)
|
||||
--- FAIL: TestMultiline/block_sass (0.01s)
|
||||
--- PASS: TestMultiline/block_less (0.00s)
|
||||
--- PASS: TestMultiline/block_sql (0.01s)
|
||||
--- PASS: TestMultiline/block_html (0.00s)
|
||||
--- PASS: TestMultiline/block_htm (0.00s)
|
||||
--- PASS: TestMultiline/block_xml (0.00s)
|
||||
--- PASS: TestMultiline/block_svg (0.00s)
|
||||
--- PASS: TestMultiline/preproc_kept (0.00s)
|
||||
--- PASS: TestMultiline/raw_string (0.00s)
|
||||
--- PASS: TestMultiline/template_literal (0.00s)
|
||||
--- PASS: TestMultiline/rust_lifetime (0.00s)
|
||||
--- PASS: TestMultiline/mixed_line_and_block (0.00s)
|
||||
--- PASS: TestMultiline/marker_in_line_comment (0.00s)
|
||||
--- PASS: TestMultiline/unterminated (0.00s)
|
||||
--- PASS: TestMultiline/conf_fallback (0.00s)
|
||||
--- PASS: TestMultiline/git_straddle_kept (0.00s)
|
||||
--- PASS: TestMultiline/git_range_removes_inside_block (0.00s)
|
||||
FAIL
|
||||
FAIL github.com/carlosarraes/shush/internal/processor 0.156s
|
||||
FAIL
|
||||
```
|
||||
|
||||
38/39 subtests pass. Failure detail for the one holdout:
|
||||
|
||||
```
|
||||
=== RUN TestMultiline/block_sass
|
||||
multiline_test.go:98: block sass mismatch:
|
||||
--- got ---
|
||||
"code_before();\n/* block line one\n block line two */\ncode_after();\n"
|
||||
--- want ---
|
||||
"code_before();\ncode_after();\n"
|
||||
```
|
||||
|
||||
Regression guard, unaffected by this work:
|
||||
|
||||
```
|
||||
$ go test ./internal/processor -run 'TestWholeFile|TestStripComments|TestResolveLexer|TestShouldDrop' -v
|
||||
--- PASS: TestStripComments (0.03s) (13/13 subtests)
|
||||
--- PASS: TestWholeFile (0.03s) (17/17 subtests)
|
||||
ok github.com/carlosarraes/shush/internal/processor 0.082s
|
||||
```
|
||||
|
||||
`go test ./...`: everything green except the one `TestMultiline/block_sass` subtest above.
|
||||
|
||||
## Language requiring special handling: `php` (per plan, applied as documented)
|
||||
|
||||
Confirmed chroma's PHP lexer only tokenizes `/* */` as `CommentMultiline` inside a `<?php ... ?>` region. Used the plan's documented `<?php` prefix wrapper on both `block_php.php` and its golden. This matches the plan's pre-approved remedy exactly — no deviation.
|
||||
|
||||
## Language that FAILED and was NOT force-fixed: `sass`
|
||||
|
||||
**Extension:** `sass` (chroma lexer name `Sass`, the indented syntax, aliased from `.sass`).
|
||||
|
||||
**Symptom:** `StripComments` leaves the `/* block line one\n block line two */` block entirely untouched — the file round-trips unchanged.
|
||||
|
||||
**Root cause (verified empirically by tokenizing with the actual chroma lexer, not guessed):**
|
||||
|
||||
1. In chroma's `Sass` lexer grammar (`lexers/embedded/sass.xml`), the `root` and `selector` states — which is what governs top-level statements — have **no rule at all** for `/*` or `//`. A `/*` at top level falls through to the generic operator-character rule and is tokenized as plain `Operator`/`NameTag` text, never as `CommentMultiline`. Verified: tokenizing `code_before();\n/* block line one\n block line two */\ncode_after();\n` through `lexers.Match("f.sass")` produces zero `Comment*` tokens.
|
||||
2. Comment recognition only exists inside the `value` state (i.e., after a `key:` declaration). Even there, I verified the `inline-comment` sub-state used for `/* ... */` does **not** span across a newline — tokenizing `color: red /* block line one\n block line two */ blue\n` still only marks `/* block line one` as `CommentMultiline`; the following line's `block`, `line`, `two`, `*/` come back as separate `NameTag`/`Operator` tokens, not comment content.
|
||||
|
||||
**Conclusion:** this is not a "wrong context" problem fixable by a minimal wrapper (PHP's class of fix). PHP just needed to be inside its language-delimiter tag; once inside, its multiline comment token behaves normally across newlines. Sass's indented-syntax grammar in chroma has no code path in which `/* */` spans multiple lines at all — there is no fixture shape that would make the plan's "block spans two lines" test meaningful for `.sass`, so I did not force one. This is a chroma lexer limitation, not a shush bug and not a fixture-authoring mistake.
|
||||
|
||||
**Options for the plan owner (not applied, needs a decision):**
|
||||
- (a) Accept the limitation, drop `sass` from `blockExts` in `TestMultiline` (or replace `block_sass.sass` with a fixture documenting "no multi-line block support" instead of testing it), and file this as a known gap.
|
||||
- (b) Route `.sass` through the `SCSS` lexer in `resolveLexer` the same way `.less` already is (`engine.go:30-34`). This is architecturally the same pattern already in the codebase, and a quick manual check shows SCSS's lexer does support multiline `/* */`. However this is a `resolveLexer` code change with broader blast radius than a test fixture — real `.sass` (indented) files pushed through the brace/semicolon-oriented SCSS grammar would very likely fall back to `legacyStrip` on the losslessness check for anything beyond trivial content, so it needs deliberate evaluation, not a drive-by fix inside a test task. I did not make this change; flagging it as the most promising remedy if `sass` multi-line support is required.
|
||||
|
||||
I left `block_sass.sass` / `block_sass.sass.golden` as the plan's uniform template (unchanged, still failing) rather than editing the golden to match the no-op output — that would hide the gap instead of reporting it.
|
||||
|
||||
## Golden correction I did make (with evidence, not a guess): `template.js.golden`
|
||||
|
||||
The plan's authored golden expected `template.js`'s last two lines to merge onto one output line:
|
||||
```
|
||||
const s = `line1 /* nope
|
||||
line2 */ still`; const x = 1;
|
||||
```
|
||||
|
||||
Actual (and, I determined, *correct*) engine output keeps them as two separate lines:
|
||||
```
|
||||
const s = `line1 /* nope
|
||||
line2 */ still`;
|
||||
const x = 1;
|
||||
```
|
||||
|
||||
**Why I concluded the plan's golden was a typo, not a signal of a real bug**, before touching it:
|
||||
|
||||
- JS's `CommentSingle` grammar rule is `//.*?\n` — the token literally swallows the line's trailing `\n` as part of its matched value (confirmed by reading `lexers/embedded/javascript.xml:59`).
|
||||
- If that were reason enough to merge lines in the output, then the *already-committed, already-passing* `TestStripComments` case `"marker inside line comment inert"` in `engine_test.go` would have to fail too — it exercises the exact same shape in C (`z(); // a /* b\nw();\n"` → `"z();\nw();\n"`), and C's `CommentSingle` rule (`//.*?\n`, `lexers/embedded/c.xml:86`) also swallows the trailing `\n`. That test asserts the two lines stay **separate**, and it passes.
|
||||
- `engine.go`'s `flush()` synthesizes its own `\n` per source line unconditionally (it does not copy the literal `\n` byte from any token); a `\n` embedded inside a *dropped* token still triggers a `flush()` call via the `segs := strings.Split(t.Value, "\n")` loop, so every physical source line boundary is preserved in the output regardless of whether the newline character was inside a comment token or not. This is the tested, approved, already-shipped Task 2 behavior, not something I invented for this task.
|
||||
|
||||
Given the already-approved regression test for the identical token shape proves no-merge is the correct, intended behavior, I fixed the one-line typo in `template.js.golden` rather than leave a known-good implementation "failing" against a golden that contradicts its own author's other tests. I did not touch `engine.go`.
|
||||
|
||||
## `make check`
|
||||
|
||||
**Not clean** — fails at the `test` stage on `TestMultiline/block_sass` (see above). Per the plan's Task 5 gate ("Gate: make check clean before commit") and the hard rule "Do NOT edit a golden to match wrong output just to go green," I stopped here rather than force a pass.
|
||||
|
||||
## Commit
|
||||
|
||||
**Not created.** Per the hard rule "jj, never git" and "Gate: make check clean before commit," no `jj describe` / `jj new` was run because `make check` is not clean. All fixture and driver files exist in the working copy, uncommitted, ready to commit once the `sass` question above is resolved (either accept-and-adjust-scope or implement the SCSS-lexer-routing fix and re-verify).
|
||||
|
||||
## Summary
|
||||
|
||||
- Driver: `internal/processor/multiline_test.go`, comment-free, matches plan exactly.
|
||||
- Fixtures: 78 files under `internal/processor/testdata/multiline/`, all hand-typed via Write, matching the plan's exact bytes except the one corrected golden (`template.js.golden`, justified above).
|
||||
- 38/39 `TestMultiline` subtests pass. `TestWholeFile` (17/17) and `TestStripComments` (13/13) regression suites stay fully green.
|
||||
- One language, `sass`, fails for a documented, verified reason (chroma lexer limitation, not a shush bug) and was deliberately left failing/reported rather than faked green.
|
||||
- `php` required the plan's pre-approved `<?php` wrapper remedy; applied as specified, no deviation.
|
||||
Reference in New Issue
Block a user