Skip to content

Test: pin the integrations web entry point to the node public surface (stacked on #236) - #513

Open
AmaadMartin wants to merge 1 commit into
fix/run-unit-integrations-vitest-projectfrom
fix/wire-unit-integrations-vitest-project
Open

Test: pin the integrations web entry point to the node public surface (stacked on #236)#513
AmaadMartin wants to merge 1 commit into
fix/run-unit-integrations-vitest-projectfrom
fix/wire-unit-integrations-vitest-project

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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Stacked on #236 (fix/run-unit-integrations-vitest-project). This PR targets that
branch, not main, and its diff is a single new test file.

Collision check (run before any code was written).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returned 413 open PRs.
The task this branch was created for — "wire the orphaned unit:integrations Vitest
project into the test scripts" — is already implemented by open PR #236, whose diff
adds --project unit:integrations to test, test:unit and test:coverage and replaces
the rotted expect(version).toBe('1.3.0') literal with a comparison against the package
manifest. Two further attempts at the same work are already closed (#245, #476), and three
PRs are open on top of #236: #479 (hardens the same version_test.ts), #343 (CI guard for
projects run by no npm script) and #311 (wildcard unit-project selection).

A competing wiring PR was therefore not opened. What follows is the one piece of the
original task that #236 does not cover, stacked on it as the overlap rule prescribes.

Problem: integrations/src/index_web.ts is the declared browser entry point in
integrations/package.json, but nothing imports it — not the node entry, not the existing
test. Once #236 makes the unit:integrations project actually run, integrations/src
lands at 66.66% with index_web.ts at a flat 0%:

 integrations/src  |   66.66 |        0 |       0 |   66.66 |
  index.ts         |     100 |     100 |     100 |     100 |
  index_web.ts     |       0 |       0 |       0 |       0 | 1-7
  version.ts       |     100 |     100 |     100 |     100 |

The gap is not just a number. index.ts and index_web.ts are two hand-maintained copies
of the same export list, and nothing anywhere asserts they agree — the web bundle can lose
an export and no test notices. That is the same class of silent rot that let
version_test.ts sit on a false assertion across two releases.

Solution: one test, integrations/test/index_web_test.ts, asserting that the web entry
exposes the same export names as the node entry. It closes the coverage gap and pins a real
invariant at the same time, so the coverage is a by-product of a meaningful assertion rather
than an import written to move a percentage.

Two deliberate choices, both worth calling out:

  • Relative ../src/index.js / ../src/index_web.js imports instead of the
    @google/adk-integrations package name.
    The repo guideline is to import via the public
    entry point, and it does not apply here: the Vitest alias
    (vitest.config.ts) maps @google/adk-integrations to integrations/src, which resolves
    to index.ts only. There is no aliased specifier for the web entry, and comparing the
    node entry against itself would assert nothing. Both entries must be reached by path for
    the comparison to mean anything.
  • expect(nodeExports).not.toHaveLength(0) before the parity assertion. Without it the
    test passes vacuously if both entries lose their exports — [] equals []. Given this
    whole task exists because a test quietly stopped meaning anything, a self-check against
    the degenerate case is worth one line. Mutation 2 below demonstrates it fires.

No production source changed; git diff against the base is exactly one added file.

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.

Environment: unset GOOGLE_CLOUD_PROJECT GOOGLE_CLOUD_LOCATION, then npm install and
npm run build (the build is mandatory — globalSetup resolves @google/adk through
core/dist, so an unbuilt tree fails with Failed to resolve entry for package "@google/adk"
and the misleading No test files found).

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/index_web_test.ts (1 test) 7ms
 ✓ |unit:integrations| integrations/test/version_test.ts (1 test) 6ms
 Test Files  2 passed (2)
      Tests  2 passed (2)

Confirming the stacked wiring actually reaches the new file (not just a direct
--project invocation):

$ npm run test:unit -- --run --reporter=verbose | grep 'unit:integrations'
 ✓ |unit:integrations| integrations/test/version_test.ts > version > should match the version declared in package.json 4ms
 ✓ |unit:integrations| integrations/test/index_web_test.ts > index_web > exposes the same public surface as the node entry point 10ms

Coverage — integrations/src, measured on this checkout:

metric before (base = #236) after Δ
statements 66.66 100 +33.34
branches 0 100 +100
functions 0 100 +100
lines 66.66 100 +33.34

Per file after: index.ts 100%, index_web.ts 100% (was 0%, lines 1-7 uncovered),
version.ts 100%.

Threshold decision: vitest.config.ts thresholds left unchanged. The configured
values (86/87/88/86) are minimums, and coverage.include is untouched by this PR — no
denominator grows, so no configured metric can fall. The change only adds covered lines in a
three-file directory, so the repo-wide movement is a rounding-level increase and nothing
approaches an integer boundary near the configured values. Ratcheting the thresholds to
today's exact numbers is a separate policy change with real downside (unrelated PRs go red on
rounding-level movement) and is out of scope here; #377 and #324 are the PRs already looking
at that question.

Proof each assertion can fail (both mutations reverted; git status clean afterwards):

  1. Parity assertion. Appended export const webOnly = true; to
    integrations/src/index_web.ts only:

    FAIL  |unit:integrations| integrations/test/index_web_test.ts > index_web > exposes the same public surface as the node entry point
    AssertionError: expected [ 'version', 'webOnly' ] to deeply equal [ 'version' ]
     ❯ integrations/test/index_web_test.ts:15:42
    
  2. Vacuous-pass guard. Replaced export {version} from './version.js'; with export {};
    in both index.ts and index_web.ts — the case where the parity assertion alone
    would pass on [] equals []:

    FAIL  |unit:integrations| integrations/test/index_web_test.ts > index_web > exposes the same public surface as the node entry point
    AssertionError: expected [] to not have a length of +0
     ❯ integrations/test/index_web_test.ts:14:29
    

Manual End-to-End (E2E) Tests:

unset GOOGLE_CLOUD_PROJECT GOOGLE_CLOUD_LOCATION
npm install
npm run build                                 # required before any vitest run
npx vitest run --project unit:integrations    # expect 2 files / 2 tests passing
npx vitest run --project unit:integrations --coverage   # expect integrations/src at 100%
npm run lint                                  # clean
npm run format:check                          # "All matched files use Prettier code style!"

npx tsc --noEmit on the new file with the root compiler options
(--strict --module nodenext --moduleResolution nodenext --target ES2020 --lib ES2022,DOM --skipLibCheck) exits 0. No any, no cast, no @ts-expect-error, no eslint-disable
anywhere in the diff.

CI note: because this PR is stacked (base is fix/run-unit-integrations-vitest-project,
not main), the validation workflow — which triggers on pull_request to main — does
not run. All validation above was performed locally on the exact pushed commit.

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.

integrations/src/index_web.ts is the declared browser entry in
integrations/package.json but is imported by nothing, so it sat at 0%
once the unit:integrations project started running. The two entry
points can drift independently; this asserts they export the same
names, taking integrations/src from 66.66% to 100%.
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Heads-up: this PR is now a no-op against its own base.

A correctness review on #236 — the branch this PR is stacked on — blocked it for leaving integrations/src/index_web.ts at 0% coverage, on the grounds that the index/index_web export-parity invariant stays unguarded regardless of which PR is nominally responsible for it. integrations/test/index_web_test.ts has therefore been added directly to #236 (commit 940babf5b), so once #236 lands this PR's diff against main is empty.

The implementation that landed keeps this PR's expect(nodeExports).not.toHaveLength(0) guard against a vacuous pass, and adds a second assertion pinning the version binding as well as the export names — the two entry points can agree on key sets while disagreeing on the value behind them, which the key-set assertion alone does not catch.

Closing this is your call; flagging it rather than doing it from #236.

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