ci: no workflow runs any test suite, on any trigger #73

New issue

Closed

opened 2026-08-28 11:56:12 +00:00 by yuris · 0 comments

yuris commented

2026-08-28 11:56:12 +00:00

Owner

Found by Reviewer67r4 while ruling on whether PR #67 should ship a regression guard. It was the decisive argument, and it generalises far past that PR.

The gap

No workflow in this repository runs any test suite, on any trigger.

$ grep -rn test .forgejo/workflows/
.forgejo/workflows/build-publish.yml: runs-on: ubuntu-latest
.forgejo/workflows/deploy-store.yml:  runs-on: ubuntu-latest

Those two matches are runs-on: ubuntu-la**test**. There is no pnpm test, no tsc --noEmit, no biome check, anywhere in automation.

deploy-store.yml is: checkout → pnpm install → wait-for-catalog → build → wrangler pages deploy. build-publish.yml builds and publishes the management container. Both fire only on push: branches:[main] plus workflow_dispatch.

So:

Nothing runs on a pull request. No branch protection, 0 required approvals, no checks.
Nothing runs on merge either. The push-to-main workflows deploy; they do not test.
A green workflow run means "the container built" or "wrangler accepted the upload". It does not mean a single test passed.

The suites are real and substantial — around 91 tests in apps/store, 238 in apps/management, plus packages/catalog — and they execute nowhere except on a developer's or agent's machine.

Why this is filed as its own issue rather than mentioned in passing

It changes what a green tick means, and several PRs in recent waves came close to leaning on that.

The repo's own package.json and turbo.json define test, lint and build tasks, so the affordance exists and looks wired. The README and PR templates talk about verification. It is easy to assume, as at least two agents did, that merging runs something. The distinction matters most exactly when someone is deciding whether to trust automation instead of running commands locally.

It also killed a real piece of work. PR #67 built a 623-line regression guard for a cascade defect that has recurred four times. Two independent arguments showed the guard graded the wrong artefact — but the decisive one was simpler: it would never have run. A test that executes on no trigger cannot defend anything, and its presence in the tree tells the next author the class is covered. The guard was deleted; #72 replaces it with a rendered-output check, and #72 explicitly depends on this issue, because a browser guard added today would not run either.

Scope

Add a workflow that runs the checks on pull requests and on pushes to main:

pnpm --filter <each package> test — per-package, not the root aggregate, so a failure names the package
tsc --noEmit per app
biome check on the repository

Three specifics that are not obvious and have each cost this repo real time:

biome is not in node_modules. The only binary is the nix one on PATH. So npx biome fetches an unrelated registry package named biome (version 0.3.3), checks nothing, prints nothing, and exits 0. A CI step written with npx would produce a permanently green lint. Use pnpm exec biome or the pinned nix binary, and assert the output contains Checked N files.
biome exits 0 on lint warnings. Its exit code tracks formatting and fixability, not findings. So an exit-code-only gate passes over real warnings. If warnings should fail CI, match on Found N warnings explicitly — and note globals.css currently carries two pre-existing warnings (noImportantStyles at 506:24, noDescendingSpecificity at 764:1), so a strict gate must either fix or baseline those first.
pnpm exec tsc --noEmit from the repo root fails with Command "tsc" not found — a workspace artefact, not a type error. Run it from each app directory.

Also decide whether the deploy workflows should depend on the check workflow. Today push: main deploys unconditionally, so a merge that breaks the build or the tests still ships. Given merging is a production release for two apps, gating the deploy on a green check is probably the point of this issue rather than a follow-up — but it is a deliberate decision, because it makes a flaky check block releases.

Explicitly out of scope

Branch protection and required approvals. Related, and worth its own decision, but a check workflow is useful immediately without them.

Verification

