Skip to content

Fix: sweep the script_js integration fixture by name shape - #766

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/script-js-fixture-output-sweep
Open

Fix: sweep the script_js integration fixture by name shape#766
AmaadMartin wants to merge 2 commits into
mainfrom
fix/script-js-fixture-output-sweep

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 7, 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

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

Problem: The script_js integration teardown removes three exact filenames, but the skill write path de-duplicates: materializeFiles appends _2, _3, ... before the extension when the target name is taken (core/src/utils/file_utils.ts). A run that starts from a dirty fixture directory therefore writes index_2.html and friends, which the teardown never names and .gitignore never covers, so they pile up as untracked files. The same dirt hides a second defect: the content assertions read the three exact names, which on a dirty run are the previous run's files, so the suite stays green while it compares stale output.

Solution: The test now sweeps the fixture directory by name shape. isGeneratedOutput strips a trailing _<n> from the basename and compares the result against the three literal names, and removeGeneratedOutputs deletes every match. beforeAll sweeps before npm install and lets a failure reject, so the run always starts clean; afterAll replaces the three exact removals with one sweep. Sweeping first also fixes the stale comparison, because de-duplication can only trigger on a dirty directory.

Safety of the predicate: it strips a suffix rather than prefix-matching, so indexed.html is not a match. The tracked fixture entries agent.ts, agent_test.ts, model_responses.json, package.json and expected/ are all rejected — path.extname('expected') is '', so the comparison is 'expected' against the list and misses. fs.rm is called without recursive: true on purpose, so a future mis-match can never take a directory with it.

No existing assertion was modified, reworded or deleted. The diff is one file under tests/, which the Vitest coverage include list (core/src/**, dev/src/**, integrations/src/**) excludes, so the coverage thresholds cannot move.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open plus a scan of every branch touching this file. #662 rewrites the same afterAll to report each removal failure, and #730, #409, #523 change materializeFiles; none of them sweeps by name shape. This PR branches from main and keeps the .catch(() => {}) teardown style byte-identical so it stays separable from #662.

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.

The change adds one assertion to the existing integration test, which runs the two new helpers on every execution. Command: npx vitest run --project integration tests/integration/skills/script_js/agent_test.ts — 1 passed.

Manual End-to-End (E2E) Tests:

A. Clean run — passes, git status --porcelain tests/integration/skills/script_js prints nothing.

B. Dirty run (the reported bug). Seed a leftover and an orphan variant, then run:

cp tests/integration/skills/script_js/expected/index.html tests/integration/skills/script_js/index.html
printf 'stale\n' > tests/integration/skills/script_js/sketch_5.js
npx vitest run --project integration tests/integration/skills/script_js/agent_test.ts

Before (git status --porcelain tests/integration/skills/script_js):

?? tests/integration/skills/script_js/index.html
?? tests/integration/skills/script_js/sketch_5.js

After: 1 passed, and git status --porcelain tests/integration/skills/script_js prints nothing.

C. Back-to-back runs — both pass, tree clean after each.

D. Proof that the change can fail.

Against the unfixed code. git stash, then scenario B. The suite still passes, because it compares the stale index.html, and leaks:

?? tests/integration/skills/script_js/index_2.html
?? tests/integration/skills/script_js/sketch_5.js

Against the new assertion. With the fix applied, delete await removeGeneratedOutputs(); from beforeAll and run scenario B. The new assertion fails:

AssertionError: expected [ 'ephemeral_entanglement.md', ...(4) ] to deeply equal [ 'ephemeral_entanglement.md', ...(2) ]
- Expected
+ Received
  [
    "ephemeral_entanglement.md",
    "index.html",
+   "index_2.html",
    "sketch.js",
+   "sketch_5.js",
  ]

E. Static gates. npx eslint tests/integration/skills/script_js/agent_test.ts and npx prettier --check ... both pass. npm run ts:check output is byte-identical with and without this change, so it adds no type error. npm run build succeeds.

Note on the local hook timeout. On a cold npm cache the fixture npm install takes about 68 seconds on my machine, over the file's own 60 s beforeAll budget, so the hook times out before the test body runs. This is pre-existing and unrelated: it reproduces on unmodified main, and #662 already proposes letting the hook inherit the project's 120 s hookTimeout. I pre-installed the fixture dependencies before each run above so the hook fits its budget.

Note on CI. run-tests is green on ubuntu-latest, windows-latest and macos-latest, and tests/integration/skills/script_js/agent_test.ts passed on all three platforms in every run. The Windows leg is flaky for reasons unrelated to this change: across four runs it failed three times, each in a different test this diff does not touch — a2a/stream/stream_test.ts ("CLI exited prematurely"), app_loader/app_loader_test.ts, and core/test/code_executors/unsafe_local_code_executor_test.ts (a 5 s unit-test timeout). Each cleared on a re-run. The same workflow fails on unmodified main (9360bf2) on macOS in app_loader_test.ts, and sibling PR #765 fails on Windows in the same test, so the flake is repo-wide.

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.

Amaad Martin added 2 commits August 7, 2026 02:34
The skill write path de-duplicates against existing files: it appends
`_2`, `_3`, ... before the extension when the target name is taken. A
teardown keyed on the three exact names therefore leaves every variant
behind, and those variants are not gitignored.

Sweep the fixture directory by name shape instead, in `beforeAll` and in
`afterAll`. Sweeping first also stops the content assertions comparing a
previous run's output, which is what kept the leak invisible.
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