Modernize generator tests - #12216
Conversation
|
@DhanushPillay is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey @sjrl , thanks for the review! I just pushed a commit with those updates. I've added the specific error codes to all the type: ignore comments (like [attr-defined]) so they're no longer generic. For the message.text checks, I dropped the ignores and just added assert message.text is not None before using them, as you suggested. I also fixed a quick merge conflict in pyproject.toml and ran the formatter so the CI should be green now. Let me know if everything looks good! |
e0c9792 to
0c82941
Compare
|
Hi @DhanushPillay, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
|
@DhanushPillay sorry false positive with the bot. |
|
@sjrl I have just pushed a commit to address the remaining review comments. I added the missing return types, replaced the type ignores in |
| ] | ||
| message = ChatMessage.from_tool( | ||
| tool_result=tool_result, | ||
| tool_result=tool_result, # type: ignore[arg-type] |
There was a problem hiding this comment.
Do you know why this one needs an ignore? The type for tool_result is ToolCallResultContentT = str | Sequence[TextContent | ImageContent | FileContent] which I thought matches the input correctly.
There was a problem hiding this comment.
You're absolutely right, it doesn't need the ignore! The issue was that without an explicit type annotation on the tool_result list, mypy inferred a narrower type (list[TextContent] or similar) instead of recognizing it as a valid Sequence[TextContent | ImageContent | FileContent]. I've fixed it by adding an explicit type hint on the variable, so mypy can see it matches ToolCallResultContentT directly. Thanks for catching that
|
@sjrl Hey, I've addressed all the review feedback and CI looks good on my end, the only failing check is the Vercel deploy which seems unrelated. Happy to make any other changes if needed |
Proposed Changes:
This PR modernizes the
test/components/generators/test suite by fully adopting strictmypytype hints.I've gone through the generator tests and cleaned them up to make sure they're completely type-safe. Specifically, this includes:
-> None) and parameter types (e.g., usingAnyfor fixtures and monkeypatches) across all the root generator and chat generator tests.str | Noneso they are now strict-type compliant (e.g. safely checking.text).# type: ignorecomments that were cluttering up the codebase.test/components/generators/directory to thetypescheck inpyproject.tomlso these tests will be officially checked in the CI pipeline going forward.How did you test it?
I verified everything locally by running
hatch run test:types. The entiretest/components/generators/directory now passes with zero errors.Notes for the reviewer
Since this is purely an internal update to the test suite and build config (no user-facing API or feature changes), I skipped generating a release note as per the contribution guidelines for CI/test changes.