Skip to content

test: enable mypy typing checks for test/components/builders - #12461

Merged
davidsbatista merged 2 commits into
deepset-ai:mainfrom
ShousenZHANG:test/type-check-builder-tests
Aug 26, 2026
Merged

test: enable mypy typing checks for test/components/builders#12461
davidsbatista merged 2 commits into
deepset-ai:mainfrom
ShousenZHANG:test/type-check-builder-tests

Conversation

@ShousenZHANG

Copy link
Copy Markdown
Contributor

Related Issues

Proposed Changes:

test/components/builders/ is not in the repository mypy target, so its test modules are not checked by hatch run test:types. This is the next disjoint increment for #10396, claimed in this comment. It does not overlap the increments in flight: #12343 has extractors/, #12433 has samplers/, #12435 has fetchers/.

The baseline was 25 errors across 2 of the 3 test modules. Nothing here changes what a test asserts:

  • 12 attr-defined on __haystack_input__ / __haystack_output__, which the @component decorator attaches at runtime. Ignored the same way the production code reads them in core/pipeline/base.py.
  • 8 index / union-attr (4 lines, two errors each) from builder.template[0].text. ChatPromptBuilder.template is typed list[ChatMessage] | str | None, and it is always a list in those tests, so this is narrowed with assert isinstance(builder.template, list) rather than silenced. That resolves both errors per line.
  • 2 no-untyped-def on the inline DocumentProducer.run test components, which now declare -> dict[str, list[Document]].
  • 1 list-item on template=[ChatMessage.from_user("Hello"), "there world"], a deliberately mixed-type template inside the test that asserts ChatPromptBuilder rejects it. Narrow # type: ignore[list-item], kept on the template= argument itself so the suppression stays attributed to the offending value.

Per AGENTS.md I kept type: ignore to the cases where it is necessary; the index / union-attr group is narrowed with a real assertion instead.

How did you test it?

Red/green against the repository configuration:

  • Before: mypy test/components/builders/ reported 25 errors in 2 files.
  • After: Success: no issues found in 4 source files.
  • Full target: hatch run test:types -> Success: no issues found in 461 source files (up from 457).
  • hatch run test:unit test/components/builders/ -> 141 passed, 1 deselected.
  • hatch run fmt-check (whole repo) -> clean, 6785 files.

Notes for the reviewer

The only lines that execute anything new are the three assert isinstance(builder.template, list) narrowings, which document an invariant those tests already rely on. Everything else is an annotation or a trailing comment.

Heads-up on this campaign: every increment edits the same single types = "..." line in pyproject.toml, so whichever one lands first leaves the others conflicting. #12433 and #12435 are showing conflicts right now for that reason. Happy to rebase this one whenever it is convenient for you.

No release note: this is test-only, which AGENTS.md excludes from the release-note requirement.

This PR was fully generated with an AI assistant. I have reviewed the changes and run the relevant tests.

Checklist

  • I have read the contributors guidelines and the code of conduct.
  • I have updated the related issue with new insights and changes.
  • I have added unit tests and updated the docstrings.
  • I've used one of the conventional commit types for my PR title.
  • I have documented my code.
  • I have added a release note file - not needed here: AGENTS.md scopes the requirement to user-facing changes, and this is test-only.
  • I have run pre-commit hooks and fixed any issue.

@ShousenZHANG
ShousenZHANG requested a review from a team as a code owner August 25, 2026 00:35
@ShousenZHANG
ShousenZHANG requested review from davidsbatista and a lite review from Copilot and removed request for a team August 25, 2026 00:35
@vercel

vercel Bot commented Aug 25, 2026

Copy link
Copy Markdown

@ShousenZHANG is attempting to deploy a commit to the deepset Team on Vercel.

A member of the Team first needs to authorize it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

  • Purpose: add test/components/builders/ to the mypy target (hatch run test:types) and resolve the resulting mypy errors via localized test annotations/narrowing, without changing test behavior.