Open a throwaway PR with a deliberately failing test and confirm the check fails and is visible on the PR. A check that cannot be made to fail is not testing anything — this repo has now caught four instruments returning a confident green for work they did not do (npx biome, biome's exit code on warnings, a pipeline masking $?, and git log --follow truncating at a rename).
Confirm a deliberately failing tsc and a deliberately failing biome each fail the job.
Confirm the passing case is green and reports which packages ran.
If the deploys are gated, confirm a failing check blocks the deploy rather than merely reporting.

Acceptance

Tests, typecheck and lint run automatically on pull requests and on pushes to main; each has been observed failing on purpose at least once; the lint step uses a binary that actually checks and asserts on output rather than exit code alone; and whether a red check blocks the production deploy is a recorded decision.

Found by `Reviewer67r4` while ruling on whether PR #67 should ship a regression guard. It was the decisive argument, and it generalises far past that PR. ## The gap **No workflow in this repository runs any test suite, on any trigger.** ``` $ grep -rn test .forgejo/workflows/ .forgejo/workflows/build-publish.yml: runs-on: ubuntu-latest .forgejo/workflows/deploy-store.yml: runs-on: ubuntu-latest ``` Those two matches are `runs-on: ubuntu-la**test**`. There is no `pnpm test`, no `tsc --noEmit`, no `biome check`, anywhere in automation. `deploy-store.yml` is: checkout → pnpm install → `wait-for-catalog` → `build` → `wrangler pages deploy`. `build-publish.yml` builds and publishes the management container. Both fire only on `push: branches:[main]` plus `workflow_dispatch`. So: - **Nothing runs on a pull request.** No branch protection, 0 required approvals, no checks. - **Nothing runs on merge either.** The push-to-main workflows *deploy*; they do not test. - A green workflow run means "the container built" or "wrangler accepted the upload". It does not mean a single test passed. The suites are real and substantial — around 91 tests in `apps/store`, 238 in `apps/management`, plus `packages/catalog` — and they **execute nowhere except on a developer's or agent's machine**. ## Why this is filed as its own issue rather than mentioned in passing It changes what a green tick means, and several PRs in recent waves came close to leaning on that. The repo's own `package.json` and `turbo.json` define `test`, `lint` and `build` tasks, so the affordance exists and looks wired. The `README` and PR templates talk about verification. It is easy to assume, as at least two agents did, that merging runs something. **The distinction matters most exactly when someone is deciding whether to trust automation instead of running commands locally.** It also killed a real piece of work. PR #67 built a 623-line regression guard for a cascade defect that has recurred four times. Two independent arguments showed the guard graded the wrong artefact — but the decisive one was simpler: **it would never have run.** A test that executes on no trigger cannot defend anything, and its presence in the tree tells the next author the class is covered. The guard was deleted; **#72** replaces it with a rendered-output check, and #72 explicitly depends on this issue, because a browser guard added today would not run either. ## Scope Add a workflow that runs the checks on pull requests and on pushes to `main`: - `pnpm --filter <each package> test` — per-package, not the root aggregate, so a failure names the package - `tsc --noEmit` per app - `biome check` on the repository Three specifics that are not obvious and have each cost this repo real time: 1. **`biome` is not in `node_modules`.** The only binary is the nix one on `PATH`. So `npx biome` fetches an **unrelated registry package** named `biome` (version 0.3.3), checks nothing, prints nothing, and **exits 0**. A CI step written with `npx` would produce a permanently green lint. Use `pnpm exec biome` or the pinned nix binary, and assert the output contains `Checked N files`. 2. **`biome` exits 0 on lint warnings.** Its exit code tracks formatting and fixability, not findings. So an exit-code-only gate passes over real warnings. If warnings should fail CI, match on `Found N warnings` explicitly — and note `globals.css` currently carries **two pre-existing warnings** (`noImportantStyles` at `506:24`, `noDescendingSpecificity` at `764:1`), so a strict gate must either fix or baseline those first. 3. **`pnpm exec tsc --noEmit` from the repo root fails** with `Command "tsc" not found` — a workspace artefact, not a type error. Run it from each app directory. Also decide whether the deploy workflows should **depend** on the check workflow. Today `push: main` deploys unconditionally, so a merge that breaks the build or the tests still ships. Given merging is a production release for two apps, gating the deploy on a green check is probably the point of this issue rather than a follow-up — but it is a deliberate decision, because it makes a flaky check block releases. ## Explicitly out of scope Branch protection and required approvals. Related, and worth its own decision, but a check workflow is useful immediately without them. ## Verification - Open a throwaway PR with a deliberately failing test and confirm the check **fails** and is visible on the PR. A check that cannot be made to fail is not testing anything — this repo has now caught four instruments returning a confident green for work they did not do (`npx biome`, biome's exit code on warnings, a pipeline masking `$?`, and `git log --follow` truncating at a rename). - Confirm a deliberately failing `tsc` and a deliberately failing `biome` each fail the job. - Confirm the passing case is green and reports which packages ran. - If the deploys are gated, confirm a failing check blocks the deploy rather than merely reporting. ## Acceptance Tests, typecheck and lint run automatically on pull requests and on pushes to `main`; each has been observed failing on purpose at least once; the lint step uses a binary that actually checks and asserts on output rather than exit code alone; and whether a red check blocks the production deploy is a recorded decision.

yuris referenced this issue

2026-08-28 11:56:56 +00:00

fix(store): card prices lose --ink to .product-card p; pin the cart-line grid (#57, #58) #67

yuris referenced this issue

2026-08-28 11:57:35 +00:00

fix(store): card prices lose --ink to .product-card p; pin the cart-line grid (#57, #58) #67

yuris referenced this issue from a commit

2026-08-28 11:57:38 +00:00

fix(store): rank the card price above the .product-card paragraph cascade (#67)

yuris referenced this issue from a commit

2026-08-29 06:24:11 +00:00

ci: run tests, typecheck and lint on pull requests and pushes to main