Skip to content

Fix: fail loudly when two agent_loader tests compile to the same fixture path - #958

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/agent-loader-test-unique-compiled-fixtures
Open

Fix: fail loudly when two agent_loader tests compile to the same fixture path#958
AmaadMartin wants to merge 1 commit into
mainfrom
fix/agent-loader-test-unique-compiled-fixtures

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):
    N/A
  2. Or, if no issue exists, describe the change:
    Problem: The AgentFile tests mock createTempDir to return one shared directory, so two tests whose agent files share a basename compile to the same absolute path. The module runner then hands the second import() the module the first test already evaluated. Seven tests collided on agent1.cjs and agent2.cjs, and four of them asserted against a module an earlier test had built. They pass only because the colliding fixtures hold identical content, so the assertions are decorative.

Solution: Every esbuild.build mock now goes through mockCompiledOutput, which claims the real options.outfile in a file-scoped Set and throws on a repeat. I renamed five fixture files, because the guard cannot pass while the existing collisions remain. No test title, assertion or expected string changed: the agent names come from the fixture content, not from the file name. dev/src/utils/agent_loader.ts is untouched, because its ?t= cache-buster is correct for the real Node ESM loader that adk run --reload_agents uses.

Renamed fixtures (source file name only): agent1_private_dir.js, agent1_disposed.js, agent2_cleanup.ts, agent1_as_app.js, agent1_cached.js.

The AgentLoader block also claims its outfile. That block is safe today only because its own beforeEach gives each load a fresh mkdtemp directory, so the guard keeps that property enforced.

Collision check: I listed all 300 open PRs on this fork and found 20 that touch dev/test/utils/agent_loader_test.ts (#918, #896, #892, #874, #861, #854, #832, #830, #815, #814, #793, #778, #755, #731, #727, #726, #725, #674, #664, #662). I grepped every one of those diffs for the guard. None adds it, so this does not duplicate a sibling. The overlap is line-level only, so this branch comes off main.

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.

One new test, rejects a second compile to a fixture name another test already used. It loads one source file through two AgentFile instances and asserts the second load() rejects with the guard's message.

Proof the tests can fail. I ran two mutations and reverted both.

  1. Deleted claimCompiledFixture(options.outfile); from mockCompiledOutput. The new test failed: AssertionError: promise resolved "FakeAgent1{ …(7), …(1) }" instead of rejecting.
  2. Put caches loaded agent instance back on agent1.js. That test failed: Compiled fixture 'agent1.cjs' was already produced by another test in this file. Reusing it makes this test import the earlier test's module instead of its own, so it passes or fails for the wrong reason - give this test's agent file a unique name. This proves the guard catches a real collision, not only the synthetic one.

Coverage: dev/test/** is outside the coverage include globs in vitest.config.ts, so the reported percentages do not move. Every added line runs. The twelve AgentFile compile sites and the whole AgentLoader block exercise the happy path, and the new test exercises the throw.

Manual End-to-End (E2E) Tests:
No runtime behaviour changes, so there is nothing to exercise by hand. I ran the local gate instead, on the pushed commit:

npx vitest run --project unit:dev dev/test/utils/agent_loader_test.ts   # 37 passed (36 before)
npm run lint                                                            # clean
npm run format:check                                                    # clean
npm run build -w core                                                   # clean

npm run ts:check reports 292 errors in this workspace, both with and without this change, and none in the changed file. It is not part of the CI validation workflow.

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.

…e path

The AgentFile block mocks createTempDir to return one shared directory, so two
tests whose agent files share a basename compile to the same absolute path. The
module runner then hands the second import() the module the first test already
evaluated. Seven tests were colliding on agent1.cjs and agent2.cjs.

Route every esbuild.build mock through mockCompiledOutput, which claims the
outfile in a file-scoped Set and throws on a repeat. Rename five fixtures so the
guard is satisfiable. No test title, assertion or expected string changes.
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