test: boot the built package in a browser, and cover the two branches nothing could reach - #483
Merged
Merged
Conversation
…y do nothing Every other spec here mounts something the module has already configured, which leaves the configuring untested. #314 lived exactly there: `b24ui.version` was a public, typed option that `setup()` ignored, and there is no failure signal for that class of bug — no error, no warning, just a `<meta>` tag with the wrong string. #324 fixed it and covered only the consuming half. Calling `setup()` directly does not work — `defineNuxtModule` merges defaults through defu before the body runs, and the body reaches for `@nuxt/kit`'s ambient Nuxt context. `loadNuxt` gives a real instance without a build: it resolves the config, installs modules and stops, in about three seconds. That needs plain Node, so the suite grows a third vitest project. `module/` is recorded in `test/vitest-include.ts` alongside the other two, and `vitest-include.spec.ts` now asserts the separation in both directions — a `module/` spec leaking into another project would not fail, it would hang. Three cases, each verified by breaking the line it covers: the option is honoured, it falls back to the package version, and `theme.prefix` reaches both places `getDefaultConfig` writes it. Refs #329
The Vue playground threw on boot and had been doing so unnoticed, because nothing in this repository has ever loaded a page in a browser. b24ui's `Link` renders vue-router's own `RouterLink`, which reads the router through `inject(routerKey)`. `routerKey` is a module-level Symbol, so two copies of vue-router in one bundle are two different keys: the injection returns `undefined` and `RouterLink` dies with `Cannot destructure property 'options'` on first render. The playground rendered a page with no navigation on it. Two copies is what this workspace has. The playground depends on vue-router 5.2.0; the repo root gets 5.1.0 hoisted from nuxt, and the b24ui runtime resolves from the root. `resolve.dedupe` is Vite's answer to exactly that. Not a packaging defect: `vue-router` is already an optional peerDependency, so an installed copy is shared and a consumer gets one. Which is why this survived — it needed something to open the page. Refs #329
…ranch `ci.yml` ran lint, typecheck, test and build, and no step ever started an application. #301 was a client-only boot failure: the unit suite stayed green for five weeks while the published package could not start an SPA. Nothing about "builds fine, dies on boot" was covered, and the first run of this script found a live instance of it (fixed in the previous commit). `pnpm build && pnpm test:smoke` builds two applications against the built package — a minimal Nuxt app under `test/smoke/fixture` and the Vue playground — serves both, loads them in Chromium and fails on anything the browser logs. Console errors rather than status codes, because a Vue app that throws in `setup()` still answers 200 with an empty root: `curl` cannot tell the two apart and that is the whole reason this needs a browser. The Nuxt half also closes the second gap in #329. `platform.ts` reads the user-agent off the request during SSR to set `data-platform` / `data-version`, which the `bitrix-mobile:` and `bitrix-desktop:` Tailwind variants key on. The vitest environment is client-only, so that branch has never run; the smoke run fetches the page with three user agents and checks the attributes. The script refuses to run against a `--stub` dist. `dev:prepare` leaves one behind, and a smoke run against it would boot `src/` under a different name and report success for a package nobody built. Verified by mutation: removing the SSR branch fails the two Bitrix user-agent checks (the `web` fallback cannot fail, and says so in a comment); making a runtime plugin throw on the client fails all four boot checks; removing the vue-router dedupe fails the SPA console check alone. Costs CI a Chromium download plus about a minute of builds. Adds `playwright-core` as a devDependency. Resolves #329
Resolves `package.json` and `pnpm-lock.yaml` against #482's dependency bumps: takes main's versions wholesale, then re-adds this branch's two entries — `playwright-core` and the `test:smoke` / `test:module` scripts — and regenerates the lockfile so `--frozen-lockfile` still holds.
…edule the job Six reviews of #483. The findings, and what each one changed. **The SPA render check was very nearly vacuous.** It measured `text.length > 100`, and the tester proved it: killing `RouterLink` — the very bug this file found — left it green, because the playground carries enough static copy to clear the threshold on a broken page. It counts the navigation's links now. Working: 80. With the dedupe removed: 1. **`serveStatic` had three ways to take the whole run down.** `file.startsWith(dir)` is a directory-escape check with no separator boundary, so a sibling named `dist-anything` passes it; `relative()` answers the question that was being asked. `decodeURIComponent` throws on a malformed `%` escape, inside a request handler, skipping every cleanup below. And a `createReadStream` with no `error` listener makes a missing file fatal instead of a failed check. **`waitForServer` never watched the child.** A Nitro server that died on boot burned the full 60 seconds and reported "did not answer", hiding the exit code that said why. **Cleanup could strand things.** `browser` and the server were started before the `try`, and the `finally` awaited `browser.close()` first — a rejection there skipped the static server and the `kill()`. Everything is inside the `try` now and each teardown is independent. **`npx` undercut the pinned `playwright-core`**, since it falls back to the registry on a resolution miss. `pnpm exec` in both the script and the workflow. **The module suite moved out of the shared vitest run.** Measured, not assumed: the `vue` project alone OOMs a worker at the fork heap limit — 152 of 153 files — on `main`, with none of this branch's files in it. The component suite is sitting on that edge and CI has been passing on the near side of it. Adding a `loadNuxt` instance to the same pool is not a risk worth taking for three specs. `vitest.module.config.ts`, `pnpm test:module`, its own CI step. The growth itself is a real defect, is not this branch's, and is filed separately. **The smoke job is now `smoke.yml`, nightly and dispatchable**, on the maintainer's call: a browser download and two application builds on every pull request is a poor trade for a library whose runtime plugins change rarely. The cost of that choice is named in the workflow and in testing.md — a boot failure is found the morning after it lands, so dispatch it by hand when you touch a runtime plugin, `setup()`, or anything that reaches the client bundle. Also: cache the browser keyed on the pinned version rather than the lockfile; say in the workflow why `--with-deps` and its sudo do not widen `permissions:`; document the two extra suites and the `--with-deps` difference in testing.md. Refs #329
This was referenced Aug 24, 2026
Closed
Closed
`test/utils/indistinguishable-snapshots.spec.ts` collected every `.snap` file
under `test/` with `readdirSync(root, { recursive: true })`. That option has no
ignore list: it walks everything and returns every path in one array — and
`test/` holds installed dependency trees, `test/nuxt/node_modules` and, since
this branch, `test/smoke/fixture/node_modules`. Hundreds of thousands of
strings, built to find 212 files.
The worker died at the heap limit, and vitest reported `Worker exited
unexpectedly` with no file name, so it read as a suite-wide condition:
main @ 6f0e71e 314 files green
this branch, 315 315 files OOM
this branch, 314 314 files OOM
--project vue alone 153 files 152 passed, 1 error
The last row named it. Halving the heap to 1 GB killed it at exactly the same
point — with real accumulation an eight-times-smaller cap would have died eight
times sooner — so exactly one file was responsible, and a verbose reporter
diffed against the collected list said which.
`snapshotFiles()` now prunes as it walks. Same 212 files, same two directories,
120+ seconds and a dead worker down to 2ms and 46 MB. The whole suite goes from
312 of 314 with two errors to 314 of 314, and from 720s to 341s.
Guarded by a test that builds a temp tree with a snapshot inside
`node_modules/` and one inside `.output/` and asserts neither is returned.
`testing.md` gains the bisect, because the failure names nothing and the cheap
first move is not obvious: halve the heap, and see whether the same number of
files passes.
… fixtures lying
Six more reviews. What each one changed.
**The fixtures were not what their comments claimed.** c12 resolves the
workspace root and merges the repository's own `.nuxtrc` into every Nuxt
instance under it, so both fixtures were booting with `@nuxt/content` installed
and `experimental.normalizeComponentNames=false`. The second one matters: it is
`true` by default in Nuxt 4, so the smoke run was booting in a configuration no
consumer has — and component resolution is exactly the class of boot failure
this file exists to catch. Restored explicitly in the config, which wins over
the rc. Both comments now say what is actually inherited instead of "kept
deliberately bare".
**The smoke run could not tell a hydrated app from a server-rendered one.**
Every check was satisfied by HTML that arrives before any client code runs, so
hydration that hangs quietly — a promise that never settles, no exception
anywhere — passed. It clicks the button now and asserts the badge moved. Proved
by adding `defineNuxtPlugin(() => new Promise(() => {}))` to the fixture: that
one check goes red and every other check stays green, which is the tester's
point made in one line.
**Console collection stopped the instant the network went quiet.** Anything
logged from `onMounted`, a deferred plugin or a lazy chunk landed after
`page.goto` returned, and the check passed or failed by how fast the runner
was. There is a 750ms settle window now; verified with a `setTimeout(…, 500)`
that logs an error, which is caught.
**The directory-escape guard repeated the bug it warns about.**
`relative(...).startsWith('..')` has no separator boundary either — in the
other direction: it rejects an ordinary file called `..hidden.js`, which then
gets served as `index.html` and reads as a routing bug. It asks about `..`
segments now.
**Three more in `run.mjs`.** The readiness probe never cancelled its response
body, holding an undici socket per attempt. `page.close()` was not in a
`finally`, so a `goto` timeout cancelled the SPA half of the run instead of
recording a failure. And the teardown comment promised independent steps while
only the first one was guarded.
**`vitest.module.config.ts` still carried the wrong diagnosis** — a claim that
the component suite leaks memory "on main, with none of this branch's files in
it", and a defect "filed separately" with no number. Neither survived the
bisect: it was #485, a directory walk in a spec of ours, fixed here. The split
survives its own bad argument and now says so.
Also: a 20-minute timeout on the nightly job, because nobody is watching it and
the GitHub default is six hours; a note on why `TZ` is not carried into the
module config.
Refs #329
Collaborator
Author
Second review roundSix more reviews after the rework. Four findings that mattered, all fixed here. The fixtures were not what their comments claimed. c12 resolves the workspace root and merges the repository's own The second line is the one that matters. The smoke run could not tell a hydrated app from a server-rendered one. Every check was satisfied by HTML that arrives before any client code runs, so hydration that hangs quietly — a promise that never settles, no exception anywhere — passed. It clicks the button now and asserts the badge moved. Proved by adding Console collection stopped the instant the network went quiet. Anything logged from The directory-escape guard repeated the bug it warns about. Three smaller ones in
Also: Verified by mutation
Gate
Generated by Claude Code |
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
2 participants
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.
Linked issue
Resolves #329 · fixes #485
Follow-ups filed: #486 (
prefixmissing fromAppConfig['b24ui']), #487 (no tests for the smoke runner itself).Type of change
revert(Scope): ...)Description
#329 lists three places where a regression would still be invisible, and ranks them: 3 (nothing in CI boots the package) covers the widest class of failure, 1 (the module's
setup()) protects a fix that has no other signal, 2 (platform.ts's SSR branch) comes nearly free alongside 3. All three are here.The first run of the new smoke test found a live bug, and getting CI green found a second one. That is the short version of why this was worth doing.
3 — nothing boots the package
Nothing in
ci.ymlstarts an application. #301 was a client-only boot failure: the unit suite stayed green for five weeks while the published package could not start an SPA.pnpm build && pnpm test:smokebuilds two applications against the built package, serves both, loads them in Chromium and fails on anything the browser logs:test/smoke/fixture@bitrix24/b24ui-nuxtthrough the workspace link — module registration, runtime plugins, SSRplaygrounds/vueConsole errors are the assertion, not a heuristic. A Vue app that throws in
setup()still answers 200 and still renders something —curlcannot tell it apart from a working one, and that is the entire reason this needs a browser.The script refuses to run against a
--stubdist.pnpm dev:prepareleaves one behind, and a smoke run against it would bootsrc/under a different name.What it found on the first run
vue-router's own
RouterLink, reading the router throughinject(routerKey).routerKeyis a module-level Symbol, so two copies of vue-router in one bundle are two different keys: the injection returnsundefinedandRouterLinkdies on first render. The playground was serving a page with no navigation on it.Two copies is what this workspace has — the playground depends on 5.2.0, the root gets 5.1.0 hoisted from nuxt, and the b24ui runtime resolves from the root. Fixed with
resolve.dedupe.Not a packaging defect:
vue-routeris already an optionalpeerDependency, so a consumer gets one copy. Which is also why it survived — nothing here had ever opened the page.1 —
b24ui.versionnever went through the realsetup()#314 was a public, typed option that
setup()ignored — no error, no warning, just a<meta>tag with the wrong string. #324 fixed it while covering only the consuming half.setup()cannot be called directly:defineNuxtModulemerges defaults through defu before the body runs, and the body reaches for@nuxt/kit's ambient context.loadNuxtgives a real instance without a build, in about three seconds.2 —
platform.ts's SSR branchThe server half has never run — the Nuxt vitest environment is client-only. It sets
data-platform/data-versionon<html>, which thebitrix-mobile:andbitrix-desktop:Tailwind variants key on, so a broken branch means a flash of wrong styling on first paint inside a Bitrix24 frame.Only two of the three user agents can fail if the branch breaks —
web/airis the fallback. That is said in a comment rather than left to be discovered later.The second bug: #485
CI went red on this branch and stayed red after the obvious fix, which is what forced a real bisect.
test/utils/indistinguishable-snapshots.spec.ts— a guard merged this morning in #480 — collected snapshots withreaddirSync(root, { recursive: true }). That option has no ignore list: it walks everything undertest/and returns every path in one array, andtest/holds installed dependency trees. Hundreds of thousands of strings, built to find 212 files. The worker died at the heap limit and vitest reportedWorker exited unexpectedlywith no file name, so it read as a suite-wide condition.main@6f0e71ef--project vuealone--project vue,--max-old-space-size=1024The last row named it. With real accumulation an eight-times-smaller heap dies eight times sooner; it died at exactly the same point, so exactly one file was responsible.
mainwas green only because it had one dependency tree undertest/instead of two — this branch's fixture pushed it over, which is how a pull request that never touched the guard broke it.snapshotFiles()prunes as it walks now. Same 212 files, same two directories, 120+ seconds and a dead worker down to 2 ms and 46 MB. The suite goes from 312 of 314 with two errors to 314 of 314, and from 720 s to 341 s. Guarded by a test that puts a snapshot insidenode_modules/and one inside.output/and asserts neither is returned.testing.mdgains the bisect, because the failure names nothing and the cheap first move is not obvious.Verified by mutation
webstayed green, as documented)setup()ignoresoptions.version(#314 again)honours a b24ui.version set in the app configgetDefaultConfigstops seeingthemethreads theme.prefix into the app confignode_modules/does not descend into an installed dependency treeChanges after review
Six reviews. What each one changed:
The SPA render check was very nearly vacuous. It measured
text.length > 100, and the tester proved it: killingRouterLink— the bug this file found — left it green, because the playground carries enough static copy to clear the threshold on a broken page. It counts the navigation's links now. Working: 80. Dedupe removed: 1.serveStatichad three ways to take the whole run down.file.startsWith(dir)is a directory-escape check with no separator boundary, so a sibling nameddist-anythingpasses it —relative()now answers the question that was being asked.decodeURIComponentthrows on a malformed%escape, inside a request handler, skipping every cleanup below. AcreateReadStreamwith noerrorlistener made a missing file fatal instead of a failed check.waitForServernever watched the child. A Nitro server that died on boot burned the full 60 s and reported "did not answer", hiding the exit code that said why.Cleanup could strand things.
browserand the server were started before thetry, and thefinallyawaitedbrowser.close()first — a rejection there skipped the static server and thekill().npxundercut the pinnedplaywright-core, since it falls back to the registry on a resolution miss.pnpm execin both the script and the workflow.The module suite moved out of the shared vitest run, into
vitest.module.config.ts/pnpm test:modulewith its own CI step. The reasoning at the time was memory, and that reasoning turned out to be wrong — the memory problem was #485. The split is kept on its own merits:loadNuxtin a plain-Node process, three specs, four seconds, and no interaction with the component pool.The smoke job is now
smoke.yml, nightly and dispatchable, on the maintainer's call: a browser download plus two application builds on every pull request is a poor trade for a library whose runtime plugins change rarely. The cost is named in the workflow and intesting.md— a boot failure is found the morning after it lands, so dispatch it by hand when touching a runtime plugin,setup(), or anything reaching the client bundle.Also: the browser is cached keyed on the pinned version rather than the lockfile; the workflow says why
--with-depsand its sudo do not widenpermissions:;testing.mddocuments both extra suites and the--with-depsdifference.Cost
ci.ymlgains one step:pnpm test:module, about four seconds — and gets ~380 s faster, because #485 was costing that. Everything browser-shaped is insmoke.yml, off the pull-request path. Addsplaywright-coreas a pinned devDependency.Checklist
Merged
main(#482's dependency bumps) — lockfile taken from main and regenerated,--frozen-lockfileholds.Gate run locally:
install --frozen-lockfile·test:workflows49/0 ·lint0 ·typecheck0 ·test314/314, 7172 passed, 0 errors ·test:module3/3 ·build·test:smoke8/8.