Fix the four outstanding CI-investigation defects, and port reviewBaselines

[chschan]

Stacked on #65 (which is stacked on #64). Retarget down the chain as each merges.

Closes out the seven defects catalogued in rhtmlCombinedScatter CI will stop working from 30 Sep 2024.

Where the seven stood

#	Defect	Before this PR
1	clean deletes tracked man/; makeDocs swallows its failure	❌ open → fixed here
2	Windows path corruption in compileRenderContentPage	✅ already fixed on master
3	testSnapshots swallows the matcher throw → failing tests report PASS	❌ open → fixed here
4	--env whitelisted to local/travis	❌ open → fixed here
5	acceptNewSnapshots defaults true → new snapshots never fail	❌ open → fixed here
6	page.waitFor blocks puppeteer upgrades	✅ already fixed on master
7	The jest ceiling	✅ already fixed on master (#64's ancestors)

2, 6 and 7 were fixed independently, not from the investigation's branch — 2 and 6 came in with the esbuild work, 7 with the jest 29 / jest-image-snapshot 6 / puppeteer 24 upgrade.

The four fixes

Defect 3 — the important one. The catch around toMatchImageSnapshot swallowed the failure entirely, so a test whose images did not match reported PASS; the job only went red via jest's aggregate snapshotState.unmatched count, and the per-test list said everything passed. Failures are now collected through the loop and rethrown once at the end — collected rather than thrown immediately, because throwing inside the loop would abort it and lose the new_snapshots diagnostic image for every widget after the first. The diagnostic write also becomes synchronous; the old fs.writeFile callback form was fire-and-forget, so the process could exit before the image hit disk.

This one matters most right now: it's what makes the upcoming CombinedScatter baseline review trustworthy.

Defect 5. acceptNewSnapshots now defaults false. At true it appended --ci=0, making jest compute updateSnapshot: 'new', so a missing baseline was written and passed — a newly added test could look green forever while testing nothing.

Defect 4. The --env whitelist allowed only local and travis, so a CI environment couldn't be named after the system running it. CombinedScatter had to bypass the flag entirely via snapshotTesting.env in its own config. Whitelist dropped.

Defect 1. clean deleted man/, which holds tracked roxygen output that only makeDocs regenerates — and makeDocs swallows its own failure so a missing R install isn't fatal. So build silently deleted tracked R documentation on every machine without R on PATH, including all CI runners. man/ is out of the list. makeDocs now runs Rscript -e instead of r --no-save with a bash herestring and POSIX redirects, none of which cmd.exe can parse — so it could never have succeeded on Windows even with R installed.

reviewBaselines ported

307 lines that existed only on the unmerged branch. Builds a local side-by-side review page, because GitHub's diff renderer gives up on a few hundred binary files — exactly the size of a regenerated baseline set.

It needed no conversion for the gulp removal (it never took a gulp argument); only its usage strings, and dropping its -t alias for --to, since yargs is a shared singleton across task modules and -t already means testNamePattern in two other tasks.

Exercised end to end against a git fixture — 1 changed, 1 added, 1 removed, 18 identical, all four categories rendered — plus both guard paths (missing --from, and --from/--to resolving to the same sha).

cc-ci-fixes is not merged here, and should not be

It branched before 8.0.0. Merging it would revert esbuild, the puppeteer/jest upgrade, jest.config.js and every .jest.test.js suite, and restore the old mocha processTestPlans.test.js — its diff against master removes 12,341 lines. These fixes were re-applied onto current master instead. cc-ci-fixes can now be deleted.

Two bugs found while doing this

A real bug in the CLI I added in #65. Task names were taken as every argument not starting with -, so a space-separated flag value was read as a task name. rhtml reviewBaselines --from HEAD failed with unknown task 'HEAD' — and rhtml testVisual -t someFilter would have failed identically, which is the form CombinedScatter's CI uses. Task names are now the leading positional arguments only, stopping at the first flag (what gulp did). Extracted to src/lib/parseTaskNames.js with 9 tests, since it shipped silently once already.

A fidelity gap in the eslint config from #64. @stylistic's customize() defaults operator-linebreak to 'before', but eslint-config-standard used 'after'. Nothing on master happened to wrap an operator, so the mismatch was invisible until a file written under the old config was added — which then reported 13 errors for previously-correct style. Set to standard's value.

Behaviour changes for widget repos

No edit needed, but results change, so sequence a bump deliberately — documented in the README's upgrade section:

• A mismatching snapshot now fails its own test. Expect previously-green runs to surface real per-test failures.
• A snapshot with no baseline now fails. Pass --acceptNewSnapshots to opt back in when bootstrapping.
• build no longer deletes man/.

After this lands, CombinedScatter's CI can drop its --acceptNewSnapshots=false workaround and set --env=ci directly instead of via widget.config.js.

Verification

check	result
eslint .	clean, exit 0
npm test	84/84 pass, 9 suites (15 new)
node ./bin/prepush	exit 0
fixture byte-diff vs the gulp pipeline	still empty — none of this changes build output
defect-3 tests against the OLD behaviour	4 of 6 fail — confirmed the tests actually catch the regression rather than merely passing against the new code
reviewBaselines end to end	4 statuses classified correctly, both guards fire

🤖 Generated with Claude Code