chore: Add type checking to test/tracing/, test/marshal/, and test/testing/#10913
Merged
bogdankostic merged 7 commits intodeepset-ai:mainfrom Mar 27, 2026
Conversation
…sting/ Partially addresses deepset-ai#10396 by extending mypy type checking to three more test directories. Fixes type errors including missing annotations on pytest fixtures, unhandled Optional returns from current_span(), incorrect generator fixture return types, and dynamic LogRecord attribute access.
Krishnachaitanyakc
requested review from
bogdankostic
and removed request for
a team
|
@Krishnachaitanyakc is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Contributor
|
Hi @Krishnachaitanyakc, thanks for opening this PR! Could you please sign the CLA? |
bogdankostic
added
the
ignore-for-release-notes
github-actions
bot
added
topic:tests
topic:build/distribution
and removed
ignore-for-release-notes
bogdankostic
requested changes
Mar 26, 2026
Contributor
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Thanks for the PR @Krishnachaitanyakc! I left a couple of comments on how we can improve it.
Contributor
There was a problem hiding this comment.
As this PR only touches our tests, we don't need a release note. Let's remove this file.
test/tracing/test_logging_tracer.py
Outdated
| ) | ||
| assert any( | ||
| record.operation_name == "haystack.pipeline.run" for record in records if hasattr(record, "operation_name") | ||
| getattr(record, "operation_name", None) == "haystack.pipeline.run" |
Contributor
There was a problem hiding this comment.
Suggested change
| getattr(record, "operation_name", None) == "haystack.pipeline.run" | |
| getattr(record, "operation_name") == "haystack.pipeline.run" |
test/tracing/test_logging_tracer.py
Outdated
|
|
||
| assert any( | ||
| record.operation_name == "haystack.component.run" for record in records if hasattr(record, "operation_name") | ||
| getattr(record, "operation_name", None) == "haystack.component.run" |
Contributor
There was a problem hiding this comment.
Suggested change
| getattr(record, "operation_name", None) == "haystack.component.run" | |
| getattr(record, "operation_name") == "haystack.component.run" |
test/tracing/test_logging_tracer.py
Outdated
| ) | ||
| assert any( | ||
| record.operation_name == "haystack.pipeline.run" for record in records if hasattr(record, "operation_name") | ||
| getattr(record, "operation_name", None) == "haystack.pipeline.run" |
Contributor
There was a problem hiding this comment.
Suggested change
| getattr(record, "operation_name", None) == "haystack.pipeline.run" | |
| getattr(record, "operation_name") == "haystack.pipeline.run" |
test/tracing/test_opentelemetry.py
Outdated
| @@ -57,6 +57,7 @@ def test_current_span( | |||
| with tracer.trace("test"): | |||
| current_span = tracer.current_span() | |||
| assert tracer.current_span() is not None | |||
Contributor
There was a problem hiding this comment.
Suggested change
| assert tracer.current_span() is not None |
test/tracing/test_tracer.py
Outdated
Comment on lines
+47
to
+49
| current = tracer.current_span() | ||
| assert current is not None | ||
| assert isinstance(current.raw_span(), NullSpan) |
Contributor
There was a problem hiding this comment.
Suggested change
| current = tracer.current_span() | |
| assert current is not None | |
| assert isinstance(current.raw_span(), NullSpan) | |
| current_span = tracer.current_span() | |
| assert current_span is not None | |
| assert isinstance(current_span.raw_span(), NullSpan) |
test/tracing/test_tracer.py
Outdated
| class TestAutoEnableTracer: | ||
| @pytest.fixture() | ||
| def configured_opentelemetry_tracing(self) -> None: | ||
| def configured_opentelemetry_tracing(self) -> Iterator[None]: |
Contributor
There was a problem hiding this comment.
Let's use Generator type here.
Suggested change
| def configured_opentelemetry_tracing(self) -> Iterator[None]: | |
| def configured_opentelemetry_tracing(self) -> Generator[None]: |
The assertion on current_span already covers this check.
bogdankostic
added
the
ignore-for-release-notes
The hasattr guard already ensures the attribute exists, so direct access is safe. Added type: ignore comments since LogRecord doesn't declare operation_name in its type stubs.
bogdankostic
requested changes
Mar 26, 2026
Contributor
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
I made a few comments on how to make the typing checks pass. Also, let's add # type: ignore[func-returns-value] to test_factory.py.
- Remove unnecessary type: ignore[attr-defined] where hasattr guards exist - Add type: ignore[func-returns-value] to delete_documents assertion
bogdankostic
approved these changes
Mar 27, 2026
Contributor
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Looking good, thanks @Krishnachaitanyakc! :)
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
3 participants
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.
Related Issues
Proposed Changes:
Follow up to #10409 and #10428.
This PR extends mypy type checking coverage to three additional test directories:
test/tracing/,test/marshal/, andtest/testing/.Changes made:
pyproject.toml: Addedtest/marshal/,test/testing/, andtest/tracing/to the mypytypesscript targettest/tracing/test_logging_tracer.py: Addedpytestimport, type annotations forcaplogparameters (pytest.LogCaptureFixture), return type annotations for componentrunmethods, and# type: ignore[attr-defined]for dynamically-added LogRecord attributestest/tracing/test_datadog.py: Addedassert current_span is not Noneto narrowSpan | Nonetype before attribute accesstest/tracing/test_opentelemetry.py: SameNonenarrowing forcurrent_span()test/tracing/test_tracer.py: AddedIteratorimport, fixed generator fixture return type (None->Iterator[None]), addedNonenarrowing forcurrent_span()before callingraw_span()test/testing/test_factory.py: Changedassert store.delete_documents([]) is Nonetostore.delete_documents([])since the method is typed to returnNoneHow did you test it?
hatch run test:types-- 333 source files, 0 errorshatch run fmt-- all checks passedhatch run test:unit -- test/tracing/ test/testing/ test/marshal/-- 75 tests passedChecklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.