Skip to content

ci: run the test suite in CI - #548

Merged
Aymericr merged 1 commit into
pascalorg:mainfrom
vjureta:ci/run-tests
Aug 4, 2026
Merged

ci: run the test suite in CI#548
Aymericr merged 1 commit into
pascalorg:mainfrom
vjureta:ci/run-tests

Conversation

@vjureta

@vjureta vjureta commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Why

The repo has 313 test files (~60k lines) and they're good tests — schema migrations, wall mitering, CSG cutouts, placement collision, NaN/Infinity sanitization. But almost none of them run on a pull request.

  • ci.yml runs only bun run check and bun run check-types.
  • mcp-ci.yml runs packages/mcp plus two apps/editor/lib files, and only on path matches.
  • turbo.json has no test task at all.
  • packages/viewer, packages/editor, packages/ifc-converter and apps/editor contain 95 test files between them and had no test script, so there was no way to run them even locally.

Net effect: roughly 2,200 of 2,491 tests were not acting as a gate on any change.

What this does

  • Adds a test task to turbo.json (dependsOn: ["^build"], no outputs) and a root test script, then one Test step to ci.yml.
  • Adds the missing test scripts to the four packages above.
  • Fixes a dependency-graph bug: packages/viewer declared @pascal-app/core only as a peerDependency. Turbo doesn't traverse peer deps, so ^build resolved to nothing and viewer's tests couldn't resolve core's dist/. editor, nodes and mcp all declare it in devDependencies and peerDependencies; this makes viewer consistent with them. (One line in bun.lock.)
  • Scopes each glob to src/tests/lib. Bare bun test at a package root also collects the compiled tests under dist/, which is why viewer appeared to have 132 tests when it has 66.
  • Documents bun test in SETUP.md and CONTRIBUTING.md (neither mentioned tests).

Verification

From a clean tree — rm -rf packages/*/dist packages/*/.turbo .turbo — on Bun 1.3.14:

Tasks:    12 successful, 12 total
Time:     1m3.611s
Package Tests
@pascal-app/core 760 pass, 0 fail (72 files)
@pascal-app/nodes 876 pass, 0 fail (112 files)
@pascal-app/editor 481 pass, 0 fail (66 files)
@pascal-app/mcp 297 pass, 0 fail (46 files)
@pascal-app/viewer 66 pass, 0 fail (14 files)
editor (app) 8 pass, 0 fail (2 files)
@pascal-app/ifc-converter 3 pass, 0 fail (1 file)

2,491 passing, 0 failing. Warm re-run is FULL TURBO (289ms), so the added CI step costs ~1 min cold and nothing when cached. bun run check (1,527 files) and bun run check-types (9/9) still pass.

Notes

  • No test files or product code were changed — this is wiring only.
  • mcp-ci.yml is left alone. It's now partly redundant with the root task, but it also builds packages/mcp and lints a specific file set, so I didn't want to fold it in without your call. Happy to do that in a follow-up.
  • Worth considering bun test --coverage or --bail later; deliberately kept out of scope.

Note

Low Risk
Infrastructure and documentation only; no runtime or test logic changes, with low blast radius beyond longer CI when the cache is cold.

Overview
Wires the monorepo test suite into CI and local workflows so thousands of existing tests actually gate PRs, without changing product or test code.

Adds a root test script and a Turborepo test task (dependsOn: ["^build"]), plus a Test step in ci.yml after typecheck. Packages that had tests but no script now expose scoped bun test commands (src / tests / lib) so runs don’t pick up compiled copies under dist/.

