Skip to content

Improve pyright verification of third-party test cases in CI #9650

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

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Feb 1, 2023

As seen in #9623, following some recent changes to our pyright config, pyright no longer verifies our checks against false-negative errors that we have in our third-party test cases. (Our standard-library test cases are fine.) Luckily, the latest release of pyright means that we can use the same pyright config for our third-party test cases as we do for our standard-library test cases, by using wildcards in include and exclude patterns. This means that we can remove a no-longer-necessary test from check_consistent.py, and it means that we no longer have to have # pyright: reportUnnecessaryTypeIgnoreComment=true comments at the top of every third-party test-case file.

Also in this PR:

  • Add a new test to check_consistent.py, ensuring that .py files are only found in the @tests/test_cases/ directory, and not at the top level of the @tests/ directory. This isn't necessarily obvious, and caused confusion in protobuf: Annotate well_known_types.pyi #9323.
  • Change the workflow file so that the pyright configs are run in a more logical order. The old order was: "strict config first on a subset of the stubs; then, if that succeeds, do the test-cases config on the stdlib test cases; then, if that succeeds, run the basic config on all the stubs". The new order is: "run the basic config first on all the stubs; then, if that succeeds, run the strict config on a subset of the stubs; then, if that succeeds, run the test-case config on the stdlib and third-party test cases".

Co-authored-by: Avasam samuel.06@hotmail.com

Co-authored-by: Avasam <samuel.06@hotmail.com>
@AlexWaygood AlexWaygood marked this pull request as ready for review February 1, 2023 15:27
@AlexWaygood
Copy link
Member Author

Here's a failing run showing that, with these changes, pyright will once again error for unused type: ignore comments in third-party test-case files: https://github.com/python/typeshed/actions/runs/4065870433/jobs/7001171534

@AlexWaygood AlexWaygood changed the title Improve pyright verification of test cases in CI. Improve pyright verification of third-party test cases in CI. Feb 1, 2023
@AlexWaygood AlexWaygood changed the title Improve pyright verification of third-party test cases in CI. Improve pyright verification of third-party test cases in CI Feb 3, 2023
with:
version: ${{ steps.pyright_version.outputs.value }}
python-platform: ${{ matrix.python-platform }}
python-version: ${{ matrix.python-version }}
no-comments: ${{ matrix.python-version != '3.10' || matrix.python-platform != 'Linux' }} # Having each job create the same comment is too noisy.
project: ./pyrightconfig.stricter.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think running stricter first would fail faster in most cases. Since they both run on all files not excluded by the stricter settings. Stricter runs on less files and a failure with basic config would also result in a failure with stricter ones.

But this job is also pretty fast anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I've found it pretty annoying in the past when we're working to enable new pyright lints, the way you have to play "pyright whackamole" with our current setup. You think you've got all the pyright errors sorted, but then you find out, oh no, it was only the strict-config errors you've got sorted! Most of typeshed is still to go, it's just we never even got to the second step 😶

This order feels more intuitive to me :)

@AlexWaygood AlexWaygood merged commit 6078927 into main Feb 7, 2023
@AlexWaygood AlexWaygood deleted the pyright-testcases branch February 7, 2023 11:53
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.

3 participants