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

Fix errors on Windows tests in new tests/functional #4767

Merged
merged 3 commits into from
Feb 25, 2022

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Feb 23, 2022

resolves #4781

Fixing test failures in tests/functional/basic on Windows platforms.

Description

Instead of generic 'write' methods, use text write methods with utf-8 encoding.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@gshank gshank requested a review from a team as a code owner February 23, 2022 16:14
@cla-bot cla-bot bot added the cla:yes label Feb 23, 2022
@gshank gshank added test windows test all run tests for all python versions + systems labels Feb 23, 2022
@gshank gshank closed this Feb 23, 2022
@gshank
Copy link
Contributor Author

gshank commented Feb 23, 2022

Tests didn't fail. Also not failing on main now.

@gshank gshank reopened this Feb 24, 2022
@gshank gshank force-pushed the pytest_fix_windows_tables branch 2 times, most recently from 90ee216 to 88e7626 Compare February 24, 2022 21:58
@gshank gshank changed the title Get more information on error Fix errors on Windows tests in new tests/functional Feb 24, 2022
@gshank gshank force-pushed the pytest_fix_windows_tables branch from 88e7626 to 6f90cc3 Compare February 24, 2022 22:31
@gshank gshank requested review from leahwicz and emmyoop February 24, 2022 22:44
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

One small request to document why we're doing this for future us. Could also be worth starting a README with the reasons we do unexpected things.

Comment on lines 218 to 219
with open(path.join(name), "w", encoding="utf-8") as fp:
fp.write(data)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is happening in multiple places. I don't want to lose why we're doing it this way (in 6 months). Could we pull it out to centralize this and add a comment as to why we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea, and much more readable. Have created 'read_file' and 'write_file' functions.

@gshank gshank requested a review from emmyoop February 25, 2022 15:57
Comment on lines +76 to +77
# We need to explicitly use encoding="utf-8" because otherwise on
# Windows we'll get codepage 1252 and things might break
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@gshank gshank merged commit 0dda0a9 into main Feb 25, 2022
@gshank gshank deleted the pytest_fix_windows_tables branch February 25, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes test all run tests for all python versions + systems test windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants