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

Add regression tests for builtins.pow and object.__reduce__ #7663

Merged
merged 17 commits into from
Apr 22, 2022

Conversation

AlexWaygood
Copy link
Member

As discussed in #1339 (comment) (among other places).

At the moment, this PR has only pyright running on the test_cases directory. We can potentially add mypy once there's a mypy release that supports assert_type.

@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review April 19, 2022 12:58
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks for setting this up! It's going to be very helpful. I'll send PRs later for my zip_longest cases from the other day.

test_cases/stdlib/test_builtins.py Outdated Show resolved Hide resolved
test_cases/stdlib/test_builtins.py Outdated Show resolved Hide resolved
Comment on lines 14 to 18
100% test coverage for typeshed is neither necessary nor desirable, as it would
lead to unnecessary code duplication. Moreover, typeshed has multiple other
mechanisms for spotting errors in the stubs. As such, this directory should
only contain tests for functions and classes which are known to have caused
problems in the past, where the stubs are difficult to get right.
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this is correct, it might need a tl;dr in bold. As discussed, it's difficult to convince some people that they don't need to write tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

How's it look after b2e59b5?

@@ -2,7 +2,8 @@
"typeshedPath": ".",
"include": [
"stdlib",
"stubs"
"stubs",
"test_cases"
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Do we really want this to be in both pyrightconfigs?
  • Does mypy support assert_type() yet? If it does, we could instead point mypy_primer at typeshed's test_cases. That way we would know if a PR to mypy breaks the tests.

Copy link
Member Author

@AlexWaygood AlexWaygood Apr 19, 2022

Choose a reason for hiding this comment

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

  • I don't really understand how the two pyrightconfigs work with each other ¯\_(ツ)_/¯. Happy to have it in only one if that's better.
  • No, mypy doesn't support assert_type yet. The PR was recently merged, and might make it into 0.950, but also might not. See Release 0.950 planning mypy#12579 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Mypy supports assert_type() on master (I added it last week). I am hoping to convince Jukka to put it in the upcoming 0.950 release. We should make mypy run on this directory too as soon as 0.950 is released, or use master if we're feeling brave.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: assert_type has been cherry-picked onto the 0.950 branch, so we should be able to point mypy_primer at this directory as soon as 0.950 is released.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully we can just run mypy regularly, no need to use mypy-primer.

@hauntsaninja
Copy link
Collaborator

Can we keep the comments in builtins? This is a super common point of clarification and there is value in having the stubs be self-documenting.

Another suggestion could be to move this under tests. Also note we have the @tests directory for each third party stubs, which might be better since the equivalent tests for third party stubs could involve dependencies.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 19, 2022

Can we keep the comments in builtins? This is a super common point of clarification and there is value in having the stubs be self-documenting.

Done (78b9dd0).

Another suggestion could be to move this under tests.

To me, it doesn't really feel like this stuff "fits in" with the other content in tests particularly well -- most of the content there is scripts for running tests, whereas this is test data. But I don't feel that strongly about it; happy to make the change if others agree. 🙂

Also note we have the @tests directory for each third party stubs, which might be better since the equivalent tests for third party stubs could involve dependencies.

Yeah, the third-party stubs would complicate things. Perhaps we could cross that bridge when we come to it?

@AlexWaygood AlexWaygood requested a review from srittau April 19, 2022 17:04
@github-actions

This comment was marked as outdated.

@@ -2,7 +2,8 @@
"typeshedPath": ".",
"include": [
"stdlib",
"stubs"
"stubs",
"test_cases"
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully we can just run mypy regularly, no need to use mypy-primer.

@JelleZijlstra JelleZijlstra merged commit 2773480 into python:master Apr 22, 2022
@AlexWaygood AlexWaygood deleted the regrtests branch April 22, 2022 06:33
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Comment on lines +3 to +5
This directory contains regression tests for the stubs found elsewhere in the
typeshed repo. Each file contains a number of test cases, all of which should
pass a type checker without error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the long term we should probably also allow test that fail a type checker to be able to test the negative case.

Copy link
Member Author

@AlexWaygood AlexWaygood Apr 22, 2022

Choose a reason for hiding this comment

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

Yup. Once we're running mypy on this directory, we can do that by # type: ignoring (with specific error codes) lines that we want to fail and running mypy with --warn-unused-ignores. We can't use that flag globally when running mypy on typeshed, but we should be able to use it for this directory.

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.

5 participants