Test: cover CJS/ESM module-type resolution in the dev AgentLoader - #778
Open
AmaadMartin wants to merge 1 commit into
Open
Test: cover CJS/ESM module-type resolution in the dev AgentLoader#778AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
The vi.mock factory for file_utils omitted loadFileData, so every read of that export threw inside getTypeFromPackageJson. The bare catch there turned the error into FileModuleType.CJS, which made the ESM branch and the parent-directory walk dead code in this suite. Complete the mock factory, give isFileExists and loadFileData faithful filesystem implementations, and add five tests that pin each branch of module-type resolution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A
Problem:
dev/test/utils/agent_loader_test.tsmocksfile_utils.jswith a factory that omitsloadFileData, one of the seven exportsagent_loader.tsimports. Vitest throws when the code under test reads that missing key, and the barecatchingetTypeFromPackageJson()turns the throw intoFileModuleType.CJS. The suite therefore never evaluated thepackageJson.type === 'module'comparison, never produced an.mjsoutfile orformat: 'esm', and never walked up to a parent directory.isFileExistswas also mocked to() => true, so every directory looked like it held apackage.json.Solution: I added
loadFileDatato the mock factory and gaveisFileExistsandloadFileDataimplementations that read the real fixture tree, matching howisFile,isFolderExistsandremoveFolderare already mocked in the samebeforeEach. Five new tests pin each branch of module-type resolution. No production file changes: the loader is correct, the harness was not.Evidence for the claim above. I added a probe to the
catchingetTypeFromPackageJsonon the base commit and ran the suite. The probe fired 36 times, and every message was[vitest] No "loadFileData" export is defined on the "../../src/utils/file_utils.js" mock.— never a realpackage.jsonproblem. After the change the probe fires once, on the malformed-package.jsontest, with a genuineSyntaxError.Collision check. I listed the open PRs on the fork and read the file lists of the 14 that touch
agent_loader,AgentFileorfile_utils(#755, #733, #731, #726, #725, #708, #703, #674, #664, #662, #653, #633, #511, #480, #457). None addsloadFileDatato the mock factory and none covers module-type resolution. #653 passesmoduleType: FileModuleType.ESMexplicitly, which short-circuits the detection this PR covers. Twelve of them edit the same test file for unrelated reasons, so expect textual conflicts, but no duplicated work.Testing Plan
Unit Tests:
npx vitest run --project unit:dev dev/test/utils/agent_loader_test.tsreports 36 passed, up from 31. The 31 existing tests keep their exact assertions.Mutation proof. I broke
getTypeFromPackageJsonsix ways and re-ran the suite each time.type === 'module'ternary ->return FileModuleType.CJS;return FileModuleType.ESM;AgentFileteststry/catch, leaving theloadFileDatacall barepackage.jsontestreturn getTypeFromPackageJson(parentDir);->return FileModuleType.CJS;return FileModuleType.ESM;loadFileDatafrom the mock factory againMutA failure message:
MutB is the answer to whether the harness repair matters for the existing tests. Before this change those 12 tests passed under every mutation of that comparison, because they never reached it.
Coverage. New lines are test code.
dev/src/utils/agent_loader.ts, measured from this suite alone, goes from 82.26% statements / 82.40% branches to 84.18% / 85.38%. Every statement and branch ingetTypeFromPackageJson(agent_loader.ts:589-612) is now executed; the ESM arm runs twice and thecatchand the root stop run once each.Manual End-to-End (E2E) Tests:
From the repo root on this branch:
The one failure in the full
unit:devrun iscli_create_test.ts > createAgent > Interactive Mode > should handle Vertex AI selection with gcloud defaults. It fails the same way on a clean checkout of the base commit with this change stashed, because this machine hasgclouddefaults configured. It is unrelated to this PR and I did not touch it.npx tsc --noEmitreports 287 errors on this branch and 287 on the base commit, none of them indev/test/utils/agent_loader_test.ts. Those pre-existing errors are being cleared by other PRs.CI result. All three matrix legs ran this file and logged
unit:dev dev/test/utils/agent_loader_test.ts (36 tests)passing.run-tests (ubuntu-latest)is green.run-tests (macos-latest)fails ontests/integration/app_loader/app_loader_test.ts, which times out at 40000ms;run-tests (windows-latest)was then cancelled by the matrixfail-fast. That macOS timeout also happens on an unmodified copy of the base commit (run 31170814985, same job, same test, same timeout), so this PR did not cause it. #664 tracks the fix, and I left it alone.Checklist