Fix: spread the real module in three incomplete vi.mock factories - #875
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: spread the real module in three incomplete vi.mock factories#875AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
An inline vi.mock factory that omits an export the module graph under test uses does not fail loudly. Vitest throws only at property-access time, so a try/catch around the call site swallows the error and the test silently takes the fallback branch. An audit of all 35 vi.mock sites with a relative specifier found three factories that omit an export reachable through the code under test: guessMimeType from core/src/utils/file_utils.ts in the two skill-script suites, and five env_aware_utils exports in event_processor_utils_test.ts. A runtime probe showed none of the three is invoked today, so each one is a latent trap rather than a live defect. Spreading the real namespace and overriding only the stubbed export makes the factory incapable of going stale, and keeps the real implementation for every export the test does not control.
AmaadMartin
force-pushed
the
fix/vi-mock-factory-missing-exports
branch
from
August 9, 2026 16:48
7ef6400 to
fc717f5
Compare
AmaadMartin
changed the base branch from
fix/agent-loader-test-package-json-module-type
to
main
August 9, 2026 16:48
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
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem: An inline
vi.mock()factory that omits an export the code under test uses does not fail loudly. Vitest throws only when the missing key is accessed, so atry/catcharound the call site swallows the error and the test silently takes the fallback branch. The suite stays green and coverage still counts the lines as executed. An audit of everyvi.mocksite in the repo found four incomplete factories.Solution: Three of the four factories now spread the real module and override only the export the test controls, which is the form already used in six other suites. A spread factory cannot omit an export, because its key set is the module's own export list. The fourth site is fixed by sibling PR #778, which touches a different file.
Audit result
I wrote a throwaway checker and ran it from the repo root. It scans every
vi.mock('<relative path>', ...)site, resolves the specifier to its.tssource, walks the test's import graph (relative imports, barrel re-exports, and the@google/adk/@google/adk-integrationsVitest aliases), prunes the graph at any module the same test file also mocks, and compares the factory key set against the names that survive. The checker is not part of this change. It lives in/tmp, outside the working tree, and nothing in this diff references it.Totals on
aee56e07: 35 sites with a relative specifier — 27 inline-factory, 6 spread-actual, 2 automock. 0 stale keys, 0 unresolved specifiers, 0 unparsed factories. 4 flagged:dev/test/utils/agent_loader_test.ts:31dev/src/utils/file_utils.tsloadFileDatacore/test/tools/skills/run_skill_script_tool_test.ts:23core/src/utils/file_utils.tsguessMimeTypecore/test/tools/skills/run_skill_inline_script_tool_test.ts:25core/src/utils/file_utils.tsguessMimeTypecore/test/a2a/event_processor_utils_test.ts:23core/src/utils/env_aware_utils.tsgetBooleanEnvVar,isBrowser,base64Decode,base64Encode,isBase64EncodedThe other 31 sites are complete, spread-actual, or automocks. I report them so the audit is visible even though only one site was an active defect.
Active vs latent, and the probe behind each verdict
I classified each hit by running the code, not by reading it. I added the missing key to the factory as a counter that increments on call, plus an
afterAllthat asserts the counter is zero. A counter detects invocation even when atry/catchswallows a throw, which a throwing probe does not.Sites #2, #3 and #4: the counter stayed at zero, so the export is reachable through the barrel but nothing the suite exercises calls it. To prove the probe can detect a call at all, I ran a control that counted a known-invoked export instead. It failed as expected:
Site #1 is the active defect. Its probe is swallowed by the bare
catchingetTypeFromPackageJson, which is the reported failure mode. PR #778 fixes it.For a latent hit, adding the missing key as a bare
vi.fn()is the wrong fix: it replaces a real implementation with a silentundefined-returning stub, so a future call gets a wrong answer instead of an error. Spreading the real namespace preserves the real implementation and closes the hole permanently.Regression guard
Site #1 is pinned by behavioural tests in PR #778. For sites #2-#4 the structural change is the guard: with the real namespace spread in, there is no factory key left to delete, so the factory cannot go incomplete again. Reverting the spread is not "dropping an export", it is reverting the fix, and the checker detects that. With
...actualremoved from site #2 the checker flags it again:With the fix in place the tally moves to 24 inline-factory / 9 spread-actual / 2 automock. The checker reports one remaining flagged site on this branch, site #1, which PR #778 closes. With both branches applied it reports
0 flagged site(s).One checker note, since it changes a number. My first version classified a factory as spread-actual whenever it mentioned
importOriginal. That is a false negative: a factory can await the original and still return a literal without spreading it, which is exactly the mutation above. The checker now decides on the returned literal, not on the mention.Collision check
gh pr list --repo AmaadMartin/adk-js --state open --limit 100plusgh pr diff --name-onlyon every plausibly adjacent PR. Two overlap:dev/testand this PR three undercore/test, and neither depends on the other. They can merge in either order.run_skill_inline_script_tool_test.ts(vi.fn().mockImplementation(f)tovi.fn(f)). I kept that line verbatim inside the new spread factory, so the two changes compose.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.
This PR changes test setup only. It adds no new test, because the fix removes a failure mode by construction rather than adding behaviour. No
src/file is touched.Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
CI is green on this PR:
run-testspasses on ubuntu-latest, macos-latest and windows-latest, pluscheck-license. I also validated locally on the pushed commitfc717f5f:Two pre-existing failures, both present on
aee56e07before this change:dev/test/cli/cli_create_test.ts— "should handle Vertex AI selection with gcloud defaults". Fails identically with my change stashed.npm run ts:checkreports 292 errors repo-wide, the same 292 onaee56e07and on this branch. One of them,run_skill_script_tool_test.tsTS2352, sits in a file I touch, at line 209 before my edit and 214 after. I add zero errors and did not fix these, as they are outside this task.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
npm run build -w corenpx vitest run --project unit:core core/test/tools/skills core/test/a2a...actualfrom any of the three factories and addguessMimeType('x.png')to a test body. Before this change that call throwsNo "guessMimeType" export is defined on the mock; after it, it returnsimage/png.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.
[ ] I have added tests that prove my fix is effective or that my feature works. No new test: the fix removes the failure mode by construction, as described under "Regression guard".
[x] New and existing unit tests pass locally with my changes.