Test: share the click-muting fixture via tests/unittests/cli/utils/conftest.py - #70
Open
AmaadMartin wants to merge 4 commits into
Open
Test: share the click-muting fixture via tests/unittests/cli/utils/conftest.py#70AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 4 commits
July 30, 2026 19:22
Six CLI unit-test modules each carry a private copy of an autouse `_mute_click` fixture. Hoist one copy - the variant that supports the `unmute_click` marker opt-out - into a directory-level conftest so every module under tests/unittests/cli/utils gains a supported way to assert on click output. test_cli_eval_pretty_print.py asserts on click.echo output via capsys, so it opts out with a module-level pytestmark; without it the new directory-wide autouse fixture would silently swallow the output it asserts on. The per-module copies are removed in the following commit so that each commit leaves the suite green.
test_cli.py, test_cli_create.py, test_cli_deploy.py,
test_cli_deploy_ignore.py and test_cli_deploy_to_cloud_run.py all defined an
autouse fixture that muted click.echo and click.secho; the conftest fixture
added in the previous commit does exactly that for the whole directory.
test_cli_deploy_ignore.py and test_cli_deploy_to_cloud_run.py referenced
click only from the deleted fixture, so their `import click` goes too. The
other three still use click (click.Abort, click.ClickException,
mock.patch("click.echo"), ...) and keep the import.
conftest autouse fixtures are ordered ahead of module-level ones, so
test_cli_deploy.py's reload_cli_deploy still runs after muting, and
importlib.reload(cli_deploy) does not undo a patch applied to the click
module object.
…erride This module deliberately mutes only click.echo and leaves click.secho live, because several of its tests assert on error text written with secho - e.g. test_cli_deploy_cloud_run_failure asserts "Deploy failed: boom" is in result.output, which cli_tools_click emits via click.secho. Record that in the docstring so the deviation from the shared conftest fixture reads as intentional, annotate the request parameter, and drop the commented-out secho line the docstring now explains.
…ady say Both _mute_click docstrings restated their own guard clause, and the conftest module docstring claimed to cover "the ADK CLI unit tests" when the fixture only reaches tests/unittests/cli/utils. Collapse each docstring to the one fact the code does not convey and scope the module docstring correctly.
This was referenced Jul 31, 2026
Open
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
Closes: #issue_number
Related: #issue_number
Problem: Six modules under
tests/unittests/cli/utils/each carried their own private copy of an autouse_mute_clickfixture that monkeypatchesclickoutput functions to no-ops. Five of those copies offered no way to opt out, so any new test in those modules that needs to assert on click output had to shadow or work around the fixture. Theunmute_clickmarker escape hatch existed in exactly one module.Solution: Hoist a single shared fixture — the variant that supports the
unmute_clickmarker opt-out — into a newtests/unittests/cli/utils/conftest.py, and delete the five redundant copies. Net result: duplication drops from six copies to two intentional definitions, and every module in the directory gains a supported opt-out. Net -3 lines.Two deliberate deviations, both because the fixtures were not behaviourally identical:
test_cli_tools_click.pykeeps its own_mute_click. It mutes onlyechoand deliberately leavessecholive. Several of its tests assert on error text written withclick.secho— e.g.test_cli_deploy_cloud_run_failureasserts"Deploy failed: boom" in result.output, emitted byclick.secho(f"Deploy failed: {e}", fg="red", err=True)insrc/google/adk/cli/cli_tools_click.py. Because a module-level fixture shadows a same-namedconftest.pyfixture, keeping the name_mute_clickpreserves today's behaviour exactly. Its docstring now records why it deviates, and the commented-outsecholine that the docstring replaces is gone.test_cli_eval_pretty_print.pyopts out withpytestmark = pytest.mark.unmute_click. It had no mute fixture before and asserts onclick.echooutput throughcapsys; a directory-scoped autouse fixture would otherwise silently break it. Module-levelpytestmarkrather than a per-test decorator, because every test in that module exists to assert on printed output.Why a directory-level
conftest.pyis safe for the other 11 modules in the directory: none of them referenceclick,capsys, orCliRunner, so the newly directory-wide muting is a no-op for them.Scope notes: no file under
src/is touched, no test is added, renamed, deleted, skipped, or xfailed, andpyproject.tomlis untouched. Theunmute_clickmarker is intentionally not registered here — it is already unregistered at HEAD (9 existing usages) and emits only aPytestUnknownMarkWarning, never an error. This change adds one new occurrence of that pre-existing warning, not a new kind of warning; registering the marker is a separate change so that the two cannot conflict in either merge order.Collision check (required by our workflow): ran
gh pr list --repo AmaadMartin/adk-python --state open --limit 100and scanned the--name-onlyfile list of every one of the 67 open PRs, plus grepped all 67 diffs for_mute_click. No open PR createstests/unittests/cli/utils/conftest.pyand no open PR adds or removes a_mute_clickline. The nearest neighbour is #52 ("register theunmute_clickpytest marker"), which touches onlypyproject.toml— complementary, zero file overlap, and safe in either merge order for the reason given above. Several open PRs edittest_cli_tools_click.py(#51, #53, #62, #64), but none of them touch the fixture block at the top of the file. Branched from currentmain(923dee79); not stacked.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 change ships no production code, so the plan is a behaviour-preservation proof rather than new test authoring.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Behaviour preservation — identical pass counts before and after.
pytest tests/unittests/cli -q -p no:randomly:main,923dee79)1 failed, 569 passed, 5 skipped, 155 warnings1 failed, 569 passed, 5 skipped, 156 warningsThe single failure is pre-existing on unmodified
mainand unrelated:test_cli_tools_click.py::test_telemetry_cli_commands, a click >= 8.2 bare-group exit-code issue (installed click is 8.4.2; PRs #51/#53/#64 address it). The155 -> 156warning delta is exactly the one predicted new occurrence of the pre-existingPytestUnknownMarkWarning: Unknown pytest.mark.unmute_click, raised attest_cli_eval_pretty_print.py:32.Targeted regression proofs, each guarding one hazard this design exists to avoid — all pass:
pytest "tests/unittests/cli/utils/test_cli_tools_click.py::test_cli_deploy_cloud_run_failure"->1 passed(provesclick.sechois still live in that module).pytest tests/unittests/cli/utils/test_cli_eval_pretty_print.py->1 passed(proves theunmute_clickopt-out keepsclick.echolive).pytest tests/unittests/cli/utils/test_cli_deploy_ignore.py->3 passed(proves the shared conftest fixture is picked up by a module that no longer defines one).Coverage of the new file: 100% line and branch.
Mutation proofs — coverage is a floor, so each behaviour was proven to be pinned by a test that actually fails without it. Every mutation was reverted afterwards and the test re-run green.
if "unmute_click" in request.keywords: returnguard fromconftest.py.->
test_cli_eval_pretty_print.pyFAILED:AssertionError: assert 'Rubric: invocation-rubric' in ''/where '' = CaptureResult(out='', err='').out. Restored ->1 passed.test_cli_tools_click.pyoverride is load-bearing. Deleted that module's_mute_clickso the conftest fixture (which mutessecho) applied to it.->
test_cli_deploy_cloud_run_failureFAILED:AssertionError: assert 'Deploy failed: boom' in ''. Restored ->1 passed. This is precisely why the override is kept rather than folded into the shared fixture.pytest tests/unittests/cli/utils/test_cli_deploy_ignore.py -q -semits 0 lines matchingCopying agent source code|Creating Dockerfile|Deploying to Cloud Runwithconftest.pypresent, and 10 such lines with it removed (Copying agent source code...,Creating Dockerfile..., ...). Restored.Lint/format, on the exact commit pushed:
pyink==25.12.0 --config pyproject.toml --check->8 files would be left unchanged.isort==8.0.1 --settings-path pyproject.toml --check-only-> exit 0.test_cli_deploy_ignore.pyortest_cli_deploy_to_cloud_run.py;import clickwas removed from exactly those two files (their onlyclickreference was the deleted fixture) and correctly retained in the three that still use it (test_cli.py:151,513,test_cli_create.pyclick.Abort/click.BadParameter,test_cli_deploy.pyclick.ClickException/mock.patch("click.echo")).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 is test-only scaffolding with no runtime behaviour change. As a manual sanity check that the shared fixture is genuinely applying, run a de-duplicated module with output capture off and confirm no click output reaches stdout:
To see the opposite, temporarily rename
tests/unittests/cli/utils/conftest.pyand re-run — deploy progress lines will appear.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.
CI status on this PR (honest classification: failed — both failures pre-existing on
main)The real test jobs ran. Two checks are red, and neither is caused by this diff; both reproduce identically on unrelated branches, so I have deliberately not "fixed" them here (doing so would duplicate live PRs — see below).
1.
Unit Tests(all of 3.10 / 3.11 / 3.12 / 3.13 / 3.14) — one pre-existing failure.This is the click >= 8.2 bare-group exit-code issue. Evidence it is not mine:
main(923dee79) before I edited a single file:1 failed, 569 passed, 5 skippedfortests/unittests/cli, same test.error_codechange that touches no CLI file) fails on the same single test in CI:FAILED ...::test_telemetry_cli_commands - assert 2 == 0.9360 passedon 3.10 versus this PR's9358. That −2 is exactly accounted for: Fix: emit LlmResponse.error_code as a plain string from the OpenAI Responses model #67 adds one test function decorated with@pytest.mark.parametrizeover 2 cases. Somain's true baseline is9358, and this PR reports9358— unchanged. Independently,git diff main -U0 | grep -E '^[-+](async )?def test_'returns nothing: this branch adds, removes, renames, skips and xfails zero tests.adk telemetry(Part 1/2) #51, Fix: make bareadk telemetryexit 0 on Click >= 8.2 #53 and Fix: print help and exit 0 for bare CLI command groups on click >= 8.2 #64, which each already address this exact test/behaviour.2.
Pre-commit Linter— pre-existing infrastructure failure, not a lint violation.The failing hook is
update-constraints, which dies with./scripts/update_constraints.sh: line 103: uv: command not foundfor all five constraints files.pre-commit/action@v3.0.1runs--all-files, so this hook fires onpyproject.tomlon every PR regardless of its diff — and this PR does not touchpyproject.tomlor anyconstraints-*.txt. Unrelated PR #67 fails on the identical hook with the identical message. Live PRs #49, #54 and #56 exist to fix this hook; duplicating them here would be scope creep.Every hook that actually applies to my files passes — verified locally on the exact pushed commit:
Everything else is green:
Mypy Checkon 3.10 / 3.11 / 3.12 / 3.13 andA2A v0.3 Testson 3.10 / 3.11 / 3.12 / 3.13 / 3.14 all pass.