Skip to content

Test: add a repo-level release version consistency test - #297

Open
AmaadMartin wants to merge 1 commit into
mainfrom
feat/release-version-consistency-test
Open

Test: add a repo-level release version consistency test#297
AmaadMartin wants to merge 1 commit into
mainfrom
feat/release-version-consistency-test

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:
    Problem: adk-js publishes every package as one linked group: release-please-config.json configures the node-workspace and linked-versions plugins so that the root package, core, dev and integrations always move to the same version. Nothing in the test suite enforces that invariant. A partially applied release — an interrupted release-please run, a hand-edited .release-please-manifest.json, a manual bump of one workspace — can leave core at one version and dev at another and still produce a fully green CI run. The failure only surfaces at publish time, as a broken npm release where @google/adk-devtools@X depends on @google/adk@Y, or on the next release, which then computes the wrong next version from a stale manifest.

Solution: Add one repo-level integration test, tests/integration/release/version_consistency_test.ts, that reads the release inputs from disk at test time and asserts:

  1. The four package.json version fields and the four .release-please-manifest.json entries are one and the same string (all eight values compared in a single toEqual over a record keyed by source label, so a failure diff names every file that drifted rather than only the first).
  2. That version matches /^\d+\.\d+\.\d+(?:-[\w.]+)?$/, so the equality above can never pass vacuously on an empty or malformed value.
  3. The set of packages the test checks is exactly the release group: the keys of release-please-config.json's packages, the keys of the manifest and the keys of the test's own LINKED_PACKAGES map are the same set; each package's configured component matches; and the linked-versions plugin's components list is the same set of component names. Adding a fifth workspace to the release group without extending this test is therefore itself a test failure, rather than a silently unchecked package.

Design notes:

  • No new dependency, no config change. The file lands under the existing integration vitest project's include glob (tests/integration/**/*_test.ts), so vitest.config.ts is untouched. It imports nothing from core/src, dev/src or integrations/src, so it needs no build step and cannot move the coverage thresholds (their include is */src/** only).
  • resolveJsonModule is not enabled in the tsconfig chain, so the JSON is read with node:fs + JSON.parse rather than imported. JSON.parse returns any, so every parse result is assigned to an explicitly unknown-typed local and narrowed through the isJsonObject type guard and the small readStringField / readObjectField / readArrayField accessors. There is no any, as any, as unknown as, @ts-expect-error or eslint-disable anywhere in the file.
  • Cross-platform paths. .github/workflows/validation.yaml runs the suite on [ubuntu-latest, windows-latest, macos-latest], so the repo root is resolved with fileURLToPath(import.meta.url) + path.resolve (the existing idiom in tests/integration/adk_web/webui_test.ts), never process.cwd(), and package paths go through path.join.
  • Structural problems fail loudly, by name. A missing or non-string version, a non-object JSON document, a missing packages/plugins key or an absent linked-versions plugin fails with a message naming the file and field (e.g. Expected "version" to be a string in core/package.json) instead of letting undefined propagate into an assertion that would then pass vacuously. These are raised with expect.fail(...) rather than throw new Error(...) so the runner reports them as assertion failures (and so TypeScript narrows the value afterwards).
  • Operational consequence, stated deliberately: this test will intentionally fail on any future release PR that lands a partial version bump. That is the point of the change, not a regression.

Out of scope on purpose: the per-package src/version.ts literals and integrations/test/version_test.ts are not touched here.

Collision check (required before implementation): gh pr list --state open --limit 100 was inspected, plus the file lists of the two plausibly adjacent PRs. #258 (Test: add self-maintaining version consistency tests for core and dev) adds core/test/version_test.ts and dev/test/version_test.ts, which pin each package's exported version against its own package.json; #245 touches integrations/test/version_test.ts and the root package.json scripts. Both are single-package by construction and cannot observe cross-package drift or manifest drift. No file overlap with this change (which adds exactly one new file), so this branches from main rather than stacking.

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. Targeted run:

$ npx vitest run --project integration tests/integration/release/version_consistency_test.ts
 ✓ |integration| tests/integration/release/version_consistency_test.ts (2 tests) 7ms
 Test Files  1 passed (1)
      Tests  2 passed (2)

Proof the tests can fail. A consistency test that has never been observed failing is indistinguishable from one that reads the wrong paths. Every assertion and every guard branch in the new file was mutated and observed failing, then reverted (git status clean afterwards):

