[go: up one dir, main page]

Skip to content
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

TEST: Add xfailing test for [func-returns-value] #17834

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

harahu
Copy link
@harahu harahu commented Sep 26, 2024

Add an xfailing test that reproduces #14179.

@harahu
Copy link
Author
harahu commented Sep 26, 2024

Is there a canonical way to point back to the github issue from the test code, so that if the test passes at some point, it is easy to also close the issue?

@brianschubert
Copy link
Contributor

Is there a canonical way to point back to the github issue from the test code, so that if the test passes at some point, it is easy to also close the issue?

I don't know about a canonical way, but any comment that points to the issue number / URL would probably be fine.
There's some existing test cases that do this in various ways.

@harahu
Copy link
Author
harahu commented Sep 26, 2024

Is there a canonical way to point back to the github issue from the test code, so that if the test passes at some point, it is easy to also close the issue?

I don't know about a canonical way, but any comment that points to the issue number / URL would probably be fine. There's some existing test cases that do this in various ways.

I added the URL as a comment.

Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
@brianschubert
Copy link
Contributor
brianschubert commented Sep 26, 2024

I think this test would be better suited for check-expressions.test. There's a block of similar cases with the prefix testNoneReturn.

check-errorcodes.test is for tests related to error codes themselves.

@brianschubert
Copy link
Contributor

This issue appear to affect any decorated function, so there may be value in including a test using an arbitrary dummy decorator, and with multiple levels of decoration.

For your consideration, please feel free to adapt as you see fit:

# test-data/unit/check-expressions.test
[case testNoneReturnDecorated]
from typing import Any, Callable, TypeVar

F = TypeVar('F', bound=Callable[..., Any])

def deco(f: F) -> F:
    pass

@deco
@deco
def f() -> None: 
    pass

class A:
    @staticmethod
    def s() -> None:
        pass

if int():
    x = f()      # E: "f" does not return a value (it only ever returns None)
if int():
    x = A.s()    # E: "s" of "A" does not return a value (it only ever returns None)
if int():
    x = A().s()  # E: "s" of "A" does not return a value (it only ever returns None)
[builtins fixtures/staticmethod.pyi]

@harahu
Copy link
Author
harahu commented Sep 27, 2024

I think this test would be better suited for check-expressions.test. There's a block of similar cases with the prefix testNoneReturn.

check-errorcodes.test is for tests related to error codes themselves.

Moved and renamed the test.

This issue appear to affect any decorated function, so there may be value in including a test using an arbitrary dummy decorator, and with multiple levels of decoration.

Good catch! I expanded the test based on your example.

EDIT: Also added an xfailing test for the __call__ method.

Copy link
Contributor
@brianschubert brianschubert left a comment

Choose a reason for hiding this comment

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

Nice

The __call__ case is interesting. There are some other tests that cover opaque callables in other kinds of expressions, like f = A(); y = f(). It seems that the relevant bit here is having the callee be a CallExpr

A few nits on the error messages:

test-data/unit/check-expressions.test Outdated Show resolved Hide resolved
test-data/unit/check-expressions.test Outdated Show resolved Hide resolved
test-data/unit/check-expressions.test Outdated Show resolved Hide resolved
harahu and others added 3 commits September 27, 2024 18:38
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
Copy link
Contributor
@brianschubert brianschubert left a comment

Choose a reason for hiding this comment

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

LGTM

Both tests pass when I force all function calls to always return None.

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.

2 participants