Skip to content

test(plugins): assert error propagation in on_*_error fallback tests#6309

Open
anxkhn wants to merge 1 commit into
google:mainfrom
anxkhn:test/plugin-error-fallback-assertions
Open

test(plugins): assert error propagation in on_*_error fallback tests#6309
anxkhn wants to merge 1 commit into
google:mainfrom
anxkhn:test/plugin-error-fallback-assertions

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown

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 (no existing issue; description below).

2. Or, if no issue exists, describe the change:

Problem:

Three plugin "fallback to runner" tests are meant to verify that when the
on_*_error callback is disabled, the underlying error is not swallowed and
propagates out through the runner. As written, they do not actually enforce that:

  • test_async_on_tool_error_fallback_to_runner and
    test_live_on_tool_error_fallback_to_runner
    (tests/unittests/flows/llm_flows/test_plugin_tool_callbacks.py) are shaped as:

    try:
        await invoke_tool_with_plugin(mock_error_tool, mock_plugin)
    except Exception as e:
        assert e == mock_error

    The only assertion lives inside the except. If the production code ever stops
    raising (exactly the regression these tests exist to catch), the try body
    completes normally, the except never runs, the assertion is skipped, and the
    test still passes green.

  • test_on_model_error_callback_fallback_to_runner
    (tests/unittests/flows/llm_flows/test_plugin_model_callbacks.py) is worse: its
    try only constructs InMemoryRunner(agent, plugins=[...]), which never runs the
    agent, so no error path is exercised at all and the test can never fail.

All three pass today over correct code, so this is latent false safety, not a live
bug: they would not catch the swallow-the-error regression they are named for.

Solution:

Rewrite the three test bodies so a no-raise regression fails them, following
conventions already used in this repo. No production code changes.

  • Tool tests: wrap the call in pytest.raises(ClientError) and assert the captured
    error, so the test fails if nothing is raised:

    with pytest.raises(ClientError) as exc_info:
        await invoke_tool_with_plugin(mock_error_tool, mock_plugin)
    assert exc_info.value == mock_error
  • Model test: actually run the agent, then assert the unhandled model error surfaces
    as a terminal error event. On the synchronous runner an unhandled model error is
    converted into an error event rather than raised to the caller, so pytest.raises
    is not the right shape here; this mirrors the existing error-event assertions in
    tests/unittests/flows/llm_flows/test_base_llm_flow.py:

    runner = testing_utils.InMemoryRunner(agent, plugins=[mock_plugin])
    events = runner.run('test')
    error_events = [e for e in events if e.error_code]
    assert len(error_events) == 1
    assert error_events[0].error_code == 'ClientError'

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.
uv run --no-sync pytest \
  tests/unittests/flows/llm_flows/test_plugin_tool_callbacks.py \
  tests/unittests/flows/llm_flows/test_plugin_model_callbacks.py -q
# 15 passed

To confirm the hardened tests actually catch the target regression, I temporarily
neutralized the three production re-raises (the raise sites in
src/google/adk/flows/llm_flows/functions.py and
src/google/adk/flows/llm_flows/base_llm_flow.py); all three rewritten tests then
failed, whereas the previous bodies still passed under the same change. I reverted
the temporary edit; no production code is modified by this PR.

Manual End-to-End (E2E) Tests:

Not applicable. This is a test-only change (assertions in three existing unit tests);
there is no user-facing behavior to exercise manually.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas. (N/A; small test edits, no new comments needed.)
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (N/A; test-only change.)
  • Any dependent changes have been merged and published in downstream modules. (None.)

Additional context

Net diff is +11/-11 across the two test files; no production files change. The model
test's error-event idiom follows test_base_llm_flow.py; each file's existing quote
style is preserved.

The three on_*_error "fallback to runner" tests placed their only assertion
inside an `except` block with no guard, so they passed even when no error was
raised, the exact regression they exist to catch. The model-callback variant
only constructed InMemoryRunner (which never runs the agent), so it could
never raise and was fully vacuous.

Rewrite them to fail on a no-raise regression, matching existing repo
conventions:
- test_async/live_on_tool_error_fallback_to_runner: wrap the call in
  `pytest.raises(ClientError)` and assert the raised error.
- test_on_model_error_callback_fallback_to_runner: run the agent and assert
  the unhandled model error surfaces as a terminal error event
  (error_code == 'ClientError'), mirroring the error-event assertions in
  test_base_llm_flow.py.

No production code 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