test(suites): run the extension and react tests the unit suite owns - #4018
Conversation
The runtime critical-flow harness packs only the root `veryfront` package and
points the fixture at that tarball. The root pins its co-published extensions to
its own exact version, so npm resolves those pins from the registry -- and on a
release-cut branch that version is not published yet, because publishing is what
the release PR unlocks. Every version bump fails identically:
npm error code ETARGET
npm error notarget No matching version found for
@veryfront/ext-bundler-esbuild@0.1.1252-rc
It passes on main only because main's version is already on npm. #3977 added the
harness after the last release cut merged, so veryfront-code#4012 is the first
bump to hit it.
Pack the co-published extensions alongside the root and name them in the fixture
manifest, so nothing is fetched from a registry. `npm exec` gets the same set,
since it resolves the CLI's dependencies into its own prefix.
The set comes from the built root manifest, not a directory listing: the build
emits 29 extension packages and the root co-publishes 6. Installing the other 23
would change what the fixture exercises.
Nothing mutates the build output. The fixture manifest is a generated test input
the harness already writes, so there is no artifact to restore afterwards.
Verified against the real build by bumping npm/ to an unpublished 0.1.9999-rc:
root tarball only npm error code ETARGET
No matching version found for
@veryfront/ext-bundler-esbuild@0.1.9999-rc
root + co-published added 64 packages in 7s
veryfront 0.1.9999-rc, ext-bundler-esbuild 0.1.9999-rc,
ext-parser-babel 0.1.9999-rc, ext-yaml 0.1.9999-rc
`UNIT_ROOTS` in run-suite.ts listed only src/, cli/ and templates/, while
three other declarations put extensions/ and react/ in the unit suite:
`pathSelectors` in suites.ts, `test.include` in deno.json, and
suites.test.ts, which asserts `resolveLeafSuiteOwners` returns
`{level: unit, suite: unit}` for both.
Ownership and execution had drifted, so 90 extension test files and
react/react.test.ts never ran in CI. `extensions/*/src/**` still reached
the coverage report, because `deno coverage --include=src/` matches on
path substring: 23,654 lines at 35% covered, holding the gate 2.63 points
below what src/ alone measures. The gate was scoring code whose tests it
silently skipped.
All 90 extension files and react/react.test.ts pass under the unit
suite's flags today: 185 tests, 1,233 steps, 0 failures. Planning goes
from 1,950 files to 2,044.
scripts/ stays out. deno.json's root `exclude` lists scripts/, so those
files are undiscoverable under the main config; `deno test` reports "No
test modules found" for them. They keep running through `test:scripts`
with scripts/test.deno.json.
The suite-parity helpers hardcoded the same three roots, so they pinned
the planner to the legacy commands' blind spot. Both now read
LEGACY_UNIT_ROOTS, and a new case asserts every owned root contributes at
least one planned file.
|
@codex review |
|
Warning Review limit reached
Next review available in: 38 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://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: 5c6c376658
ℹ️ 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".
Addresses Codex P2 on #4018. The first version hardcoded the roots in run-suite.ts and restated them in run-suite.test.ts, so a `pathSelector` added to or removed from the unit suite in suites.ts would leave both copies stale: the regression test would keep passing while UNIT_ROOTS drifted from ownership again, which is the exact failure the test exists to prevent. UNIT_ROOTS now reads the unit suite's own `pathSelectors` and subtracts UNPLANNABLE_UNIT_ROOTS, and the test derives its expectation from the same registry instead of a second hand-kept list. A newly owned root is planned without a second edit, and one that cannot yield files fails the guard. Per AGENTS.md, reuse the existing registry rather than adding a parallel source of truth.
|
Addressed the P2 in You were right that the guard could not do its job. Both now read the registry: const UNPLANNABLE_UNIT_ROOTS = new Set(["scripts/"]);
const UNIT_ROOTS = (() => {
const unit = LEAF_TEST_SUITES.find((suite) => suite.id === "unit");
if (!unit) {
throw new Error("The leaf suite registry no longer defines a unit suite.");
}
return unit.pathSelectors.filter((root) => !UNPLANNABLE_UNIT_ROOTS.has(root));
})();The test derives its expectation the same way rather than keeping a second list, so a newly owned root is planned without a second edit, and one that cannot yield files fails the guard. The Planning is unchanged at 2,044 files ( |
|
@codex review |
…hards CI caught what a local run could not: ext-dev-ui-react's browser-bundle test drives a real Chromium through tests/_helpers/playwright.ts, and only the browser e2e jobs run `playwright install`. In a coverage shard it fails with "Playwright browser bridge exited before reporting an endpoint". It passed locally because a Chromium was already present. It is the only test under a unit root that reaches for a browser -- every other Playwright test lives under tests/e2e/ -- and nothing noticed because extensions/ was never planned. Excluded through BROWSER_DEPENDENT_TESTS, exported so the suite-parity helpers filter on the same set instead of repeating the path. Named rather than dropped silently: the test still needs a home in a browser-capable job, which is follow-up work, not this change. coverage:unit now plans 2,043 files (extensions 89).
|
Pushed
It is the only test under a unit root that reaches for a browser (every other Playwright test lives under Excluded via
Separately, for anyone reading the red checks: |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
… into fix/pack-copublished-extensions
Addresses Codex P1 and P2 on #4017. P1 -- the Deno matrix still reached the registry. `scaffoldProject` forwarded only `packed.root` to `usePackedVeryfrontDenoTasks`, which extracts that tarball and runs `deno install` inside `.veryfront-packed-cli/package`. That manifest still pins the six co-published extensions to its own version, so the sibling tarballs already on disk were never declared to that install and it resolved them from the registry. `redirectCoPublishedPins` now rewrites those pins to `file:` before the install; it only touches pins the manifest already declares, so it cannot widen what gets pulled in. P2 -- templates add first-party extensions the root does not depend on: `minimal` installs @veryfront/ext-content-mdx, `docs-agent` installs @veryfront/ext-document-kreuzberg. Those pins carry the build's version and fail the same way. `localiseTemplateExtensions` packs them from the generated manifest, so only the selected template's extensions are packed and unrelated ones stay out. It runs before the runtime-specific wiring because both paths install from that manifest. Not verifiable by this PR's CI: its head is 0.1.1251, which is published, so the release-cut path cannot reproduce there. Both fixes are aimed at the unpublished-version case that CI does not currently exercise. The two TS2307 errors in cli/utils/terminal-select.ts are pre-existing -- 2 before and after this change on the same file set.
…nt' into fix/pack-copublished-extensions
The extension-suite branch must rerun CI against an unpublished release candidate. Merge the reviewed runtime harness fix so Bun, Deno, and Node resolve co-published extensions from local tarballs instead of the registry. Constraint: Current main identifies an unpublished release candidate Rejected: Ignore the three runtime failures | required checks must exercise the real package flow Confidence: high Scope-risk: moderate Reversibility: clean Directive: Drop this temporary dependency merge after #4017 lands on main Tested: #4017 Bun critical flow passed locally; #4017 Node and Deno critical checks passed in CI Not-tested: Exact #4018 combined head pending CI
…' into fix/pack-copublished-extensions # Conflicts: # scripts/test/runtime-e2e-helpers.ts # scripts/test/template-runtime-e2e.ts
The finalized package harness introduced three non-ASCII dashes in explanatory comments. Rephrase them with periods and commas so the runtime fix follows the repository-wide public-copy constraint without changing behavior. Constraint: AGENTS.md forbids em and en dash characters Rejected: Leave comments unchanged because they are not runtime output | repository copy rules cover source comments too Confidence: high Scope-risk: narrow Reversibility: clean Tested: Focused runtime/template tests passed (3 tests, 27 steps); format, lint, and diff checks passed Not-tested: Exact pushed head full CI pending
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Current main advanced through the semantic-audit PR while extension-suite CI was running. Merge the conflict-resolved and standards-clean #4017 head so this branch remains mergeable and exercises local extension tarballs across every runtime. Constraint: #4009 merged an earlier runtime harness revision into main Rejected: Resolve the duplicated harness independently here | #4017 owns the canonical conflict resolution Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep #4017 as the source of truth for runtime harness conflicts Tested: #4017 exact head has 0 Standards findings, 0 Spec findings, and focused tests pass Not-tested: Exact #4018 combined head full CI pending
The Chromium-backed Dev UI bundle check could not run safely in unit shards, but excluding it from unit and coverage planning left it without an executable suite owner. Move it into the existing Deno browser e2e regression lane, wire that task explicitly, and remove the special unowned exclusion. Constraint: tests/README requires every executable test path to map to exactly one leaf suite and runner owner. Rejected: Keep BROWSER_DEPENDENT_TESTS as a silent unit exclusion | it preserves the unowned-test gap that triggered the review. Rejected: Rename to .playwright.ts | this file uses Deno.test with the repository Chromium helper, not Playwright test() fixtures. Confidence: high Scope-risk: narrow Directive: Browser-backed Deno tests should live under an e2e/browser task instead of being excluded from unit ownership. Tested: red-first run-suite regression failed before the move. Tested: VF_DISABLE_LRU_INTERVAL=1 deno test --config=scripts/test.deno.json --no-check --allow-read --allow-write --allow-run=bash scripts/test/run-suite.test.ts Tested: DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text deno test --no-check --allow-all tests/e2e/regressions/dev-ui-browser-bundle.test.ts --unstable-worker-options --unstable-net Tested: deno task test:e2e:rsc-browser Tested: VF_DISABLE_LRU_INTERVAL=1 deno task test:layout Tested: VF_DISABLE_LRU_INTERVAL=1 deno task lint:test-semantic-dispositions Tested: deno fmt --check deno.json scripts/test/run-suite.ts scripts/test/run-suite.test.ts scripts/test/test-semantic-audit-migration.ts tests/e2e/regressions/dev-ui-browser-bundle.test.ts && deno lint --no-config scripts/test/run-suite.ts scripts/test/run-suite.test.ts scripts/test/test-semantic-audit-migration.ts tests/e2e/regressions/dev-ui-browser-bundle.test.ts Not-tested: full CI matrix locally
|
@codex review @coderabbitai review Please review exact head This head fixes the hard ownership issue from the standards review: Verification:
Review threads are currently all resolved ( |
|
|
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Integrate the latest mainline suite planner and Dev UI browser E2E relocation before publishing the semantic hardening. The combined inventory ratchets from the current base without restoring either migrated test to the unit boundary. Constraint: PR #4018 advanced main and removed an adjacent semantic inventory entry.\nConfidence: high\nScope-risk: moderate\nDirective: Re-measure semantic and layout baselines whenever concurrent migration PRs land.\nTested: semantic audit 4 tests/84 steps; suite planner 2 tests/14 steps; fmt/typecheck; semantic audit 2140/762; layout 2348/2297/51.\nNot-tested: Full repository pre-push gate runs on this merge commit.
2 participants
90 extension test files and
react/react.test.tshave never run in CI, whileextensions/*/src/**still counted toward the 80% coverage gate. The gate was scoring code whose tests it silently skipped.The drift
Four declarations, one disagreement:
extensions/+react/?pathSelectorsinsuites.tstest.includeindeno.jsonsuites.test.ts— assertsresolveLeafSuiteOwners→{level: unit, suite: unit}UNIT_ROOTSinrun-suite.tsOwnership said unit suite; execution disagreed. The existing tests only validated ownership, never that an owned root contributed a file, so the gap was invisible.
Why it mattered beyond the dead tests
deno coverage --include=src/matches on path substring, so it also selectsextensions/*/src/**. Measured from the shard artifacts of CI run32627256834:srcextensions23,654 lines of extension source sat in the gate's denominator at 35%, holding the number 2.63 points below what
src/alone measures — precisely because the tests that would have covered it were never selected.Safety
All 90 extension files and
react/react.test.tspass today under the unit suite's exact flags — verified before changing anything:Planning goes from 1,950 files to 2,044. The gate number should rise, since these tests cover source already in the denominator.
scripts/deliberately excludeddeno.json's rootexcludelistsscripts/, so those files are undiscoverable under the main config —deno testreturns "No test modules found". They keep running through the dedicatedtest:scriptstask withscripts/test.deno.json. Documented in the code rather than left as a silent asymmetry.Test changes
The suite-parity helpers hardcoded the same three roots, so they pinned the planner to the legacy commands' own blind spot — parity with a legacy command is only worth asserting where the legacy command was right. Both now read
LEGACY_UNIT_ROOTS, and a new case asserts every owned root contributes at least one planned file, so this cannot drift again silently.run-suite.test.ts,suites.test.ts,test-layout.test.ts: 7 passed, 38 steps, 0 failed.Relationship to the other open PRs
Independent — branches from
mainand touches neithercoverage-ci.tsnorcodecov.yml, so it does not conflict with #4013/#4015/#4016.Worth noting this supersedes the fix I first considered: excluding extension source from coverage. That would have hidden the symptom and left 90 tests dead. Running them is the fix.