Slice B: run the test suite on every pull request - #207
Merged
Conversation
Adds .github/workflows/ci.yml with two jobs, split by cost. The fast job installs, builds, and runs the Node-only tier. It sets PUPPETEER_SKIP_DOWNLOAD because it never launches a browser and Chrome is ~150MB of the install. It runs on a Node 22/24 matrix: 22 because that is what the new `engines` field claims, so the claim is verified rather than asserted, and 24 because it is closest to what development runs on. The browser job runs the full tier on Node 24 only — doubling the Chrome download to cover a second Node version is not worth it. Puppeteer's Chrome is cached on the lockfile hash, so only a Puppeteer bump pays the download again. `npm run build` is the type check. It is the same compile the published package depends on, so a separate lint step would add time without adding coverage. No secrets are required: the tests that would use an LLM API key skip when one is absent (verified in slice A), so this works on pull requests from forks. Also adds a CI badge to the README. Branch protection is deliberately not part of this change — the checks should earn trust before they can block a merge.
The first CI run failed one test: the auth scaffold's submit button clipped its label under the long-text perturbation, on Linux only. It was the one button in the library not built from the shared `button()` helper. Hand-rolled, it missed the minWidth: 0, overflow: hidden and textOverflow: ellipsis that every other button gets — the hardening Phase 29 slice C added precisely so hostile labels truncate by design instead of escaping their box. It passed the Phase 29 scaffold sweep because macOS font metrics left just enough room. Linux fonts are wider and it overflowed. Building it from `button()` keeps every property it had (name, full width, accent fill, the same padding and radius) and adds the three it was missing. Verified locally: npx tsx test-scaffold-system.ts — all structures pass, including page/auth stress CLEAN. CI verifies the Linux side.
Second Linux-only stress failure the pipeline caught. Same shape as the auth button: the identity cell in the data-table row has carried minWidth: 0 and an ellipsis label since Phase 29, and the three columns beside it never got it. Rather than fix the one column CI named, I audited the library for the pattern — text inside a horizontal frame with a fixed or percentage width, where it cannot wrap out of trouble and has no designed truncation. Exactly nine hits, all of them these three cells across the three sample rows. Every one is now hardened and the audit comes back empty. Note the audit was deliberately narrow. A broader sweep (any text under any width-constrained ancestor) returns 113 hits, but almost all are headlines and body copy that wrap correctly; adding ellipsis there would truncate prose that is currently fine. Verified locally: test-scaffold-system all pass, test-component-structures 28/28.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second and final slice from
docs/specs/CI-SPEC.md. Slice A gave the suite a runner and got it green; this wires it to GitHub Actions.The workflow
Two jobs, split by cost rather than by kind.
The fast job installs, builds, and runs the Node-only tier — 67 tests, about two minutes locally. It sets
PUPPETEER_SKIP_DOWNLOADbecause it never launches a browser and Chrome is roughly 150MB of an otherwise quick install. It runs on a Node 22 and 24 matrix: 22 because that is the lower bound the newenginesfield claims, so the claim is verified by the pipeline instead of asserted in a file nobody checks, and 24 because it is nearest to what development actually runs on.The browser job runs the full tier on Node 24 only. Doubling a Chrome download to cover a second Node version buys very little. Puppeteer's browser is cached against the lockfile hash, so only a Puppeteer bump pays for the download again.
npm run buildserves as the type check. It is the same compile the published package already depends on, so adding a separate lint step would cost time without covering anything new.Two things worth reviewing
No secrets are required. Slice A verified that all six tests which reach the network or want an LLM API key skip cleanly when no key is present, and that a cold
FRAMESMITH_HOMEdoes not break the Node-only font tests. That is exactly the state a fresh runner is in, which is why this works on pull requests from forks as well as your own branches.The Node 22 leg is the one genuine unknown. There is only one Node installed on the development machine, so there was no way to test the lower bound locally — the matrix leg on this pull request is the first real check of it. If 22 fails, the honest fix is to raise
enginesto match what actually works rather than to quietly drop the matrix leg.Not included, on purpose
Branch protection. The checks should run green for a while before they are allowed to block a merge, and turning it on is a one-click change once you trust them. Also still out of scope, as the spec records: a test framework, release automation, and coverage measurement.
After this merges
The badge in the README starts resolving, and
mastergains its first automated verification. Worth watching the first few runs for flakiness in the browser job specifically — Chrome on a CI runner is the part most likely to need a tweak, though--no-sandboxis already passed at launch, which is the usual culprit.What the first runs found
The pipeline paid for itself before it merged. Both Node legs passed on the first run, so
engines: ">=22"is verified rather than asserted. The browser job failed twice — not on environment, but on two real defects that this machine structurally cannot reproduce.Both are the same shape: a scaffold element missing the
minWidth: 0and ellipsis hardening that Phase 29 slice C added everywhere else, so a hostile label escapes its box instead of truncating by design. Both passed that sweep because macOS font metrics are narrower than Linux's and the overflow never quite happened here.The first was the
authsubmit button — the one button in the library not built from the sharedbutton()helper. Rebuilding it from the helper keeps every property it had and adds the three it lacked.The second was the
data-tablerole, status and amount cells. Rather than fix the single column the failure named, I audited the library for the pattern: text inside a fixed-width horizontal cell where it cannot wrap and has no designed truncation. Exactly nine hits, all in that one row builder, all now fixed, and the audit comes back empty. The audit was kept deliberately narrow — a broader version returns 113 hits, but those are headlines and body copy that wrap correctly, and truncating them would make the library worse.Final run: all three jobs green. The Node-only jobs finish in about 75 seconds, the browser job in about three minutes.