Skip to content

Test: drive the RestApiTool suite through real operations and contexts - #750

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/rest-api-tool-test-real-operation-fixtures
Open

Test: drive the RestApiTool suite through real operations and contexts#750
AmaadMartin wants to merge 3 commits into
mainfrom
fix/rest-api-tool-test-real-operation-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: core/test/tools/openapi_tool/rest_api_tool_test.ts reached around RestApiTool with 30 as unknown as casts. Eight tests replaced the private operationParser with a literal parameter array, and 14 tests passed {} as the tool context. The suite therefore never ran OperationParser at all: with OperationParser.processOperationParameters() emptied, all 26 tests still passed. This is the guideline violation "Never Widen private So a Test Can Reach It", plus the hard rule against as unknown as.

Solution: Every test now declares a real OpenAPIV3.OperationObject and drives the real parser, and a createToolContext() factory builds a real Context. The three auth tests build a real ToolAuthHandler (its constructor is public) and stub only prepareAuthCredentials. The cast count goes 30 -> 0, with no any, no @ts-expect-error, no eslint-disable, and no obj['privateField'] access. No production file changes.

Rewriting existing tests is the deliverable here. The usual rule is "add a new test, do not rewrite an existing one", which exists to protect regression signal. The mutation table below is the proof the signal got stronger, not weaker.

One test is deleted: the runAsync-level should fallback to JSON if no requestBody in spec. It was reachable only by faking the parser. A body-located ApiParameter exists only when operation.requestBody is a non-$ref object with non-empty content (operation_parser.ts:74-90), and in exactly that case prepareRequestBody takes the 'content' in requestBody branch (rest_api_tool.ts:243), never the else if (finalData !== undefined) fallback. The test pinned a state the real pipeline cannot produce. The identically named module-level test still pins that branch directly with no mocking, and the deleted test's Content-Type: application/json assertion moved into should stringify object body.

One assertion is strengthened, not weakened. should extract query parameters from path used two toContain checks. Both still passed with the path query-string merge removed, because the unmerged URL is ...?existing=param?new_param=value and still contains both substrings. The test now asserts the exact URL, which makes that mutation lethal. This corrects the plan's prediction that the mutation would drop existing=param.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returns 640 open PRs. Five touch this file (#436, #463, #514, #578, #579); all are purely additive and none removes a cast (git diff main...<branch> -- <file> | grep -c '^-.*as unknown as' is 0 for each). The sibling clean-up of openapi_toolset_test.ts (#337, #645) is tracked separately and is not touched here.

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.

npx vitest run --project unit:core core/test/tools/openapi_tool/rest_api_tool_test.ts -> 25 passed (26 before, minus the deleted test).
npx tsc --noEmit -> 0 errors whose path is this file, matching the baseline. The repo has 2186 pre-existing errors elsewhere; none is new.
npx eslint <file> and npx prettier --check <file> -> clean.

Mutation proof. Each mutation was applied to core/src/tools/openapi_tool/rest_api_tool.ts, the single test file was run, then the source was reverted with git checkout. All twelve rewritten tests fail:

Test Mutation Verbatim failure
pass the configured credential key L63 configureCredentialKey -> no-op expected "spy" to be called with arguments / received "credentialKey": undefined
should stringify object body L194 body = argValue -> body = undefined received "body": undefined, "headers": {}
should replace path parameters L205 drop url.replace received "http://api.example.com/users/{id}"
should stringify bodyData L196 drop the bodyData[originalName] write received "body": undefined
should add header parameters L187 headers[originalName] -> headers[param.name] received "headers": {"trace_id": "trace-123"}, expected traceId
should get declaration L72 parameters: schema -> parameters: {} expected { name: 'test_tool', …(2) } to deeply equal { name: 'test_tool', …(2) }, parameters.properties.user_id missing
should extract query parameters from path L209 drop the path query-string merge received "http://api.example.com/test?existing=param?new_param=value"
x-www-form-urlencoded body L253 return JSON.stringify(finalData) received "body": "{\"foo\":\"bar\",\"baz\":\"qux\"}", expected Any<URLSearchParams>
multipart/form-data body L255 return String(finalData) received "body": "[object Object]", expected Any<FormData>
should return pending if auth is pending L89 drop the pending early return expected { Object (error) } to deeply equal { pending: true, …(1) }
should apply auth credentials to fetch request L120 applyCredential(...) -> initialUrl expected header "X-API-Key": "secret_key", received "headers": {}
should configure auth scheme and credential via setters L53 configureAuthScheme -> no-op received undefined where the scheme was expected

Negative control. With OperationParser.processOperationParameters() emptied (operation_parser.ts:52-72):

  • before this change: Tests 26 passed (26)
  • after this change: Tests 4 failed | 21 passed (25) (path, header, query and declaration tests)

The old suite could not see the parser disappear. The new one can.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Not applicable. This change is test-only and alters no production behaviour. tests/e2e/tools/rest_api_tool_auth_e2e_test.ts is untouched.

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 3 commits August 6, 2026 19:13
The RestApiTool suite proved its claims by replacing the private
operationParser with literal parameter arrays and by passing empty object
literals as the tool context. Neither reaches OperationParser, so the suite
carried no signal about it: with processOperationParameters() emptied, all
26 tests still passed.

Each test now declares a real OpenAPIV3.OperationObject and runs through the
real parser, and a createToolContext() factory builds a real Context. The
runAsync-level 'should fallback to JSON if no requestBody in spec' test is
deleted: a body parameter exists only when the operation declares a
requestBody with content, and in that case prepareRequestBody never takes the
fallback branch. The module-level test of the same name already pins that
branch directly, and its Content-Type assertion moves into 'should stringify
object body'.

The query-parameter test now asserts the exact URL. Its two toContain checks
both passed with the path query-string merge removed, because the unmerged
URL still contains both substrings.
…ApiTool suite

The three auth tests replaced ToolAuthHandler with an object literal cast
through unknown, and the setter test cast its security scheme and credential
the same way. ToolAuthHandler's constructor is public, so each test now builds
a real handler and stubs only prepareAuthCredentials. The security scheme is
annotated (the identical literal is already uncast in 'should apply auth
credentials to fetch request') and the credential carries its required
authType.

This removes the last five casts: the file is now at zero.
createToolContext() set three fields no test reads: LlmAgentConfig.model is
optional and no test drives an LLM, and createSession already defaults userId
and events. The two stub ToolAuthHandlers took an authScheme and an
authCredential that no assertion can observe, because both tests replace
prepareAuthCredentials, the only consumer of those private fields.
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