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 Windows to Github CI Action #528

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Conversation

jpdakran
Copy link
Member

@jpdakran jpdakran commented Apr 1, 2022

Goal:

  • Add windows-latest to CI github action to ensure tests are passing on all three major platforms before committing to master.

Obstacle:

  • Major differences between how NamedTemporaryFile works on Linux vs Windows
  • Specifically there is a PermissionError when trying to access NamedTemporaryFile on Windows

Solution:

  • Instead of using NamedTemporaryFile directly. Setup a mock NamedTemporaryFile called mock_named_temporary_file where we handle the creation and tear-down. The key here is to setting delete=False. By doing this we can access the NamedTemporaryFile after creation but the file is not deleted. This is why our mock object handles the tear down ensuring all temp files are deleted.

Obstacle:

  • Difference between how TemporaryDirectory works on Linux vs Windows
  • Specifically, there is a bug on windows for python versions < 3.8 where the tear down does not happen properly for TemporaryDirectory

Solution:

  • Expect a failure for the single test case using TemporaryDirectory. We will only expect a failure when the platform is windows and the python version is < 3.8

Obstacle:

  • Path differences between Windows vs Linux
  • Specifically test_data/files/file_with_secrets.py on Linux while test_data\\files\\file_with_secrets.py on Windows

Solution:

  • Leverage Pythons pathlib path module for instantiating paths which handles the platform differences

Obstacle:

  • Parameterized test inputs on windows have a max length of 32767 characters

Solution:

  • For the keyword_test.py, Change the length for the long_line from 40707 characters to 32039 characters

@jpdakran jpdakran force-pushed the dakranj-add-windows-to-ci branch 30 times, most recently from c06ba33 to 90ef5b6 Compare April 5, 2022 15:10
@jpdakran jpdakran force-pushed the dakranj-add-windows-to-ci branch 12 times, most recently from aaf8ecf to a9f4a35 Compare April 5, 2022 18:45
@jpdakran jpdakran marked this pull request as ready for review April 5, 2022 19:44
Copy link
Member

@lorenzodb1 lorenzodb1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants