-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Move Windows tests from oldest to newest version #19545
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
OK, 7 tests failed, LOL: 5 tests involving cc @JukkaL |
I am retrying first, just in case. Just to rule out one-off fluctuation. |
OK, retry did not help, but here is some data:
|
Any chance the ResourceWarning is related to #18145? I really know nothing about Windows quirks, that was just a quick attempt to stop recurring random CI crashes... |
FWIW, we used to test on the oldest Python version because there was the MSVC ABI differences. The ABI has been stable for quite a while (though may change in a few years) so I think it is fine to change it for now. If there is an ABI break we can always move the job back. |
I can try to take a look at the Windows failures later today. It looks like I can reproduce them locally. |
@emmatyping Great, thanks! |
This PR should fix the rest of the test failures in #19545. A change to `os.path.relpath` in 3.13 seems to have broken the handling of Windows paths beginning with `\\`. To resolve this issue, we don't split the drive letter off of the path and instead verify the path is on the current drive. If it isn't it will never resolve to the package root because that must be on the same drive as the CWD: https://github.com/python/mypy/blob/5b03024e829940cf3c3e3d99fc6625f569d02728/mypy/main.py#L1571-L1572 Keeping the drive letter allows relpath to properly generate a relative path and make the tests pass. I need to investigate if the relpath change is a regression in CPython.
This is part of fixing the failed tests in #19545 The async tests previously used many invocations of `asyncio.run`, which likely caused issues with event loop management. The documentation for `asyncio.run` states: > This function cannot be called when another asyncio event loop is running in the same thread. ... > This function should be used as a main entry point for asyncio programs, and should ideally only be called once. Calling `asyncio.run` multiple times could cause the test processes to hang for strange event loop reasons. This commit converts most test cases to be run in a single event loop managed by the default driver, which is now async aware. Not all tests could be converted, e.g. the test that runs an async function in a sync context. However, the test suite does succeed with these changes, and these tests can be further modified if needed.
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This is mostly to speed-up CI, but also as we discussed with Jukka this may make more sense as we want to test new features on more platforms.