Skip to content

Improve docs on running stubtest locally #8822

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
Oct 8, 2022

Conversation

AlexWaygood
Copy link
Member

No description provided.

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! Removing the example and the new text looks good, but not clear to me what we're attempting to clarify in the changed text.

tests/README.md Outdated
Thus the easiest way to run this test is via Github Actions on your fork;
if you run it locally, it'll likely complain about system-specific
differences (in e.g, `socket`) that the type system cannot capture.
As such, if you run this test locally, it *may* complain about system-specific
Copy link
Collaborator

@hauntsaninja hauntsaninja Oct 2, 2022

Choose a reason for hiding this comment

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

The original text is more prescriptive about how you should run stubtest, which I think is valuable (I don't think it's a particularly worthwhile or even feasible endeavour to make test_stubtest pass cleanly on all possible configurations of python and system). Also why the extra hedging? It is "likely" that you'd get system-specific complaints and the type system genuinely "cannot capture" most extant system-specific differences. What are the goals of this change?

Copy link
Member Author

@AlexWaygood AlexWaygood Oct 2, 2022

Choose a reason for hiding this comment

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

Well, I've run stubtest_stdlib.py many times locally on my Windows machine, and never once run into an issue where stubtest complained about something locally but didn't in CI. So whether or not it's "likely" that you'll get system-specific complaints would appear itself to be system-specific :)

For me, it's always seemed somewhat strange that these docs recommend running it via Github Actions, when it's so much easier to just run it locally. But maybe the situation's different on Linux/macOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

the type system genuinely "cannot capture" most extant system-specific differences.

I don't think there are that many allowlist entries left that are due to platform-specific differences. And I think there's a solid chance that we could remove a lot of the remaining ones, if somebody has the inclination to put in the necessary work.

Copy link
Collaborator

@hauntsaninja hauntsaninja Oct 2, 2022

Choose a reason for hiding this comment

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

whether or not it's "likely"

Oh okay, yeah that makes sense. For reference, when I run stubtest_stdlib.py I get: Found 139 errors (checked 485 modules).

I don't think there are that many allowlist entries left

The presence of allowlist entries is fine — by definition those don't cause stubtest_stdlib.py to fail. The issue is all the random ways in which local system and local Python can differ from CI, causing stubtest_stdlib.py to fail when run locally.

These can be worked around by adding regexes to stubtest allowlists (which is what we ended up doing with socket) and using the allow-unused-empty-regex trick, but I don't think this work is particularly worthwhile and can result in false negatives. For a dumb example, if someone has a Python build that doesn't include tkinter, there just isn't a great way to avoid stubtest tkinter errors

More generally, stubtest breaks the norm that if you checkout a project and run its tests, and those tests fail, something unusual is happening, so I feel the need to disclaim :-) Fwiw, the wording here was introduced because this pitfall confused some contributors, e.g. #3782 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

which is what we ended up doing with socket

Only on linux these days ;)

Cool, this all makes sense, I'll push a commit to improve this bit

Copy link
Member Author

@AlexWaygood AlexWaygood Oct 2, 2022

Choose a reason for hiding this comment

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

For a dumb example, if someone has a Python build that doesn't include tkinter, there just isn't a great way to avoid stubtest tkinter errors

Yeah, this is definitely true. I guess one thing I don't like hugely about the current wording is the idea that "the type system cannot capture" this stuff. For a lot of this stuff, it's not really the type system (we could probably describe differences between various linux systems more precisely if we were prepared to do stuff like if sys.platform == "freebsd" or whatever). For that kind of thing, it's more "we don't want to get into all of that because it's a fool's errand to try to capture platform-specific minutiae for little-used and unusual platforms".

(Some of it obviously is "the type system" -- it's obviously pretty hard to distinguish between a macOS build with tkinter and one without tkinter.)

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 for working on this! Made a suggestion with a wording nit / explicitly suggesting that opening a PR is really fine, but feel free to ignore and merge.

Co-authored-by: Shantanu <[email protected]>
@hauntsaninja hauntsaninja merged commit 6a2232a into python:master Oct 8, 2022
@AlexWaygood AlexWaygood deleted the stubtest-docs branch October 8, 2022 23:30
@AlexWaygood
Copy link
Member Author

Thanks! :-)

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