-
-
Notifications
You must be signed in to change notification settings - Fork 680
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
✅ Add CI configs to run tests on Windows and MacOS #824
Conversation
There's one remaining issue on Windows, it happens in most of the [UPDATE: see "Windows encoding issue with |
📝 Docs preview for commit 07d593b at: https://50422059.typertiangolo.pages.dev |
Ok, the Windows & MacOS-specific issues seem to be addressed, but now the coverage report has broken 😭
I continue investigating... [UPDATE: fixed by using |
📝 Docs preview for commit 88d7aa0 at: https://f06c393d.typertiangolo.pages.dev |
📝 Docs preview for commit 5dec753 at: https://b93386d7.typertiangolo.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, you're good 🙇 😎
Impeccable job with all this! ✨
And all the data and references in the description... :chef_kiss: (we need that chef kiss emoji in GitHub).
Just a tiny nitpick, mainly to be annoying. 😅
📝 Docs preview for commit 324d1fb at: https://15f240fb.typertiangolo.pages.dev |
Amazing job, thank you! 🙇 🚀 |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Extend test suite:
In total, this runs the CI two more times than before (7 instead of 5).
Fixes
To get the CI to go green for all systems, following edits/fixes were required:
Ensure we're always reading in UTF-8
tests/test_tutorial/test_parameter_types/test_file/test_tutorial004(_an)
failed on Windows:To fix it, I set the
encoding
forread_text
to beutf8
.Skip completion tests for Windows and MacOS
This isn't the cleanest of solutions, but it seems to me that these completion tests were very much written to work on Linux/bash only. In the future, it'd be nice to have MacOS & Windows compatible tests for this - I can make an issue for it to follow up later?
Use OS in coverage report name
By including the OS in the
coverage
file, we prevent overwriting / confusion:Windows encoding issue with
coverage run
Added an
env
argument tosubprocess.run
calls that were failing for thetest_commands/test_help
test_script
tests. I'm still not clear why we were gettingUnicodeDecodeError
's for these. I double checked all files in thetyper
codebase and couldn't find any with a wrong decoding. Something must be happening when callingcoverage run
. Either way, by taking inspiration from cpython#105312 and chaiNNer#2815 it seems to be fixed.MacOS issue with Python dir caching
The Python installation is being cached with
actions/cache@v3
, which refers toenv.pythonLocation
. However on macOS, this directory appears to be empty. The "Post Run" log would mentionand then the next run would appear to "successfully" restore from this cache, but the commands wouldn't actually be available:
Cf also a related discussion actions/setup-python#813. I decided to implement the same workaround as microsoft/torchgeo#2024, basically disabling the caching for MacOS.
Windows issue with Python dir caching
One remaining issue is to investigate why the Windows caching isn't currently working:
This is not necessary to get this PR green, so I suggest to keep this as follow-up work.
Fix combining coverage files from different OS's
When combining the different
.coverage.XXX
files from the different runs in the matrix,coverage
needs to remap the locations. It's easier to just use therelative_files
option as suggested in the Coverage docs.