Fix: derive the integrations version assertion from package.json and run the dead unit:integrations vitest project - #476
Conversation
The assertion hardcoded '1.3.0' while integrations/src/version.ts exports '1.5.0'. release-please rewrites integrations/package.json and the x-release-please-version line in integrations/src/version.ts together but has no knowledge of this test, so any literal here is guaranteed to go stale at the next release. Read the sibling manifest at test runtime (resolved from import.meta.url, since vitest runs from the repository root) and assert against it. That keeps the expectation correct across future bumps and, unlike a literal, actually detects the failure mode this file exists to guard: the two release-please targets drifting out of agreement.
vitest.config.ts defines a unit:integrations project, but no npm script named it. --project is an allowlist and vitest warns about nothing, so integrations/test/ had never executed anywhere -- including in CI, which runs npm run test:coverage. Select the project from test, test:unit and test:coverage so a regression under integrations/test/ fails the build.
|
Closing this as a duplicate of #236, which already lands both halves of this task and is open, green on all three OS legs, and My pre-work collision scan missed it: I ran
I checked whether #236's static JSON import has a latent typing problem, because the spec I was working from asserted that The only substantive delta left is the extra |
1 participant
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Related: feat(integrations): create new top-level integrations package google/adk-js#449 (the PR that introduced the now-stale literal)
Problem: Two coupled defects, neither of which is fixable alone.
integrations/test/version_test.tsassertedexpect(version).toBe('1.3.0'), butintegrations/src/version.tsexports'1.5.0'.release-please-config.jsonlistsintegrations/src/version.tsas anextra-filesentry for theintegrationscomponent, so release-please rewritesit (and
integrations/package.json) on every release — but it has no knowledgeof the test. The test was added at
1.3.0in feat(integrations): create new top-level integrations package google/adk-js#449 and never touchedsince, while
version.tswas bumped twice by release PRs. Any literal in thisfile is guaranteed to go stale at the next release, so simply changing
'1.3.0'to
'1.5.0'would reintroduce the identical bug.vitest.config.tsdefines aunit:integrationsproject, but no root npm script named it.
--projectis an allowlist and vitestemits no warning for a project that is never selected, so
integrations/test/had never executed anywhere — including CI, which runs
npm run test:coverage.That is why defect 1 went unnoticed.
Reproduced on
mainbefore the change:...while
npm run test:coveragepassed theunit:integrationsproject byentirely:
grep -c "unit:integrations"over the full pre-changenpm run test:coverageoutput returns 0.Solution: two commits, both required.
integrations/package.jsonat runtime and assertsversionequalsmanifest.version, plus a shape check pinning themajor.minor.patchformatthat the
// version: major.minor.patchcomment inversion.tsdocuments.Neither assertion subsumes the other: the shape check pins the format, the
manifest check pins agreement between the two files release-please rewrites
together — which is the real failure mode (a rewrite of
package.jsonthatsilently misses the
x-release-please-versionextra-file, or a hand-edit of onewithout the other). The manifest path is resolved from
import.meta.url, not theprocess CWD, because vitest projects run from the repository root; that also
keeps it correct on the Windows and macOS legs of the CI matrix.
--project unit:integrationstotest,test:unitandtest:coverage, sothe directory actually gates merges.
.github/workflows/validation.yamlalreadycalls
npm run test:coverage, so no workflow change is needed.Why a runtime
readFileSyncrather thanimport pkg from '../package.json':resolveJsonModuleis not enabled anywhere in the tsconfig chain (roottsconfig.jsonextendsgts/tsconfig-google.json), so a static JSON import doesnot type-check, and under
"module": "nodenext"it would additionally require animport attribute.
An existing test was rewritten rather than added to. The repository convention
is normally "add a new test, do not modify an existing one", and that convention
exists to preserve a regression signal. This is the documented exception: the
existing assertion was itself the defect — a stale literal that had been failing
invisibly — so there was no correct signal to preserve. The file's regression signal
is strictly strengthened, from one brittle assertion that must be hand-edited every
release to two derived assertions that never need editing. No other test was
modified, skipped, weakened or deleted.
Not touched (all verified with
git diff --quiet):integrations/src/version.ts(release-please managed — the// x-release-please-versionand// version: major.minor.patchcomments areload-bearing),
release-please-config.json,vitest.config.ts(the projectdefinition was already correct; only the scripts selecting it were wrong), the
coverage
thresholdsblock,.github/workflows/validation.yaml, andpackage-lock.json(no dependency added). The diff is 2 files, +27/-5.Collision check. Per the pre-work check against the open PRs on this fork
(
gh pr list --limit 200, thengh pr diff --name-onlyover every one of them,filtered for
package.jsonandintegrations/):Feat: port FirestoreSessionService...) overlaps. It contains abyte-identical version of the
package.jsonscript wiring in this PR (itspost-image blob for
package.jsoniscf793293, the same as this branch's), andit also touches
integrations/test/version_test.ts— but with-expect(version).toBe('1.3.0')/+expect(version).toBe('1.5.0'), i.e. thehardcoded-literal fix that this PR exists to eliminate. It therefore does not land
this change. I did not stack on it: Feat: port FirestoreSessionService from adk-python to @google/adk-integrations #466 is a large feature port that adds a
new runtime dependency (
@google-cloud/firestore), and gating a two-file CIhygiene fix behind it — including for the separate upstream-port step — would be a
worse outcome than a trivially resolvable overlap. Whichever merges second, the
package.jsonhunks are identical and theversion_test.tsresolution should bethis PR's derived assertion, not the literal.
Test: fail CI when a root test script targets a vitest project no workflow runs) is adjacent but complementary and does not conflict — it addstests/integration/repo_config/workflow_scripts_test.tsonly. Its assertion(every project a root script targets must be run by some workflow) still holds
after this change, because
unit:integrationsis added totest:coverage, whichvalidation.yamlinvokes.Test: pin core and dev version exports to their package.json) coversthe
coreanddevpackages and touches neither file in this diff. Deliberatelyleft out of scope here.
package.jsonscripts orintegrations/.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
The change is the test. Both new assertions execute on every run and the file has
no branches, so new-line coverage is 100% by construction.
Mutation testing — proof each assertion can actually fail. Coverage alone is not
proof, so each test was run against mutated source.
integrations/src/version.tswas reverted after every mutation and confirmed byte-identical to
HEAD(
git diff --exit-code integrations/src/version.ts).Mutation 1 —
export const version = '1.5.0'→'9.9.9'(simulatesrelease-please rewriting the manifest but missing the extra-file). The
manifest-agreement test fails; the shape test still passes, which is what proves the
manifest check — not the shape check — is the one that catches release drift:
Mutation 2 —
'1.5.0'→'not-a-version'. Both tests fail, proving the shapecheck is live:
Proof the script wiring is live (defect 2).
npm run test:unitnow picks theproject up — 182 → 183 test files, 2574 → 2576 tests:
And with mutation 1 re-applied, the failure now reaches the exact command CI runs —
which, as shown above, it provably did not before (0 matches pre-change):
Coverage before/after. Measured on real CI (
run-tests (ubuntu-latest), the samejob on both sides) so these are full
npm run test:coveragenumbers, not a subset:fork
mainat this branch's exact base commitb390217eversus this PR.main@b390217e)Test files 224 → 225, tests 2678 → 2680 passed. Every metric is above the thresholds
(statements 86 / branches 87 / functions 88 / lines 86) and every metric moved up,
which is the direction the change predicts:
vitest.config.tsalready listedintegrations/src/**/*.tsundercoverage.include, so those three files were alreadyin the denominator at 0%; executing the test loads two of them for the first time.
Nothing is added to the denominator. (
index_web.tsis the browser entry point, is notimported by this test, and stays at 0% — pre-existing and out of scope.)
The
thresholdsblock was not modified.That same before/after CI log pair is also the cleanest proof of defect 2:
grep -c unit:integrationsover the pre-change CI log returns 0, while over thisPR's CI log it matches both the command line and
✓ unit:integrations integrations/test/version_test.ts (2 tests) 8ms.(The numbers come from CI rather than my sandbox because the full
npm run test:coveragecannot go green there for reasons that pre-date this change — thee2eproject needs a real
GOOGLE_GENAI_API_KEY, theintegrationfixtures need networknpm install, and oneunit:devgcloud test picks up the machine's real gcloudconfig through its mock — and vitest does not emit the coverage table when tests fail.)
CI status on this PR: green on all three OS legs —
run-tests (ubuntu-latest),run-tests (macos-latest)andrun-tests (windows-latest)all pass(run 30715200431),
with the new project executing on each:
✓ unit:integrations integrations/test/version_test.ts (2 tests).The Windows leg needed two reruns, and it is worth recording why so a reviewer does
not read it as instability introduced here. It failed on a different test each
time, and never on this diff:
b390217e(main, before this PR)adk_web/webui_test.ts > 'Run from ADK CLI'integration/build_setup/build_setup_test.ts > js_esmTest timed out in 20000msunit:core core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdoutTest timed out in 5000msAll three are process-spawning tests racing a fixed timeout on a slow runner, and the
Windows leg is chronically red on this fork independently of this PR: it failed or was
cancelled in 4 of the last 6
validation.yamlruns onmain(macOS failed in 2 ofthose 6). This change adds two filesystem-read assertions and alters no timeout, no
testTimeout, and not theintegrationproject's selection.Other CI steps, run on the exact pushed commit:
No
@ts-expect-error,@ts-ignore,eslint-disable,as any,as never, orcoverage-ignore pragma appears anywhere in this diff, and there is no
console.log.const manifest: {version: string} = JSON.parse(...)needs no suppression:eslint.config.jsuses the untypedtseslint.configs.recommendedpreset, sono-unsafe-assignmentis not active, andtsc --noEmitis clean.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.