Changes:

  • Extend the mypy types target in pyproject.toml to include test/components/builders/.
  • Fix mypy errors in builder tests by adding targeted # type: ignore[attr-defined] for runtime-injected __haystack_input__ / __haystack_output__ and by narrowing ChatPromptBuilder.template to list before indexing.
  • Add explicit return types for inline @component test components (run(...) -> dict[str, list[Document]]) and a narrow # type: ignore[list-item] for the deliberate mixed-type negative test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
test/components/builders/test_prompt_builder.py Adds typing suppressions for runtime-injected sockets and return type annotations for an inline test component.
test/components/builders/test_chat_prompt_builder.py Narrows template before indexing, adds socket suppressions, annotates inline test component return type, and scopes a deliberate mixed-type ignore.
pyproject.toml Includes test/components/builders/ in the mypy types script target.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@ShousenZHANG

Copy link
Copy Markdown
Contributor Author

Heads-up on the red Unit jobs: the failure is pre-existing on main and unrelated to this PR.

The single failing test is test/components/converters/test_pypdf_to_document.py::TestPyPDFToDocument::test_run_detect_paragraphs_to_be_used_in_split_passage, on all three platforms. Everything else passes (1 failed, 6298 passed, 8 skipped).

It reproduces on main itself, not just here — the Tests workflow started failing on main at 29b4171c and has failed on every run since, including ea63575f, which is what this branch is based on:

2026-08-24T16:11  failure  ea63575f   <- base of this branch
2026-08-24T14:04  failure  29b4171c
2026-08-24T10:04  success  7c8ce0f2
2026-08-24T07:40  success  0528f0cb

The assertion diff is a text-extraction change, not a typing one:

- to the public or limited to use within an organization for maintaining its internal knowledge
+ to the public or limited to use within an organization for maintaining its inter nal knowledge

pypdf is emitting a stray space inside "internal", so this looks like it came in with a dependency bump rather than from application code.

This PR touches only test/components/builders/ and one line of pyproject.toml; it doesn't go near the converters. Locally against this branch: hatch run test:types gives Success: no issues found in 461 source files (457 before), hatch run test:unit test/components/builders/ gives 141 passed, and hatch run fmt-check is clean across the repo.

Happy to rebase once main is green again if that's easier for review.

Adds the directory to the `types` target and clears the 25 errors that
surfaced, without changing what any test asserts:

- Narrow `builder.template` with an `isinstance` check before indexing it,
  since it is typed `list[ChatMessage] | str | None` but is always a list in
  these tests.
- Annotate the return type of the inline `DocumentProducer.run` components.
- Add a narrow `# type: ignore[list-item]` to the deliberately mixed-type
  template inside the validation test that asserts it is rejected.
- Ignore `attr-defined` on `__haystack_input__` / `__haystack_output__`, which
  the `@component` decorator attaches at runtime, matching how the production
  code reads them.
@ShousenZHANG
ShousenZHANG force-pushed the test/type-check-builder-tests branch from e8e97a2 to 2fd9c1a Compare August 25, 2026 11:45
@ShousenZHANG

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #12463 has landed, so the Unit failure I mentioned above is gone. Root cause was exactly that: PyPDF 6.16.2 changed the extracted text, and the converter test's expected string was updated to match.

Verified locally against the same PyPDF version CI uses (6.16.2):

  • test/components/converters/test_pypdf_to_document.py -> 15 passed
  • full hatch run test:unit -> 6266 passed
  • hatch run test:types -> Success: no issues found in 461 source files (457 before this PR)
  • hatch run fmt-check -> clean across the repo

The change itself is unchanged by the rebase: one line in the types target plus the typing fixes in test/components/builders/.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Coverage report

This PR does not seem to contain any modification to coverable code.

@vercel

vercel Bot commented Aug 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
haystack-docs Ready Ready Preview Aug 26, 2026 6:49am

Request Review

@davidsbatista davidsbatista left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks!

@davidsbatista
davidsbatista enabled auto-merge (squash) August 26, 2026 06:52
@davidsbatista
davidsbatista merged commit 7e0c48c into deepset-ai:main Aug 26, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants