stubtest_third_party.py: Allow running non-listed platforms locally #9173
Conversation
|
Hmm, I'm not a massive fan of the script magically doing something different in CI due to the presence of an environment variable. I like the idea of a CLI flag more. |
|
Understandable, I figured that might be the case. Btw, failure should be fixed by #9176 |
There was a problem hiding this comment.
Another idea.
Let's say you work on a module that has several supported platforms like psutil. And you want to run several tests locally: for win32, linux, and macos.
Right now there's no way to do it.
With mypy / stubtest you can pass --platform=X as a CLI argum
8000
ent.
Why won't we have the same thing here?
So, if no --platform is passed: we just skip unmatching platforms.
If it is, we use whatever is passed (with a warning that the results might not be accurate).
It doesn't make much sense to pass I have a few nits to sort out (I'll review soon), but I broadly like the design of this PR now. |
|
Yes, it does not make much sense. This is why I've added this part:
But, it is sometimes better than not being able to run stubtest at all. |
There was a problem hiding this comment.
Thanks! I like the design of this PR, as I said. I agree that the least surprising behaviour is if python tests/stubtest_third_party.py foo tests the stubs for foo locally on whatever system you're running on. That's the behaviour we had prior to #8923, and it's the behaviour we should go back to; it's the CI run that should require additional command-line flags.
I think the logic at l
8000
ine 37 can be simplified a bit, and the messages printed can be less verbose -- have offered a suggestion there. I'm also not keen on the name of the flag -- --ci-run isn't very descriptive about what the flag actually does. Maybe --supported-stubs-only?
tests/stubtest_third_party.py
Outdated
| @@ -167,6 +181,7 @@ def main() -> NoReturn: | |||
| parser.add_argument("-v", "--verbose", action="store_true", help="verbose output") | |||
| parser.add_argument("--num-shards", type=int, default=1) | |||
| parser.add_argument("--shard-index", type=int, default=0) | |||
| parser.add_argument("--ci-run", action="store_true") | |||
There was a problem hiding this comment.
It would be nice to add a short description of what the flag does here, using the help= keyword argument
There was a problem hiding this comment.
I agree on the flag name. It's a principle I need to remind myself more often:
"Don't make code that's environment-specific. Instead give options and let the environment decide".
So instead of having a "ci" flag, I should have a flag that describes this option.
I went with "specified" instead of "supported". For the same reason as the multi-platform PR by sobolevn: It's not necessarily unsupported. It's just not tested (it could simply be redundant).
There was a problem hiding this comment.
"Specified" is a great choice!
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Here is my proposed solution for: #8923 (comment)
This should keep the same behaviour on the CI (although with a more accurate skip message when no platform are listed).
While still allowing to run stubtest locally.
I am not immediatly skipping if platforms are listed but is ran on an "unsupported" platform locally, because of the following edge case:
Say a stub is the same on darwin and linux, but different on windows. You don't want to run the CI for all three, so you may specify
platforms = ["linux", "win32"]. But it's still fine to run locally on macos.In this solution, the behaviour is affected by the "CI" environment variable: https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables . But it could be something else like a CLI flag passed by the github action.