Skip to content

Test: drop the npm install from the AgentLoader discovery integration fixture - #276

Open
AmaadMartin wants to merge 1 commit into
fix/flaky-install-bound-integration-suitesfrom
feat/trim-integration-test-npm-installs
Open

Test: drop the npm install from the AgentLoader discovery integration fixture#276
AmaadMartin wants to merge 1 commit into
fix/flaky-install-bound-integration-suitesfrom
feat/trim-integration-test-npm-installs

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 29, 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:

Stacked PR. Base is fix/flaky-install-bound-integration-suites (#218), not main.

Problem: The integration Vitest project runs 14 separate npm install invocations inside test fixture directories, each materialising ~600 packages, on all three OSes of the validation matrix. One of those 14 is pure waste: the AgentLoader discovery and loading integration suite in tests/integration/app_loader/app_loader_test.ts never spawns a subprocess. It constructs AgentLoader in-process and calls listApps() / listAgents() / getAppFile(). esbuild (inside AgentFile.load()) and Node both resolve @google/adk and @google/adk-devtools from the workspace-root node_modules, which npm workspaces populates with @google/adk -> ./core and @google/adk-devtools -> ./dev symlinks — the same physical targets the fixture's file: deps pointed at. Installing them again into the fixture buys nothing, costs ~600 package materialisations plus a full recursive teardown per CI job per OS, and additionally forces AgentLoader.preloadAgents() to stat every top-level entry of the fixture's own node_modules.

Solution: Delete the install, and with it tests/integration/app_loader/discovery/package.json — the manifest exists only to carry the file: deps, and it is the thing that makes npm install meaningful there. The fixture's four sources are untouched.

Deleting that manifest has one non-obvious consequence, which is the reason for the new assertion. getFileModuleType() (dev/src/utils/agent_loader.ts:574) delegates a .ts entry to getTypeFromPackageJson(), which walks up to the first package.json and returns ESM iff it declares "type": "module". Previously the walk stopped at the fixture's own manifest, which has no "type" field, so artifacts compiled to .cjs with format: 'cjs'. Now the walk continues past discovery/, app_loader/, integration/ and tests/ (none of which have a package.json) and reaches the repository root, which declares "type": "module" — so artifacts compile to .mjs with format: 'esm'. That path is not exotic; it is already exercised in CI by the agent_loader/import_meta_url and skills/script_js fixtures, both of which declare "type": "module".

linkProjectNodeModules() likewise now resolves to the repository-root node_modules instead of the fixture's, which is a strict superset.

The suite's assertions are unchanged in meaning: still 2 apps and 4 agents-or-apps. Neither the deleted package.json (not a JS entry point) nor the fixture node_modules (a directory with no app.* / agent.* inside it) ever contributed to those counts.

Scope notes:

  • No production source changes. Only tests/ is touched, and coverage.include in vitest.config.ts covers only core/src, dev/src and integrations/src, so coverage and its thresholds are unaffected.
  • No timeout is changed, in vitest.config.ts or anywhere else. The discovery beforeAll loses its explicit hook timeout only because its body is now a single synchronous constructor call; the afterAll keeps FIXTURE_HOOK_TIMEOUT_MS exactly as it was.
  • The remaining 13 fixtures keep their package.json and their install: getFileModuleType() reads their "type" field, and the build_setup suite exists precisely to prove the published package layout resolves from a consumer project.
  • No suppressions of any kind are added (@ts-expect-error, @ts-ignore, eslint-disable, any, as any) — none was needed.

Collision check (required by the contribution workflow, recorded here):

gh pr list --repo AmaadMartin/adk-js --state open --limit 100 --json number,title,headRefName
# then, for every one of the 100 open PRs:
gh pr diff <n> --repo AmaadMartin/adk-js --name-only | grep -i app_loader/discovery

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.

This is a test-infrastructure change: the integration suite is the test for it, and no unit test is added. A unit test that mocked child_process to assert an install did not happen would assert the mock, not the behaviour. The behavioural consequence is instead pinned by a real assertion inside the suite (see the mutation proof below).

Targeted runs only (never the whole repo suite), Linux, Node v22.22.0, on the exact commit pushed:

  1. Discovery suite, no fixture install and no network state. From a clean tree with the fixture's node_modules / package-lock.json removed:

    npx vitest run --project integration \
      tests/integration/app_loader/app_loader_test.ts -t 'AgentLoader discovery'
    

    Test Files 1 passed (1), Tests 3 passed | 3 skipped (6).

  2. All four install-bound suites (the four files Fix: stabilise install-bound and server-spawning integration suites #218 touches, to confirm nothing regressed underneath the stack):

    npx vitest run --project integration \
      tests/integration/app_loader/app_loader_test.ts \
      tests/integration/agent_loader/agent_dirname_test.ts \
      tests/integration/build_setup/build_setup_test.ts \
      tests/integration/skills/script_js/agent_test.ts
    

    Test Files 4 passed (4), Tests 30 passed | 4 skipped (34), Duration 480.92s. git status --porcelain clean afterwards.

  3. Static checks. npm run lint → clean. npm run format:checkAll matched files use Prettier code style!. npm run ts:check fails identically with and without this diff — I captured its full output on the stack base and on this commit and diff reports them byte-identical, so the failures are pre-existing (they are the subject of separate PRs) and none of them names a file in this diff.

Prove the test can fail (mutation). The new assertion is the regression guard for the whole change, so it was run against the unmutated pre-change state:

  • Mutation: restore tests/integration/app_loader/discovery/package.json with its original contents (git show HEAD:...), leaving everything else as-is.

  • Result — the new assertion FAILS:

    FAIL |integration| tests/integration/app_loader/app_loader_test.ts >
      AgentLoader discovery and loading integration >
      should load App from directory entrypoint and expose App and rootAgent
    AssertionError: expected '.cjs' to be '.mjs' // Object.is equality
     ❯ tests/integration/app_loader/app_loader_test.ts:105:51
    
  • Mutation reverted (rm the restored manifest) → Tests 3 passed, green again.

Measured effect (Linux, directional — not CI numbers):

Run Before After
app_loader_test.ts -t 'AgentLoader discovery' Duration 96.74s Duration 27.22s
app_loader_test.ts (whole file, 6 tests) Duration 302.93s Duration 225.59s

Both "before" figures were taken on the stack base (this change stashed) in the same session, so they already include #218's cheap-install flags; the delta is attributable to removing the install, not to the flags.

Manual End-to-End (E2E) Tests:

npm install && npm run build          # repository root
git status --porcelain                # -> empty
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts
                                      # -> Test Files 1 passed, Tests 6 passed
git status --porcelain                # -> still empty: the discovery fixture must
                                      #    NOT have regrown node_modules or a
                                      #    package-lock.json, and the three CLI
                                      #    fixtures must have cleaned theirs up
ls tests/integration/app_loader/discovery
                                      # -> service_alpha  service_beta
                                      #    standalone_agent.ts  standalone_app.ts

All five steps were run and produced exactly the output above.

CI status: absent — validated locally instead. validation.yaml triggers on pull_request: branches: [main], and this is a stacked PR whose base is fix/flaky-install-bound-integration-suites, so the run-tests matrix never fires (only auto-assign runs, which is not validation). Everything below was therefore run locally on the exact pushed commit 1f70b67e, with a clean working tree:

Command Result
npm run build pass
npm run lint pass, no output
npm run format:check All matched files use Prettier code style!
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts Test Files 1 passed (1), Tests 6 passed (6), Duration 247.67s
git status --porcelain afterwards empty
ls tests/integration/app_loader/discovery service_alpha service_beta standalone_agent.ts standalone_app.ts

npm run ts:check is the one red check, and it is red identically on the stack base — its output is byte-identical with and without this diff.

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.

…y fixture

The discovery suite never spawns a subprocess: it constructs AgentLoader
in-process and calls listApps()/listAgents()/getAppFile(). esbuild and Node
both resolve @google/adk and @google/adk-devtools from the workspace-root
node_modules, whose symlinks point at the same core/ and dev/ the fixture's
file: deps did, so materialising ~600 packages into the fixture was pure
waste on every CI job on every OS.

Deleting the fixture package.json is required to remove the install, and it
changes which manifest getTypeFromPackageJson() finds: the walk now reaches
the repository root, which declares "type": "module", so the fixture compiles
to .mjs/esm instead of .cjs/cjs. A new assertion on the compiled artifact's
extension pins that.

Removing the install also stops preloadAgents() stat-ing every top-level
entry of a fixture node_modules, and makes the suite runnable offline.
This was referenced Jul 30, 2026
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