Skip to content

Fix: drop the dead nodejs-vertexai mock from the VertexAiSessionService tests - #563

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/remove-dead-nodejs-vertexai-mock
Open

Fix: drop the dead nodejs-vertexai mock from the VertexAiSessionService tests#563
AmaadMartin wants to merge 1 commit into
mainfrom
fix/remove-dead-nodejs-vertexai-mock

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):
    No existing issue. The block being removed was introduced with the file itself in Add support for Agent Engine Sessions google/adk-js#249 ("Add support for Agent Engine Sessions").

  2. Or, if no issue exists, describe the change:
    Problem: core/test/sessions/vertex_ai_session_service_test.ts opened with a vi.mock('nodejs-vertexai', ...) factory carrying the comment // Mock the unreleased nodejs-vertexai package so the import resolves. That premise is false — there is no import to resolve:

  • nodejs-vertexai is declared nowhere. Not in the root package.json, not in core/package.json, not in package-lock.json, and no file under core/, dev/, integrations/ or tests/ imports it. Before this change a repo-wide search returned exactly two hits, both in this one file: the comment and the vi.mock call.
  • The mocked symbol SessionsClient had exactly one reference in the whole repo — its own declaration inside that factory. Nothing constructed or imported it.
  • The suite actually exercises the real Vertex AI surface from a genuine dependency: it imports Sessions from @google-cloud/vertexai, which core/package.json declares as "@google-cloud/vertexai": "^1.12.0", and that is the same package core/src/sessions/vertex_ai_session_service.ts imports Client, Sessions and its request/response types from.

The net effect was a mock factory registered for a specifier that is never resolved, so the factory body never executed. It was inert but actively misleading: a reader would reasonably conclude the suite depends on an unreleased Vertex AI client.

Solution: Delete the comment and the vi.mock factory — 11 lines, one file, 0 insertions. Why this rather than anything larger: the block is dead, so deletion is the whole fix; no production code, no package.json, no config, and no other mock, import, fixture or assertion is touched. The 57 existing test cases are the regression signal and are left byte-identical.

vi stays imported deliberately. It is still used by vi.fn() in the beforeEach client stub, by vi.useFakeTimers() / vi.runAllTimersAsync() / vi.useRealTimers(), and by six vi.spyOn(...) call sites. Dropping it would trip @typescript-eslint/no-unused-vars.

The two import groups keep their existing contents and order; only the now-redundant second blank line between them goes with the block (leaving both blank lines fails npm run format:check under prettier-plugin-organize-imports).

Collision check (run before any code was written): gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 across all open PRs, filtered for vertex/mock/dead/remove in titles and branch names, then gh pr diff --name-only on every candidate touching or adjacent to this file (#503, #287, #270, #201, #474, #303, #475, #439, #515, #274, #512, #460). Result:

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.

No new tests, deliberately. This removes dead code and adds zero executable lines, so the coverage requirement is satisfied vacuously — and a test asserting the absence of a mock would be noise. No existing test was added, removed, renamed, retitled or restructured; the 57 cases are unchanged in name, order and count.

[x] All unit tests pass locally.

# before the edit (mock present)
npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts
  Test Files  1 passed (1) | Tests  57 passed (57)

# after the edit
npx vitest run --project unit:core core/test/sessions/vertex_ai_session_service_test.ts
  Test Files  1 passed (1) | Tests  57 passed (57)

npm run lint          # eslint "**/*.ts" — clean, exit 0
npm run format:check  # All matched files use Prettier code style! — exit 0
npm run build         # clean

npm run ts:check reports Found 281 errors in 41 files both with and without this change (checked by restoring the file from the parent commit and re-running) — a pre-existing repo-wide condition in the test tree that this diff neither adds to nor relieves. It is not one of the gates in .github/workflows/validation.yaml.

Full unit:core project: Test Files 1 failed | 167 passed (168), Tests 3 failed | 2348 passed (2351). The 3 failures are all in core/test/code_executors/unsafe_local_code_executor_test.ts and are pre-existing and unrelated — verified by git stashing this change and re-running that file against the untouched base, which reproduces the identical 3 failures (Tests 3 failed | 15 passed (18)).

Proof the deleted code was genuinely dead. There is no new assertion to falsify, so the standard mutation check does not apply. The equivalent proof is a positive one, and I ran it rather than merely asserting it:

  1. Deadness probe — reinstated the mock with a factory body that throws (vi.mock('nodejs-vertexai', () => { throw new Error('DEADNESS PROBE: the nodejs-vertexai factory executed'); })) and re-ran the suite. Result: Test Files 1 passed (1) | Tests 57 passed (57). The throw never fired, so the factory body never executed.
  2. Control probe — to show that probe has signal and is not just Vitest swallowing factory errors, applied the same throwing factory to @google/genai, a specifier this file does import. Result: Test Files 1 failed (1) | Tests no tests, with Caused by: Error: CONTROL PROBE: an imported specifier factory executed.

A throwing factory on an imported specifier detonates the file; the same throwing factory on nodejs-vertexai changes nothing. That is direct evidence the removed block was unreachable.

Static postconditions, all verified after the edit:

grep -rn "nodejs-vertexai\|SessionsClient" core dev integrations tests package.json package-lock.json
  # (no output — zero hits repo-wide, down from 3)
git diff --stat
  # 1 file changed, 11 deletions(-)   [file: 1186 -> 1175 lines]

Manual End-to-End (E2E) Tests:
Not applicable — this is a test-only deletion with no runtime behaviour, no public API surface, and no code path to exercise. The equivalent manual verification is the static audit above plus the two CI-mirroring checks (npm run lint, npm run format:check), both clean. Per the change's scope, the integration and e2e projects were not run: nothing under tests/integration/ or tests/e2e/ references this mock.

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.

…ervice tests

The test file opened with a vi.mock('nodejs-vertexai', ...) factory and the
comment "Mock the unreleased nodejs-vertexai package so the import resolves".
No such package is declared in any package.json or the lockfile, nothing in the
tree imports it, and the mocked SessionsClient symbol was referenced only by its
own declaration inside that factory. The suite exercises the real
@google-cloud/vertexai Sessions surface instead, so Vitest registered a factory
for a specifier that is never resolved and the factory body never ran.

The vi import stays: it is still used by vi.fn in the client stub, the fake
timer helpers, and six vi.spyOn call sites.

Test count is unchanged at 57 passing in 1 file.
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