Skip to content

Test: assert the integrations version export is well-formed semver (stacked on #236) - #479

Open
AmaadMartin wants to merge 1 commit into
fix/run-unit-integrations-vitest-projectfrom
fix/integrations-version-test-staleness
Open

Test: assert the integrations version export is well-formed semver (stacked on #236)#479
AmaadMartin wants to merge 1 commit into
fix/run-unit-integrations-vitest-projectfrom
fix/integrations-version-test-staleness

Conversation

@AmaadMartin

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):
    Not applicable — no public issue tracks this.

  2. Or, if no issue exists, describe the change:

Stacked PR. Base is fix/run-unit-integrations-vitest-project (#236), not main.
This PR contains one commit / +7 lines. Read it against that base.

Problem: integrations/test/version_test.ts pinned the exported version to a
hardcoded literal (expect(version).toBe('1.3.0')) while
integrations/src/version.ts had moved on. That literal is structurally
guaranteed to rot: release-please-config.json registers integrations with an
extra-files entry for src/version.ts and groups it under linked-versions, so
every release rewrites integrations/src/version.ts and integrations/package.json
together and never touches the test. The failure survived multiple releases because
the unit:integrations vitest project was not referenced by any root npm script,
so CI never loaded integrations/test/**.

#236 already fixes both of those. It replaces the literal with an assertion
against the sibling manifest and adds --project unit:integrations to test,
test:unit and test:coverage. This PR does not re-implement any of that.

Solution: this PR adds the one assertion #236 does not have — that the exported
version is a well-formed semver string — and nothing else.

Why it is not redundant with the consistency assertion: both files are rewritten
from a single release-please-computed value, so a malformed version propagates to
version.ts and package.json identically and the consistency check still
passes. The two assertions pin different properties — agreement between the files,
and the shape of the value itself. Mutation A below demonstrates exactly this: with
both files set to 'not-a-version', #236's assertion is green and only the new one
fires.

The prerelease branch of the pattern ((-[\w.]+)?) is deliberate. release-please's
generic updater writes whatever version it computes, and a 1.6.0-rc.1 bump must
not turn this test into the next stale one (mutation D).

Collision check (performed before writing any code, as required).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returned 378 open
PRs. #236 (fix/run-unit-integrations-vitest-project, OPEN, MERGEABLE) touches the
same two files and lands the bulk of this work, so rather than open a competing
implementation this PR is stacked on #236's branch. I verified #236 empirically
rather than trusting its diff: applied its test file to a clean checkout, ran the
targeted project (passing), and confirmed tsc --noEmit reports no diagnostics for
integrations/test/** — its import ... with {type: 'json'} form is valid under the
repo's nodenext config, and is in fact more robust than a readFileSync +
fileURLToPath form because it involves no path or cwd resolution at all. Adjacent
but non-colliding, checked by filename: #343 and #418 (CI guards under
tests/integration/repo_config/ that fail when a vitest project or script is
unreferenced — complementary to #236, not duplicative), #297
(tests/integration/release/version_consistency_test.ts, the repo-level
cross-package check), and #417/#258 (core/dev version tests). This PR touches
none of those paths.

On rewriting an existing test. The repo convention is add a new test, do not
rewrite an existing one
. This PR adds a second it() and leaves #236's
assertion untouched. The original expect(version).toBe('1.3.0') was removed by
#236, under the documented exception that the assertion encoded wrong behaviour — it
pinned a value release-please is contractually required to change. No test was
skipped, disabled, weakened, or deleted here.

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.

$ npx vitest run --project unit:integrations
 ✓ |unit:integrations| integrations/test/version_test.ts (2 tests) 5ms
 Test Files  1 passed (1)
      Tests  2 passed (2)

Proof each test can fail. Every assertion was run against mutated source and
observed to fail; all mutations were reverted.

# Mutation Result
A version.ts and package.json both → 'not-a-version' consistency passes, shape fails — isolates this PR's delta
B version.ts'9.9.9' (manifest untouched) consistency fails, shape passes
C both files → 1.6.0 (simulates the next release bump) both pass, no test edit needed
D both files → 1.6.0-rc.1 (prerelease bump) both pass

Mutation A — the one that proves the new assertion carries signal #236 does not:

   ✓ version > should match the version declared in package.json 3ms
   × version > should be a well-formed semantic version string 11ms
 FAIL integrations/test/version_test.ts > version > should be a well-formed semantic version string
 AssertionError: expected 'not-a-version' to match /^\d+\.\d+\.\d+(-[\w.]+)?$/

Mutation B:

   × version > should match the version declared in package.json 14ms
   ✓ version > should be a well-formed semantic version string 1ms
 AssertionError: expected '9.9.9' to be '1.4.0' // Object.is equality

Mutations C and D are the anti-staleness evidence: the pre-fix test failed this
simulation by construction (it asserted a frozen literal), while the post-fix test
survives a normal bump and a prerelease bump with zero edits. Together they also
exercise both branches of the regex's optional prerelease group (absent in C,
present in D).

Manual End-to-End (E2E) Tests:

From the repository root, on this branch:

npm install
npm run build                                  # required: the vitest global setup
                                               # imports @google/adk via core/dist
npx vitest run --project unit:integrations     # 1 file, 2 tests, passing
npx prettier --check integrations/test/version_test.ts   # clean
npm run lint                                   # clean

To observe the original bug, check out main and run the same vitest command:
AssertionError: expected '1.5.0' to be '1.3.0' at version_test.ts:12.

Known pre-existing condition, not introduced here: npm run ts:check exits
non-zero on this branch. It reports 45 findings, and the set is byte-identical
with and without this commit (captured by stashing the change and diffing the two
outputs) — none of them are in integrations/test/**. They come from #236's base
commit, which is an older main. This PR neither adds to nor fixes them.

CI note: .github/workflows/validation.yaml triggers on pull_request against
main. Because this is a stacked PR whose base is fix/run-unit-integrations-vitest-project,
those jobs will not run here; the commands above were run locally on the exact pushed
commit instead.

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.

The consistency assertion pins version.ts to the sibling manifest, but
both files are rewritten from a single release-please computed value, so
a malformed version propagates to both and passes unnoticed. Assert the
shape independently so the exported constant is checked, not just its
agreement with package.json.

The prerelease branch of the pattern is deliberate: release-please's
generic updater writes whatever version it computes, and an -rc bump
must not make this test the next stale one.
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