Skip to content

Chore: prefix Node built-in imports with node: across tests/e2e - #551

Open
AmaadMartin wants to merge 1 commit into
mainfrom
feat/node-protocol-prefix-tests-e2e
Open

Chore: prefix Node built-in imports with node: across tests/e2e#551
AmaadMartin wants to merge 1 commit into
mainfrom
feat/node-protocol-prefix-tests-e2e

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 2, 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):
    N/A — no public issue tracks this cleanup.

⚠️ Duplicate disclosure — read before reviewing. The collision check below
found that open PR #547 already contains this exact change, byte for byte,
as part of a wider sweep. This PR was opened anyway on explicit review-gate
instruction, not because a gap was found. If #547 is going to merge, close
this PR as a duplicate — nothing here is unique to it.
Details in
Collision check below.

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

Problem: 43 import statements across 20 files under tests/e2e/ import Node
built-ins with bare specifiers (fs, path, http, url). All four are real,
squattable package names on npm, so a same-named package installed into
node_modules can shadow the core module. The rest of the repository already
uses the node: prefix, which resolves unambiguously to the built-in and cannot
be shadowed — including one file inside tests/e2e itself
(tests/e2e/tools/mcp/load_mcp_resource_e2e_test.ts:13), which made the
directory internally inconsistent.

Solution: Prefix all 43 specifiers with node:. Breakdown: fs ×20,
path ×20, url ×2, http ×1. Nothing else changes — no binding is renamed, no
import form changes (import * as X stays import * as X, named imports stay
named), no test logic, no file added or deleted. The diff is exactly
+43 / −43 across 20 files, and 0 non-import lines changed (verified
mechanically, command below).

Two notes for reviewers:

  • One line is relocated, not hand-edited. In
    tests/e2e/tools/rest_api_tool_auth_e2e_test.ts,
    import {OpenAPIV3} from 'openapi-types'; moves below
    import * as path from 'node:path';. That is
    prettier-plugin-organize-imports re-sorting: node:* now sorts before
    openapi-types, where bare path sorted after it. It is the output of
    npm run format, not a manual change. So the diff is "43 one-line changes plus
    one relocated line", not 43 clean one-liners.
  • No lint rule was added. Enforcing this via n/prefer-node-protocol or
    similar is a separate, repo-wide change to CI and is deliberately out of scope
    here (other open PRs attempt it; this one does not touch eslint.config.js,
    package.json, or the lockfile).

Collision check (run before any code was written, per contributing workflow):

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000

This surfaced several adjacent PRs. Three of them contain this same tests/e2e
sweep:

PR Branch Relationship to this PR
#547 fix/node-protocol-builtin-imports Strict superset — contains this change byte-identically, plus core/, dev/src/cli/cli.ts, integrations/build.js, vitest.config.ts
#422 feat/enforce-node-protocol-lint-rule Same 20-file sweep plus an ESLint enforcement rule and lockfile churn
#346 feat/lint-enforce-node-protocol-imports Same 20-file sweep plus an ESLint enforcement rule and lockfile churn

The overlap with #547 was verified at tree level, not by eyeballing hunks: for
every tests/e2e/**/*.ts file I applied the mechanical rewrite
s/ from '(fs|path|http|url)';/ from 'node:$1';/ to the base version and diffed
the result against #547's version of the same file. Every file is byte-identical
except rest_api_tool_auth_e2e_test.ts, where the only difference is the
openapi-types relocation described above — which #547 has also already applied.

The residual delta of this PR over #547 is therefore zero bytes. It is not a
stack candidate either, since there is nothing left to stack. Recommended
disposition: merge #547 and close this PR and #422/#346 as duplicates of it.

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.

No tests were added, deliberately. This change adds no code paths, branches,
or functions — it rewrites 43 import specifiers. A test asserting on import
specifiers would test the TypeScript compiler and the Node resolver, not this
project. Verification is therefore static plus a runtime load check.

Unit Tests:
[x] I have added or updated unit tests for my change. — N/A, and stated
explicitly rather than silently skipped:
there are zero new executable
lines, so the new-line-coverage bar is met vacuously. No existing test was
modified, skipped, weakened, or deleted.
[x] All unit tests pass locally. — see the type-check equivalence below; the
full unit suite was not run, as no non-test source file is touched.

