Skip to content

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

Merged
tiangolo merged 23 commits intofastapi:masterfrom
svlandeg:feature/windows_ci
May 14, 2024
Merged

✅ Add CI configs to run tests on Windows and MacOS#824
tiangolo merged 23 commits intofastapi:masterfrom
svlandeg:feature/windows_ci

Conversation

@svlandeg
Copy link
Copy Markdown
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 torchgeo/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
Copy Markdown
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 ☝️]

@github-actions

This comment was marked as outdated.

@svlandeg
Copy link
Copy Markdown
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
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@svlandeg svlandeg marked this pull request as ready for review May 13, 2024 16:33
Copy link
Copy Markdown
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. 😅

Comment thread tests/test_completion/test_completion.py Outdated
@github-actions
Copy link
Copy Markdown
Contributor

@tiangolo tiangolo merged commit b751a13 into fastapi:master May 14, 2024
@tiangolo
Copy link
Copy Markdown
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