Test: build real InvocationContexts in the skill-script integration fixtures - #835
Open
AmaadMartin wants to merge 1 commit into
Open
Test: build real InvocationContexts in the skill-script integration fixtures#835AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
…ixtures Both fixtures cast an object literal through `as unknown as InvocationContext`, which turns off structural checking for everything the tool under test reads off the context. Build the real graph instead: a real LlmAgent, Session, PluginManager and InvocationContext, all from the @google/adk public entry point. No test name, assertion or count changes.
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: The
createMockContexthelper in both skill-script integration test files casts an object literal throughas unknown as InvocationContext. That double cast turns off structural checking, so the compiler cannot see that the fake omitsinvocationIdandpluginManager, which the tool under test reads. IfRunSkillScriptToolstarts using a field the fake lacks, the suite fails at runtime with aTypeErrorinstead of failing to compile.Solution: Both helpers now build the real object graph from the
@google/adkpublic entry point: a realLlmAgent,Session,PluginManagerandInvocationContext. This follows the ADK JS guideline "Use Factory Functions in Tests" and copies the construction pattern incore/test/tools/agent_tool_test.ts. No production source, test name, assertion, or test count changes.The three newly-real fields are inert here.
invocationIdonly keys thegetOrFetchSkillcache, which returnsthis.skills[name]before reading the cache for statically registered skills.UnsafeLocalCodeExecutorreads nothing offinvocationContext, so theprocess.cwd()assertions are unaffected. TheisLlmAgent(agent) -> agent.codeExecutorfallback stays unreachable because both suites always pass{codeExecutor: executor}.Collision check: I listed the 300 open PRs on the fork and read the diffs of every PR that touches these two files (#634, #564, #556) plus the adjacent context-fixture PRs (#748, #750, #753). None replaces this cast. #564 removes an identical cast in
core/test/utils/artifact_utils_test.ts, a different file. #556 and #634 edit test bodies in these files but not the helper, so this PR branches frommainrather than stacking.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 change adds no test. It rewrites two fixture helpers, so the acceptance evidence is the unchanged test inventory plus a clean type check.
The 4 skips are the Windows-only PowerShell and cmd cases (
it.skipIf(!IS_WINDOWS)).Proof the fixture can fail. For a suppression removal the mutation is a compile-time one. I deleted
pluginManager: new PluginManager([]),fromcreateMockContextand rannpx tsc --noEmit:The old cast suppressed exactly this error. I restored the line afterwards.
tsc --noEmithas a dirty baseline onmain: 771 error lines across pre-existing files, none in these two. I captured the full output before and after this change anddiffreports them byte-identical, so this PR adds no type error.Also clean:
npx prettier --checkandnpx eslinton both files, andgrep -rn "as unknown as InvocationContext" tests/integration/returns nothing.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
These suites run real scripts through
UnsafeLocalCodeExecutor, with no mocked executor.CI note: the first macOS leg failed on
tests/integration/app_loader/app_loader_test.tswithTest timed out in 40000ms. That file is unrelated to this diff, and the leg passed on re-run. All threerun-testslegs (ubuntu, macos, windows) are green.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.