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

Fix stdlib/disutils testing #9734

Merged
merged 14 commits into from
Feb 21, 2023
Merged

Fix stdlib/disutils testing #9734

merged 14 commits into from
Feb 21, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Feb 14, 2023

In hindsight the solution was quite simple: Just don't have setuptools installed in a test environment.
Turned on stubtest for distutils for python 3.10+
Fixed enough stubtest to get the CI green.

Missing variables and functions were added with stubgen. Then filled in necessary types for pyright to pass.

Solves one of the two concerns raised in #9520 (namely distutils being a mess to test, and untestable on 3.10+)
As for the second one: distutils shouldn't be imported with setuptools>=60 installed. But even if we wanted to support that use-case, we could.

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 14, 2023

In its current state, this PR shows that the tests are run correctly on all version. Which I wanted to demo.
But I could split off stubtest allowlist entries fixes into a different PR, and add a few entries for now just to get the CI green.

.github/workflows/daily.yml Outdated Show resolved Hide resolved
.github/workflows/stubtest_stdlib.yml Outdated Show resolved Hide resolved
tests/stubtest_stdlib.py Outdated Show resolved Hide resolved
stdlib/distutils/cmd.pyi Outdated Show resolved Hide resolved
stdlib/distutils/core.pyi Outdated Show resolved Hide resolved
stdlib/distutils/core.pyi Outdated Show resolved Hide resolved
stdlib/distutils/cygwinccompiler.pyi Outdated Show resolved Hide resolved
stdlib/distutils/dist.pyi Outdated Show resolved Hide resolved
stdlib/distutils/fancy_getopt.pyi Outdated Show resolved Hide resolved
stdlib/distutils/fancy_getopt.pyi Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM. @hauntsaninja, does this seem like a reasonable approach to you? It involves adding a little bit of complexity to stubtest_stdlib.py, but I think it's worth it.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

(only looked at stubtest_stdlib)

Approach looks fine, but let's remove the return code logic from the pip commands. We only care about preserving the stubtest return code, we don't care about pip's return codes. I'd also not capture output from those installs.

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 21, 2023

I see pip's output is verbose enough printing our own error message is redundant
image

@Avasam Avasam requested a review from hauntsaninja February 21, 2023 06:48
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pip (https://github.com/pypa/pip)
+ src/pip/_internal/locations/_distutils.py:60: error: Unused "type: ignore" comment

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, changes to stubtest_stdlib.py lgtm!

@AlexWaygood AlexWaygood dismissed hauntsaninja’s stale review February 21, 2023 08:05

Requested changes were made

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you!

@AlexWaygood AlexWaygood merged commit a6c6bc1 into python:main Feb 21, 2023
@AlexWaygood
Copy link
Member

I'd also not capture output from those installs.

Hmm, are you sure about that @hauntsaninja? The output from pip is now pretty noisy when I run stubtest_stdlib.py locally (which I do quite a lot):

(.venv) C:\Users\alexw\coding\typeshed>python tests/stubtest_stdlib.py
Collecting mypy==1.0
  Using cached mypy-1.0.0-cp311-cp311-win_amd64.whl (8.8 MB)
Collecting typing-extensions>=3.10
  Using cached typing_extensions-4.5.0-py3-none-any.whl (27 kB)
Collecting mypy-extensions>=0.4.3
  Using cached mypy_extensions-1.0.0-py3-none-any.whl (4.7 kB)
Installing collected packages: typing-extensions, mypy-extensions, mypy
Successfully installed mypy-1.0.0 mypy-extensions-1.0.0 typing-extensions-4.5.0

[notice] A new release of pip available: 22.3.1 -> 23.0.1
[notice] To update, run: C:\Users\alexw\AppData\Local\Temp\tmppzy1r9wi\Scripts\python.exe -m pip install --upgrade pip
Found existing installation: setuptools 65.5.0
Uninstalling setuptools-65.5.0:
  Successfully uninstalled setuptools-65.5.0
C:\Users\alexw\AppData\Local\Temp\tmppzy1r9wi\Scripts\python.exe -m mypy.stubtest --check-typeshed --custom-typeshed-dir . --allowlist tests\stubtest_allowlists\py3_common.txt --allowlist tests\stubtest_allowlists\py311.txt --allowlist tests\stubtest_allowlists\win32.txt --allowlist tests\stubtest_allowlists\win32-py311.txt
Success: no issues found in 484 modules
stubtest succeeded

I'd find the output easier to parse if we suppressed pip's prints to the terminal (other than when pip errors out, obviously).

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