# Mutation Observed failure
1 dev/package.json version9.9.9 AssertionError: expected {…} to deeply equal {…}, diff line - "dev/package.json": "1.4.0" / + "dev/package.json": "9.9.9"
2 .release-please-manifest.json "core"9.9.9 same record diff, naming .release-please-manifest.json#core
3 delete "integrations" from the manifest test 1: Expected "integrations" to be a string in .release-please-manifest.json; test 2: expected [ '.', 'core', 'dev' ] to deeply equal [ '.', 'core', 'dev', 'integrations' ]
4 config component: "devtools""devtool" expected {…} to deeply equal {…}, diff - "dev": "devtools" / + "dev": "devtool"
5 drop "integrations" from the linked-versions components expected [ 'adk', 'devtools', 'main' ] to deeply equal [ 'adk', 'devtools', 'integrations', 'main' ]
6 set all five files to a malformed 1.4 (equality would still hold) expected '1.4' to match /^\d+\.\d+\.\d+(?:-[\w.]+)?$/ — the anti-vacuity guard
7 core/package.json version → the number 140 Expected "version" to be a string in core/package.json
8 manifest replaced with [] Expected .release-please-manifest.json to hold a JSON object
9 config packages[] Expected "packages" to be an object in release-please-config.json
10 config plugins{} Expected "plugins" to be an array in release-please-config.json
11 remove the linked-versions plugin entry Expected a "linked-versions" plugin in release-please-config.json
12 linked-versions components → a string Expected "components" to be an array in release-please-config.json#linked-versions

On coverage: the new file is test code and is not instrumented — vitest.config.ts's coverage include is core/src/**, dev/src/**, integrations/src/**, so this change cannot move the thresholds. Both it blocks execute every non-guard line on a healthy repo; the guard branches are unreachable by design when the repo is consistent, and mutations 3 and 6–12 above are the direct evidence that each of them is live and produces a named message.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

From the repo root:

npm install && npm run build         # required once: the integration project's globalSetup imports @google/adk
npx vitest run --project integration tests/integration/release/version_consistency_test.ts

To watch it catch a real drift, edit any one of package.json, core/package.json, dev/package.json, integrations/package.json or .release-please-manifest.json to a different version and re-run — the failure diff names the file you changed. Revert afterwards.

Full local validation run on the pushed commit:

$ npx vitest run --project integration            # whole integration project
 Test Files  3 failed | 33 passed (36)
      Tests  15 failed | 74 passed | 13 skipped (102)

The three failing suites (build_setup, agent_loader/agent_dirname_test.ts, skills/script_js) are the fixture-installing suites; they fail in this sandbox with npm ERR! code E401 Incorrect or missing password because the environment has no reachable npm registry. They fail identically without this change, and the new test spawns no process and touches no network.

CI on this PR — read this before re-running it. run-tests is green on ubuntu-latest and macos-latest. The windows-latest leg is red, and it is red on a pre-existing failure that this change does not touch:

FAIL  integration  tests/integration/tools/run_skill_script_tool_test.ts
  > RunSkillScriptTool Integration with UnsafeLocalCodeExecutor
  > successfully executes a real PowerShell skill script
Error: Test timed out in 5000ms.
 Test Files  1 failed | 214 passed | 20 skipped (235)

The new test itself passed on all three legs, Windows included:

✓  integration  tests/integration/release/version_consistency_test.ts (2 tests) 6ms

That PowerShell case is it.skipIf(!IS_WINDOWS) and spawns a real powershell -File <tmp>/script.ps1 child process against vitest's default 5000 ms testTimeout, which the repo never overrides. It is marginal, not broken: on a recent passing Windows run of a sibling PR the same file took 5105 ms for its 12 cases, i.e. the spawn lands within a few hundred milliseconds of the budget. Because vitest packs files across a fixed set of forked workers, adding any file to the integration project reshuffles that packing, which is enough to tip a test sitting that close to its limit — which also explains why re-running the job reproduces the same result rather than flaking green: the same file set schedules the same way.

This PR deliberately does not "fix" that by widening the timeout here. Doing so would (a) bundle an unrelated change into a PR whose entire diff is one new file, and (b) duplicate #210, which is open against exactly this file and raises those four Windows-only cases to an explicit 60 s. This change should land on top of, or after, that one.

$ npm run lint            # eslint "**/*.ts"        -> clean, no output
$ npm run format:check    # prettier "**/*.ts"      -> All matched files use Prettier code style!
$ bash scripts/check_license.sh                     -> ✅ All files have the correct license header.
$ npx tsc --noEmit | grep -c version_consistency    -> 0   (pre-existing errors elsewhere in the test tree are untouched)

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.

release-please keeps core, dev, integrations and the root package in one
linked-versions group, but nothing enforced that invariant: a partially
applied release could leave the workspaces and
.release-please-manifest.json on different versions with fully green CI.

This test reads the four package.json files, the manifest and
release-please-config.json at runtime and asserts they all declare one
version, and that the set of packages it checks is exactly the
linked-versions group, so adding a workspace to the release group without
extending this test fails loudly.
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.

1 participant