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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -140,26 +140,29 @@ jobs:
with:
file: "pyproject.toml"
field: "tool.typeshed.pyright_version"
- uses: jakebailey/pyright-action@v1
- name: Run pyright with basic settings on all the stubs
uses: jakebailey/pyright-action@v1
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 :)

- uses: jakebailey/pyright-action@v1
- name: Run pyright with stricter settings on some of the stubs
uses: jakebailey/pyright-action@v1
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.testcases.json
- uses: jakebailey/pyright-action@v1
project: ./pyrightconfig.stricter.json
- name: Run pyright on the test cases
uses: jakebailey/pyright-action@v1
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.testcases.json

stub-uploader:
name: Run the stub_uploader tests
Expand Down
4 changes: 4 additions & 0 deletions pyrightconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
"stdlib",
"stubs",
],
"exclude": [
// test cases use a custom config file
"stubs/**/@tests/test_cases"
],
"typeCheckingMode": "basic",
"strictListInference": true,
"strictDictionaryInference": true,
Expand Down
2 changes: 2 additions & 0 deletions pyrightconfig.stricter.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
"stubs",
],
"exclude": [
// test cases use a custom pyrightconfig file
"stubs/**/@tests/test_cases",
"stdlib/distutils/command",
"stdlib/lib2to3/refactor.pyi",
"stdlib/_tkinter.pyi",
Expand Down
1 change: 1 addition & 0 deletions pyrightconfig.testcases.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"typeshedPath": ".",
"include": [
"test_cases",
"stubs/**/@tests/test_cases"
],
"typeCheckingMode": "strict",
// Using unspecific "type ignore" comments in test_cases.
Expand Down
1 change: 0 additions & 1 deletion stubs/invoke/@tests/test_cases/check_task.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# pyright: reportUnnecessaryTypeIgnoreComment=true
from __future__ import annotations

from invoke import Context, task
Expand Down
1 change: 0 additions & 1 deletion stubs/protobuf/@tests/test_cases/check_struct.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# pyright: reportUnnecessaryTypeIgnoreComment=true
from __future__ import annotations

from google.protobuf.struct_pb2 import ListValue, Struct
Expand Down
2 changes: 0 additions & 2 deletions stubs/regex/@tests/test_cases/check_finditer.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# pyright: reportUnnecessaryTypeIgnoreComment=true

from __future__ import annotations

from typing import List
Expand Down
2 changes: 0 additions & 2 deletions stubs/requests/@tests/test_cases/check_post.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# pyright: reportUnnecessaryTypeIgnoreComment=true

from __future__ import annotations

from collections.abc import Iterable
Expand Down
16 changes: 7 additions & 9 deletions tests/check_consistent.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,24 +65,22 @@ def check_stubs() -> None:
allowed = {"METADATA.toml", "README", "README.md", "README.rst", "@tests"}
assert_consistent_filetypes(dist, kind=".pyi", allowed=allowed)

tests_dir = dist / "@tests"
if tests_dir.exists() and tests_dir.is_dir():
py_files_present = any(file.suffix == ".py" for file in tests_dir.iterdir())
error_message = "Test-case files must be in an `@tests/test_cases/` directory, not in the `@tests/` directory"
assert not py_files_present, error_message


def check_test_cases() -> None:
for package_name, testcase_dir in get_all_testcase_directories():
for _, testcase_dir in get_all_testcase_directories():
assert_consistent_filetypes(testcase_dir, kind=".py", allowed={"README.md"}, allow_nonidentifier_filenames=True)
bad_test_case_filename = 'Files in a `test_cases` directory must have names starting with "check_"; got "{}"'
for file in testcase_dir.rglob("*.py"):
assert file.stem.startswith("check_"), bad_test_case_filename.format(file)
with open(file, encoding="UTF-8") as f:
lines = {line.strip() for line in f}
assert "from __future__ import annotations" in lines, "Test-case files should use modern typing syntax where possible"
if package_name != "stdlib":
pyright_setting_not_enabled_msg = (
f'Third-party test-case file "{file}" must have '
f'"# pyright: reportUnnecessaryTypeIgnoreComment=true" '
f"at the top of the file"
)
has_pyright_setting_enabled = "# pyright: reportUnnecessaryTypeIgnoreComment=true" in lines
assert has_pyright_setting_enabled, pyright_setting_not_enabled_msg


def check_no_symlinks() -> None:
Expand Down