Verification performed (Node v22.22.2, npm 9.2.0):

  1. Pre-edit safety scan. A specifier rename can only change behaviour if
    something mocks the bare specifier, since Vitest keys its mock registry on the
    specifier as written and fs / node:fs are not guaranteed to alias.
    grep -rn "vi\.mock\|vi\.doMock\|jest\.mock\|vi\.importActual" tests/e2e --include='*.ts'
    0 matches. Also 0 require('fs')-style or dynamic import('fs')-style
    calls of built-ins. Every affected import is a static ESM import.
  2. npx tsc --noEmit — no new errors, proven by differential run. The
    checkout reports 281 pre-existing errors (core/dist vs core/src type
    identity clashes after a build, plus pre-existing test typing issues). I ran
    the check on the pristine base and on this branch, stripped ANSI codes, sorted
    the error lines, and diffed them: identical sets, 281 before and 281 after.
    Errors located in tests/e2e: 0. Errors of the form
    Cannot find module 'node:…': 0.
  3. npm run lint → exit 0, clean.
  4. npm run format → applied the one openapi-types relocation noted above,
    and changed no file outside the 20.
  5. npm run format:check → exit 0, All matched files use Prettier code
    style!
  6. npm run build → exit 0.
  7. Postconditions.
    • grep -rn "from '\(fs\|path\|http\|url\)'" tests/e2e --include='*.ts'
      no matches (was 43 before the edit).
    • Repo-wide from 'node:…' count in *.ts rose 130 → 173 (= 130 + 43).
    • git diff --stat20 files, +43 −43, all under tests/e2e/; 0 files
      outside it.
    • git diff -U0 -- tests/e2e | grep '^[+-]' | grep -v '^[+-][+-]' | grep -cv "^[+-]import"
      0, i.e. no non-import line changed anywhere.

Mutation / can-this-fail check. There is no unit test here to mutate, so the
equivalent falsifiable check is the completion grep, run in both directions:
before the edit it returned 43 matches across 20 files; after the edit it
returns 0.
Had the rewrite silently failed or missed files, that grep would
still report the leftovers — it is the check that can fail. Both counts are
recorded above.

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

The e2e Vitest project targets live Gemini / Vertex AI endpoints and needs
credentials, so it cannot be run to green in a credential-free environment. It
was still used as a runtime import-resolution proof, which is the property
this change could actually break:

npx vitest run --project e2e \
  tests/e2e/tools/rest_api_tool_auth_e2e_test.ts \
  tests/e2e/custom_metadata/custom_metadata_test.ts \
  tests/e2e/routing/model_fallback_test.ts

All three files collect and execute; they fail only later, inside model
construction, with API key must be provided via constructor or GOOGLE_GENAI_API_KEY or GEMINI_API_KEY environment variable. Crucially, the
identical command on the unmodified base produces the identical three failures
with the identical cause
, so the behaviour is unchanged and the failure is
purely the missing credential. Reaching model construction at all proves the
rewritten node: specifiers resolve under the real ESM loader.

To reproduce with credentials: export GOOGLE_GENAI_API_KEY (or
GEMINI_API_KEY) and re-run the command above; it exercises the same files this
PR touches.

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. — no
comments were added or needed; the change is 43 import specifiers.
[x] I have added tests that prove my fix is effective or that my feature works.
— N/A with reasoning given above (no new executable code); the falsifiable
check is the before/after completion grep, 43 → 0.
[x] New and existing unit tests pass locally with my changes. — type-check error
set is byte-identical to the base (281 → 281, 0 in tests/e2e), lint and
format:check clean, and the touched e2e files behave identically to base.

CI status

All checks green on commit dc3a3f75:

Job Result
run-tests pass
run-tests (ubuntu-latest) pass
run-tests (macos-latest) pass
run-tests (windows-latest) pass
check-license, auto-assign pass (not validation)

Getting there took re-runs, and the reason is worth recording for whoever sees this
history: two unrelated pre-existing flakes, both confirmed as flakes because the very
same commit went green with zero code changes.

  • core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout failed three times on windows-latest with Test timed out in 5000ms, once recorded at 5038ms — over the limit by 38ms, while 2679 other tests
    passed. It spawns a real PowerShell process, whose cold start is slow and variable on a
    loaded runner (commit b56761bf had already added -NoProfile to speed this up). It
    passed on the fourth run.
  • tests/integration/app_loader/app_loader_test.ts timed out once on macos-latest
    (40000ms) and passed on re-run.

Neither can be affected by this PR: both live in Vitest projects (unit:core,
integration) whose include globs are core/test/** and tests/integration/**, while
every file here is in the separate e2e project (tests/e2e/**), and neither test
imports anything from this diff.

No test was skipped, weakened, or given a longer timeout to achieve this. The Windows
timeout is filed as separate follow-up work (suggested fix: an explicit generous timeout
on that one test, matching other process-spawning tests — not skipping it, since it covers
real shell execution).

This PR's own files are reported by CI as ↓ … 1 skipped: collected and loaded, which
proves the rewritten node: specifiers resolve, then self-skipped for lack of model
credentials.

Rewrite 43 import statements in 20 files under tests/e2e so every Node
built-in specifier carries the node: prefix (fs x20, path x20, url x2,
http x1).

The prefix makes the specifier resolve unambiguously to the Node core
module, so a same-named package in node_modules cannot shadow it --
fs, path, http and url are all real, squattable names on npm.

This is a zero-behavior-change edit: no binding is renamed and no import
form changes. prettier-plugin-organize-imports relocated one line in
tests/e2e/tools/rest_api_tool_auth_e2e_test.ts, because node:* now sorts
before openapi-types.
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