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

✅ Add CI configs to run tests on Windows and MacOS #824

Merged
merged 23 commits into from
May 14, 2024

Conversation

svlandeg
Copy link
Member

@svlandeg svlandeg commented May 3, 2024

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:

AssertionError: assert 'la cig�e�a trae al ni�o' in 'some settings\nla cigüeña trae al niño'

To fix it, I set the encoding for read_text to be utf8.

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:

image

Windows encoding issue with coverage run

Added an env argument to subprocess.run calls that were failing for the test_commands/test_help test_script tests. I'm still not clear why we were getting UnicodeDecodeError's for these. I double checked all files in the typer codebase and couldn't find any with a wrong decoding. Something must be happening when calling coverage 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 to env.pythonLocation. However on macOS, this directory appears to be empty. The "Post Run" log would mention

Cache Size: ~0MB (478 B)

and then the next run would appear to "successfully" restore from this cache, but the commands wouldn't actually be available:

image

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:

image

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 the relative_files option as suggested in the Coverage docs.

@svlandeg svlandeg added the repo / tests Involving the CI / test suite label May 3, 2024
@svlandeg svlandeg linked an issue May 3, 2024 that may be closed by this pull request
1 task
@svlandeg
Copy link
Member Author

svlandeg commented May 3, 2024

There's one remaining issue on Windows, it happens in most of the test_script tests in the test_tutorial/test_commands/test_help directory. I'm not quite sure why there is an issue there specifically and what is causing the UnicodeDecodeError. I'll continue to look into this.

[UPDATE: see "Windows encoding issue with coverage run" section in original post ☝️]

Copy link

📝 Docs preview for commit 07d593b at: https://50422059.typertiangolo.pages.dev

@svlandeg
Copy link
Member Author

svlandeg commented May 13, 2024

Ok, the Windows & MacOS-specific issues seem to be addressed, but now the coverage report has broken 😭

No source for code: '/Users/runner/work/typer/typer/docs_src/arguments/default/tutorial001.py'.

I continue investigating...

[UPDATE: fixed by using relative_files, cf original post ☝️]

@svlandeg svlandeg added the p3 label May 13, 2024
Copy link

📝 Docs preview for commit 88d7aa0 at: https://f06c393d.typertiangolo.pages.dev

Copy link

📝 Docs preview for commit 5dec753 at: https://b93386d7.typertiangolo.pages.dev

@svlandeg svlandeg marked this pull request as ready for review May 13, 2024 16:33
Copy link
Member

@tiangolo tiangolo left a 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. 😅

tests/test_completion/test_completion.py Outdated Show resolved Hide resolved
Copy link

📝 Docs preview for commit 324d1fb at: https://15f240fb.typertiangolo.pages.dev

@tiangolo tiangolo merged commit b751a13 into fastapi:master May 14, 2024
17 checks passed
@tiangolo
Copy link
Member

Amazing job, thank you! 🙇 🚀

@svlandeg svlandeg deleted the feature/windows_ci branch May 15, 2024 07:53
nkabir pushed a commit to rk-forks/typer that referenced this pull request Jul 8, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal p3 repo / tests Involving the CI / test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI configs to run tests on Windows and MacOS
2 participants