Fixes viewer test/build ordering: @pascal-app/core is added to packages/viewer devDependencies (alongside the existing peer) so Turbo can build core before viewer tests resolve dist/. Viewer tsconfig also excludes **/*.test.* from the library build.

Docs: SETUP.md, CONTRIBUTING.md, and the PR checklist now tell contributors to use bun run test (not bare bun test) and explain why.

Reviewed by Cursor Bugbot for commit 4f5bc81. Bugbot is set up for automated code reviews on this repo. Configure here.

The repo has 313 test files (~60k lines) but nothing ran them on a pull
request: `ci.yml` only did lint + type check, and `mcp-ci.yml` covered
only packages/mcp plus two apps/editor files. So roughly 2,200 of the
2,491 tests never executed as a gate.

- add a `test` task to turbo.json (`dependsOn: ["^build"]`) and a root
  `test` script, then a `Test` step to ci.yml
- add the missing `test` scripts to packages/viewer, packages/editor,
  packages/ifc-converter and apps/editor, which all contained tests but
  had no way to run them
- add `@pascal-app/core` to packages/viewer devDependencies. It was
  declared only as a peerDependency, which Turbo does not traverse, so
  `^build` resolved to nothing and viewer's tests could not resolve
  core's `dist/`. editor/nodes/mcp already declare it in both places.
- scope each test glob to `src`/`tests`/`lib` so compiled tests under
  `dist/` are not collected a second time
- document `bun test` in SETUP.md and CONTRIBUTING.md

Verified from a clean tree (no dist, no turbo cache): 12/12 tasks,
2,491 tests, 0 failures; warm re-run is fully cached.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Aymericr

Aymericr commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

This is the highest-leverage PR in the queue right now — 2,833 tests existed in the repo and CI ran none of them. Verified from a clean clone with your branch: bun run test → 12/12 tasks, all green.

core 932 · nodes 944 (+1 skip) · editor 575 · mcp 299 · viewer 72 · editor-app 8 · ifc-converter 3

I rebased onto main and pushed three fixes to your branch.

1. The viewer devDependency range was stale. "@pascal-app/core": "^0.9.2" — the workspace moved to 1.0.0-beta.4, so the declared range no longer matches the package it resolves to. Bumped to ^1.0.0-beta.4 (one matching line in bun.lock). The dependency itself is right to add: 7 viewer test files import @pascal-app/core, so it was previously an undeclared dependency that only worked by hoisting.

Worth recording since it could mislead a later reader: this does not break bun install --frozen-lockfile. I checked, because that would have blocked the PR outright — bun links the workspace package regardless of the range, and the frozen install succeeds either way. It's a correctness fix, not an unblock. Related: main's lockfile carries ~64 lines of the same drift from the 1.0 bump. I deliberately left that alone rather than run a full bun install here, since it isn't this PR's scope; it's worth a separate one-line pass.

2. bun test in the docs never runs the scripts this PR adds. test is one of Bun's own subcommands, so it shadows the package script. bun test from the root doesn't reach turbo run test at all — it runs Bun's own collector over every file it can find. Concretely, in packages/viewer that collected 144 tests instead of 72: every test compiled into dist/ got counted a second time. So the docs would have pointed contributors at a command that silently does the wrong thing. Changed to bun run test and bun --cwd packages/core run test, with a note explaining why.

3. Fixed the underlying cause of that double-count rather than only documenting around it. packages/viewer/tsconfig.json was missing the "**/*.test.ts" / "**/*.test.tsx" excludes that core, nodes, and mcp all have, so compiled tests were landing in dist/ — and since files: ["dist"], they were being published inside the viewer tarball. Added the excludes; dist/ is now test-free and the scoped glob is no longer load-bearing.

One thing I'd flag as a follow-up rather than fold in here: packages/mcp and packages/nodes use an unscoped bun test as their script, which works today but will collect dist/ the moment either package emits tests there. Making those bun test src too would be consistent with the other four — happy for that to be a separate PR.

Thanks for this one. It's exactly the kind of infrastructure work that's easy to skip and pays for itself immediately. Merging once CI reports.

@Aymericr

Aymericr commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Merging. This gates 2,833 tests that were never running in CI — verified green from a clean clone: 12/12 turbo tasks (core 932, nodes 944, editor 575, mcp 299, viewer 87, editor-app 8, ifc-converter 3).

I pushed three things on top of yours:

  • packages/viewer/tsconfig.json was missing the **/*.test.ts / **/*.test.tsx excludes that core, nodes and mcp all have. That was the root cause of the inflated viewer count you'd have seen (174 vs 87 — dist/ copies double-counted), and it also meant compiled tests were shipping in the published viewer tarball via files: ["dist"].
  • packages/viewer/package.json — the @pascal-app/core devDep range was still ^0.9.2 against a workspace at 1.0.0-beta.4. Seven viewer test files import core, so the range needed to be right. To be clear this was not blocking your CI step: I tested bun install --frozen-lockfile against the stale range and it succeeds (1364 packages, lockfile unmodified) because bun links the workspace package regardless. Correctness fix, not an unblock.
  • CONTRIBUTING.md / SETUP.md — documented the thing that makes this PR necessary in the first place: test is one of Bun's own subcommands, so bun test never reaches the package script. It runs Bun's collector over every file it can find, including dist/. bun run test goes through Turborepo, which builds workspace deps first (several packages import theirs from dist/) and runs each package's scoped script.

Two things I deliberately left out of scope: ~64 lines of pre-existing lockfile drift on main (separate one-line pass), and the unscoped test scripts in mcp/nodes. Thanks for this one — CI that doesn't run the tests is the kind of gap that's easy to leave sitting for a year.

@Aymericr
Aymericr merged commit f079046 into pascalorg:main Aug 4, 2026
3 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