fix(tests): make apps/web and apps/api suites green - #164
Merged
Conversation
`bun run test` failed with 149 failures in apps/web and 240 in apps/api. Two independent causes, no production code involved. Cause 1 — cross-file mock leakage (the large majority). bun registers mock.module() factories globally for the whole test process and never unwinds them. Almost every component/router test mocks a module partially (say @tanstack/react-query with only useMutation), so in a combined run the first partial factory wins and every later file importing a missing export dies with "SyntaxError: Export named 'useX' not found in module ...". Run alone, 157/170 web files and 123/129 api files already passed. Each app now runs one `bun test` process per file (scripts/test-isolated.ts), which is the same remedy CI already applies to apps/api's OpenAPI contract test, generalized. Parallel, so the web suite takes ~10s and api ~21s. Cause 2 — 19 genuinely broken files, fixed individually: - Tests that never mocked tRPC/react-query/website context and only passed when a sibling file's mock happened to leak in (precision-flow-section, promo-precision-flow-scene, fake-conversation, page-tree-node, timeline-ui-test-page, use-faq-mutations, use-file-mutations). - Assertions left behind by deliberate source changes: background fps/pointer trail defaults (changed in "feat: bolder ascii"), the tools catalog order and section counts, and the visitor-source-badge favicon, which asserted 12 while the source has always rendered 16 intrinsic pixels at a 12px display size — both landed in the same commit, so it never passed. Where the source exports a constant the test now reads it instead of hardcoding, so tuning a value cannot silently break the test again. - docs-widget-release hardcoded /Users/anthonyriera/code/cossistant-monorepo, so it only passed on one machine and would fail in CI. It now derives the repo root from import.meta.dir. - seo-routes and sitemap assumed a localhost origin; getSiteUrl() only falls back to localhost when NODE_ENV is "development", which bun test does not set. They now pin NEXT_PUBLIC_APP_URL. - Tests whose fake db predated a query extraction (plan.self-hosted, website, contact-control, load-context) now serve the same fixtures through the extracted queries. Result: 15/15 turbo test tasks pass — web 692, api 703, protocol 18, memory 33, facehash 24, release 8. check-types 20/20 and Biome clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b159aeb14d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const args = process.argv.slice(2); | ||
| const filters = args.filter((arg) => !arg.startsWith("-")); | ||
|
|
||
| const glob = new Glob("src/**/*.test.{ts,tsx}"); |
There was a problem hiding this comment.
When apps/web runs bun run test, this glob limits discovery to src, whereas the previous bun test command also discovered the repository-root next.config.test.ts. That configuration test is now silently skipped, so the suite can report success even if the Facehash image-host configuration it protects regresses; include root-level tests in the isolated runner's discovery.
Useful? React with 👍 / 👎.
apps/workers (7 files), packages/jobs (8) and packages/tiny-markdown (1) each had test files but no `test` script, so `turbo run test` skipped them entirely and 16 files had never run in CI. Wiring them up surfaced two real failures: - apps/workers/src/queues/index.test.ts mocked five worker factories but not ./lifecycle-email/worker, which was added later. Unmocked it builds a real BullMQ worker and opens a Redis connection, so the test could only pass against a live Redis — it failed with ECONNREFUSED everywhere else. It also mocked @cossistant/redis partially, dropping createRedisConnection. - packages/jobs ai-agent-background asserted a 60s default delay; AI_AGENT_BACKGROUND_DELAY_MS is 30s. The test now reads the exported constant instead of hardcoding it. Full suite: 18/18 turbo test tasks pass — web 692, api 703, jobs 52, workers 33, memory 33, facehash 24, protocol 18, release 8, tiny-markdown 7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
fake-support-context rebuilt its state object on every getState/getSnapshot call. useSyncExternalStore requires getSnapshot to return a stable reference between store changes — a fresh object each call makes React see a new snapshot on every render and bail out with "Maximum update depth exceeded". Its subscribe() was also a no-op, so store changes never reached React. Harmless today because nothing subscribes to the controller through useSyncExternalStore. #161 adds useStoreSelector(controller, ...) inside useSupport(), at which point the timeline UI test page infinite-loops — in the browser as well as in tests. The real controller in @cossistant/core already does this correctly: it holds `snapshot` in a closure and rebuilds it only in syncSnapshot(). The fake now mirrors that and forwards store changes to its subscribers, so it satisfies the contract in both cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
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 free
to 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.
bun run testcurrently fails: 149 failures inapps/web, 240 inapps/api— and three more packages had test files that never ran at all. This makes the whole monorepo green. No production code is touched — only test files,testscripts, and the runner script.Cause 1 — cross-file mock leakage (the large majority)
bun registers
mock.module()factories globally for the whole test process and never unwinds them. Nearly every component/router test here mocks a module partially (e.g.@tanstack/react-querywith onlyuseMutation). In a combined run the first partial factory wins, and every later file importing a missing export dies with:That is not a defect in the tests. Run individually, 157/170 web files and 123/129 api files already passed.
Each app now runs one
bun testprocess per file viascripts/test-isolated.ts— the same remedy CI already applies toapps/api's OpenAPI contract test (and documents in a comment), generalized. It runs files in parallel, so it costs almost nothing (web ~10s, api ~20s). The runner takes a path filter (bun run test src/lib) and prints full output for any failing file.Cause 2 — 19 genuinely broken files
Fixed individually, not worked around:
precision-flow-section,promo-precision-flow-scene,fake-conversation,page-tree-node,timeline-ui-test-page,use-faq-mutations,use-file-mutations.targetFpsand pointer-trail defaults (retuned infeat: bolder ascii), and the tools catalogorder/section counts (the catalog grew). Where the source exports a constant, the test now reads it instead of hardcoding, so future tuning cannot silently break the test again.visitor-source-badgeassertedheight="12"while the source has always renderedheight={16}at a 12px display size (size-3, for favicon crispness). Both landed in the same commit — this test has never passed.docs-widget-releasehardcoded/Users/anthonyriera/code/cossistant-monorepo, so it only passed on one machine and would fail in CI. It now derives the repo root fromimport.meta.dir.seo-routes/sitemapassumed a localhost origin.getSiteUrl()only falls back to localhost whenNODE_ENV === "development", whichbun testdoes not set. They now pinNEXT_PUBLIC_APP_URL.plan.self-hosted,website,contact-control,load-context. The routers moved to extracted queries (listOrganizationWebsitePlanTargets,listActiveAiAgentSummariesForWebsite,getActiveVisitorForWebsite,db.query.website); these now serve the same fixtures through those queries, so existing call sites are unchanged.Cause 3 — suites that never ran
apps/workers(7 files),packages/jobs(8) andpackages/tiny-markdown(1) had test files but notestscript, soturbo run testskipped them. Wiring them up surfaced two real failures:apps/workers/src/queues/index.test.tsmocked five worker factories but not./lifecycle-email/worker, added later. Unmocked, it builds a real BullMQ worker and opens a Redis connection — so it could only pass against a live Redis and failed withECONNREFUSEDanywhere else. It also mocked@cossistant/redispartially, droppingcreateRedisConnection.packages/jobsai-agent-backgroundasserted a 60s default delay;AI_AGENT_BACKGROUND_DELAY_MSis 30s. It now reads the exported constant.Verification
bun run test: 18/18 turbo tasks pass@cossistant/api@cossistant/web@cossistant/jobs@cossistant/workers@cossistant/memoryfacehash@cossistant/protocol@cossistant/release@cossistant/tiny-markdownbun run check-types20/20. Biome clean on every changed file.Note
With
apps/api's suite now isolated per file, the dedicated "Test REST OpenAPI contract" CI step is redundant (it exists only to dodge this leakage). Left in place here — worth removing separately.🤖 Generated with Claude Code