-
Notifications
You must be signed in to change notification settings - Fork 106
Add tests for type-checker false negatives #976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
226ffb2
to
8a0a510
Compare
@@ -5302,7 +5305,7 @@ async def execute_operation( | |||
async def execute_operation( | |||
self, | |||
operation: Callable[ | |||
[ServiceHandlerT, nexusrpc.handler.StartOperationContext, InputT], | |||
[ServiceT, nexusrpc.handler.StartOperationContext, InputT], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR reveals one bug in Nexus typing: it should be a type error to reference an operation on a ServiceHandler
other than the one that was used to instantiate the client but, before this fix, it was not.
# assert-type-error-pyright: 'No overloads for "execute_operation" match' | ||
await nexus_client.execute_operation( # type: ignore | ||
# assert-type-error-pyright: 'Argument of type .+ cannot be assigned to parameter "operation"' | ||
MyServiceHandler2.my_sync_operation, # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type assertion failed without the fix in this PR
247765d
to
0feac07
Compare
0feac07
to
fd9fd09
Compare
tests/test_client_type_errors.py
Outdated
return WorkflowOutput() | ||
|
||
|
||
async def _test_start_and_execute_workflow_type_errors(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon my lack of understanding of this new type checking stuff mixed in with regular tests. Just browsing the code, it is hard to understand when this code is executed or how it works compared to other tests. Does it run or is it just type-checked? If not, I wonder if a decorator or some other name would be clearer for "test that does not run" rather than just _test
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. I'd prefer not to introduce a decorator that doesn't do anything (in Python since they're actually functions rather than mere annotations I feel that they should do something). But I've renamed the functions as follows:
async def _start_and_execute_workflow_code_for_type_checking_test():
async def _signal_workflow_code_for_type_checking_test():
async def _query_workflow_code_for_type_checking_test():
async def _update_workflow_code_for_type_checking_test():
async def _update_with_start_workflow_code_for_type_checking_test():
and also the file does already have this header:
"""
This file exists to test for type-checker false positives and false negatives.
It doesn't contain any test functions.
"""
Adds a library that dynamically generates test making assertions about expected type errors, and uses it to make assertions about the type errors expected in malformed client calls (nexus and non-nexus).
An example looks like
Note
# assert-type-error-pyright:
comment followed by a regex# type: ignore
on the line with the error. The check will still be made, and the# type: ignore
is required in order for the tests to pass general type-checking.The library used for generating the tests is currently present in both
nexus-rpc
andtemporalio
.