Skip to content

Fix: repair the 54 undefined names under tests/ and contributing/, and lint for them - #233

Open
AmaadMartin wants to merge 5 commits into
mainfrom
fix/f821-undefined-names-tests-contributing
Open

Fix: repair the 54 undefined names under tests/ and contributing/, and lint for them#233
AmaadMartin wants to merge 5 commits into
mainfrom
fix/f821-undefined-names-tests-contributing

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

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

Problem: ruff check tests/ contributing/ --select F821 reports 54 undefined names. No tool in the repo looks for them: the ruff pre-commit hook is scoped files: ^src/, lint.select is ["F401"], and mypy excludes tests/ and contributing/samples/. Three of the 54 are real NameErrors that a marker or a skip currently hides. The rest are missing imports and two names that never existed.

Solution: I repaired all 54 findings and added a second ruff pre-commit hook that runs --select F821 over tests/ and contributing/, so the class of defect cannot come back. I used a separate hook rather than widening the existing files: ^src/ pattern, because an explicit --select on the command line replaces the configured select for that run. Widening the existing hook would instead apply F401 to tests/, which is a much larger and unrelated change. No # noqa suppressions.

What changed, by finding:

  • test_workflow_schema.py (40), test_workflow_node.py (1), test_workflow_parallel_worker.py (2) — added the missing Any, AsyncGenerator, Context and RequestInput imports.
  • test_multimodal_tool_results_plugin.py (6) — MockTool and ToolReturningGenAiPartsPlugin are phantom names that exist nowhere in the repository. The mock_tool fixture returns Mock(spec=BaseTool) and the plugin fixture returns MultimodalToolResultsPlugin, so I annotated them with those types.
  • contributing/samples/models/interactions_api/main.py (1) — test_agent = agent.root_agent becomes test_agent = root_agent. The module imports root_agent directly and has no name agent, so interactive mode crashed on entry. The sibling function 50 lines above already uses the correct form.
  • tests/integration/test_system_instruction.py (4) — deleted. Please object if you want it kept. An unconditional pytest.skip(allow_module_level=True) under the comment # Skip until fixed. has disabled it since the initial import commit. It calls UnitFlow() and _context_formatter, which appear nowhere else in this repository and have no from __future__ import annotations to make them inert, so removing the skip raises NameError on the first test. CI does not run tests/integration, and instruction and context-variable population is covered by the 36 tests in tests/unittests/flows/llm_flows/test_instructions.py.

Two genuine NameErrors are repaired: RequestInput and agent.root_agent.

Collision check. I ran gh pr list and checked every open PR for these files. No open PR makes this change. Three overlap without conflicting: #168 widens the src/ ruff hook for F401 and removes unused imports from two of these files, #141 and #180 change a fixture decorator in the multimodal test. My hunks do not touch the same lines, so I branched from main rather than stacking. #200 selects F821 for src/ only and touches a disjoint file set.

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 adds no executable product code, so it adds no test cases. The regression guard is the new hook. I proved it fails by reintroducing the defects it must catch.

# 1. Every finding is gone.
$ ruff check tests/ contributing/ --select F821
All checks passed!

# 2. The explicit file list CI's `pre-commit run --all-files` will pass to ruff.
$ git ls-files 'tests/*.py' 'tests/**/*.py' 'contributing/*.py' 'contributing/**/*.py' \
    | xargs ruff check --select F821 --force-exclude      # 1078 files
All checks passed!

# 3. Both hooks, over every tracked file.
$ pre-commit run --all-files
ruff (legacy alias)..................................Passed
ruff undefined-name check (tests, contributing)......Passed
isort / pyink / addlicense / codespell / ............Passed

# 4. The affected tests.
$ pytest tests/unittests/workflow/test_workflow_schema.py \
         tests/unittests/workflow/test_workflow_node.py \
         tests/unittests/workflow/test_workflow_parallel_worker.py \
         tests/unittests/plugins/test_multimodal_tool_results_plugin.py -q -rxX
54 passed, 1 skipped, 2 xfailed

Mutation runs, to prove each guard fails on the defect it pins:

  • I deleted from typing import Any from test_workflow_schema.py and restored agent.root_agent in the sample, then ran the hook. It failed: ruff undefined-name check (tests, contributing)......Failed, Found 22 errors (20 Any, 2 agent). Restoring both lines returns it to Passed.
  • I deleted the RequestInput import and ran test_parallel_worker_pauses_for_human_input with --runxfail. It failed at line 446 with NameError: name 'RequestInput' is not defined, before reaching a single assertion. With the import, the same run reaches its assertions and fails on the node path: ('...Worker@1/Worker@1', ...) != ('...Worker@1', ...).

That second run answers the xfail question. test_parallel_worker_pauses_for_human_input still XFAILs after the fix, so the NameError was not why it failed and the marker ctx.run_node needs barrier for parallel HITL is still accurate. I left both the xfail and the skip markers untouched.

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Manual End-to-End (E2E) Tests:

The interactions_api sample needs live model credentials, so I did not run it. The line-375 fix is verified by inspection against the identical working line 324, and by ruff reporting zero F821.

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.
  • 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.

CI

Unit Tests (3.10-3.14), A2A v0.3 Tests (3.10-3.14) and Mypy Check (3.10-3.13) all pass. The new hook passes: ruff undefined-name check (tests, contributing)......Passed.

Pre-commit Linter fails on one unrelated hook, update-constraints. It regenerates the constraints files with today's --exclude-newer date and then reports the tree as dirty. This is not my change: the same job fails on the same hook at my base commit 352d11d3, and PRs #163, #213, #214 and #225 already address it.

Amaad Martin added 5 commits August 11, 2026 17:47
Three workflow test modules use Any, AsyncGenerator, Context and
RequestInput without importing them. The RequestInput uses sit in
runtime yield position, so they raise NameError whenever the node runs.
MockTool and ToolReturningGenAiPartsPlugin do not exist anywhere in the
repository. The fixtures produce Mock and MultimodalToolResultsPlugin.
An unconditional module-level skip has disabled this file since the
initial import. It calls UnitFlow and _context_formatter, which have
never existed, so removing the skip raises NameError. The behaviour it
targeted is covered by tests/unittests/flows/llm_flows/test_instructions.py.
…tive mode

The module imports root_agent directly and has no name agent, so
interactive mode raised NameError before it could start a runner.
The existing ruff hook is scoped to src/ and mypy excludes tests/ and
contributing/samples/, so nothing looked for undefined names there.
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