Skip to content

Test: Convert the reflect-and-retry plugin tests to pytest style - #246

Open
AmaadMartin wants to merge 2 commits into
mainfrom
feat/pytest-style-reflect-retry-plugin-tests
Open

Test: Convert the reflect-and-retry plugin tests to pytest style#246
AmaadMartin wants to merge 2 commits into
mainfrom
feat/pytest-style-reflect-retry-plugin-tests

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 12, 2026

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: The two reflect-and-retry test files were the last unittest.IsolatedAsyncioTestCase classes under tests/unittests/plugins/, so the directory used two testing idioms. The guidelines require pytest and pytest.raises(SpecificError, match=...). Three assertRaises sites asserted only the exception type, and two of them used a bare Exception.

Solution: I dropped the base class, marked the 32 remaining async tests with @pytest.mark.asyncio, and rewrote the 122 self.assert* calls as bare asserts. Each of the 8 pytest.raises calls now carries a match= pattern copied from the plugin source. No production source changes.

10 tests are now synchronous. They never await. They only build plugins and call adk_handle_model_error, _check_for_model_error and _get_model_name_from_context, which are all plain def (_reflect_retry_model_plugin.py:79,103,120). They were async because IsolatedAsyncioTestCase made that the idiom, so the coroutine and its event loop bought nothing. The collected count per file is unchanged.

Collision check: gh pr list --repo AmaadMartin/adk-python --state open --limit 100 returned no pull request that touches either file. The nearest neighbours are #179 and #180 (async marker hygiene) and #168 (unused imports); I checked their file lists with gh pr diff --name-only and none includes test_reflect_retry_model_plugin.py or test_reflect_retry_tool_plugin.py. This branch is based on the current main.

The two pytest.raises(Exception) sites are deliberate. _ensure_exception (reflect_retry_tool_plugin.py:269) returns error if isinstance(error, Exception) else Exception(str(error)), so the raised object is exactly Exception, not a subclass. Both tests drive a plugin whose extract_error_from_result returns a dict. Each site adds assert type(exc_info.value) is Exception, which pins the exact type and subsumes the old assertNotIsInstance(cm.exception, TypeError) guard. Giving the plugin a specific error type is a behaviour change and is out of scope.

Why dropping IsolatedAsyncioTestCase is safe. I deleted a comment that justified the base class, so here is why its hazard no longer applies. An earlier form of that comment cited pytest-dev/pytest-asyncio#1039. That issue is a pytest-asyncio 0.25.1 regression on Python 3.9 only; the reporter states that 3.10 through 3.13 are unaffected, and the maintainers closed it as not planned. This repository requires Python 3.10 or newer (pyproject.toml:15) and CI tests 3.10 through 3.14, so no supported version can hit it.

Four further reasons:

  1. Neither class defines setUp, asyncSetUp, tearDown, setUpClass, or any class attribute. Every plugin, mock and runner is built inside the test body, so nothing outlives one test's loop.
  2. ScopedFailureTracker.__init__ creates an asyncio.Lock(). Since Python 3.10, a Lock binds to the running loop on first acquire, not at construction. That is also what makes the 10 synchronous tests above safe.
  3. pyproject.toml:341 already sets asyncio_default_fixture_loop_scope = "function", and no async fixture is involved, so each test gets a fresh loop.
  4. testing_utils.TestInMemoryRunner already runs under plain pytest-asyncio elsewhere, for example tests/unittests/flows/llm_flows/test_functions_simple.py.

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.

Collected counts are unchanged: 18 for the model file and 24 for the tool file, before and after. The directory total stays 658.

uv run pytest tests/unittests/plugins -q
  before: 658 passed      after: 658 passed

uv run pytest tests/unittests/plugins/test_reflect_retry_model_plugin.py \
              tests/unittests/plugins/test_reflect_retry_tool_plugin.py \
              -q -o asyncio_mode=strict
  after: 42 passed, 0 skipped, 0 failed

uv run pytest tests/unittests/plugins -q -o asyncio_mode=strict
  before: 31 failed, 521 passed, 106 errors    after: identical, entry for entry

The strict-mode directory run fails for a reason this change does not touch. Five other files in the directory hold unmarked async tests. I captured the 137 FAILED/ERROR node ids before and after; diff reports them identical, and none belongs to the two converted files.

Mutation testing. I ran each converted test against broken source and confirmed it fails.

Mutation Result
"Agent model not found." -> "Agent model missing." 1 failed: Expected regex: 'Agent model not found'
retry-limit message drops {self.max_retries} 1 failed: Expected regex: 'failed consecutively 1 times ...'
_ensure_exception returns error unwrapped 2 failed: both dict-error tests
the non-negative max_retries guard removed 1 failed: DID NOT RAISE ValueError
the default plugin name changed 1 failed: test_plugin_initialization_default
failure counter increments by 2 19 failed across both files
one @pytest.mark.asyncio removed 1 failed: async def functions are not natively supported

Rows 1, 4 and 5 target tests that are now synchronous, so the conversion did not weaken them. The last row shows the strict-mode gate reports a missing marker as a failure, not a silent skip.

Manual End-to-End (E2E) Tests:
Not applicable. This change alters no runtime behaviour. test_hallucinating_tool_name drives a real LlmAgent through testing_utils.TestInMemoryRunner and is carried across unchanged apart from the marker and one assertion.

CI. Unit Tests pass on Python 3.10, 3.11, 3.12, 3.13 and 3.14. Mypy Check and the A2A v0.3 Tests pass on every version.

Pre-commit Linter fails, for a reason this change does not cause. The failing hook is update-constraints, which reports "files were modified by this hook". That hook runs only on pyproject.toml and constraints-*.txt; this branch touches neither. The same job fails on the other open pull requests, for example #243, #244 and #245.

I also ran the formatters locally on the pushed commit:

uv run pyink --check <both files>    2 files would be left unchanged
uv run isort --check-only <both>     clean
uv run ruff check <both files>       All checks passed!

ruff check tests/unittests/plugins/ reports 4 pre-existing F401 errors in test_global_instruction_plugin.py and test_multimodal_tool_results_plugin.py. Those files are not in this change.

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 2 commits August 11, 2026 23:15
…tyle

The two reflect-and-retry test files were the last IsolatedAsyncioTestCase
classes under tests/unittests/plugins. Drop the base class, mark all 42
async tests with @pytest.mark.asyncio, and rewrite the 122 self.assert*
calls as bare asserts.

The 8 assertRaises sites become pytest.raises with a match= pattern taken
from the plugin source. The two dict-error sites keep Exception, because
_ensure_exception wraps a non-Exception error in the base class; they now
also pin type(exc) is Exception, which subsumes the old
assertNotIsInstance(TypeError) regression guard.

No production source is touched.
…hronous

10 of the 42 converted tests contain no await. They only build plugins and
call adk_handle_model_error, _check_for_model_error and
_get_model_name_from_context, which are all plain def. They were async only
because IsolatedAsyncioTestCase made that the idiom, so the coroutine and
its event loop bought nothing.

Make those 10 plain def and drop their now-pointless asyncio marker. The
collected count per file is unchanged at 18 and 24; the remaining 32 async
tests keep their marker. Constructing a plugin outside a running loop is
safe on the repository's Python 3.10 floor, where asyncio.Lock binds to a
loop on first acquire rather than at construction.

Also tighten one assertion on a line this branch already rewrites:
throw_exception_if_retry_exceeded is False, not merely "is not True", which
would also pass for None or 0.
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