-
Notifications
You must be signed in to change notification settings - Fork 862
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 platform tests #3285
Fix platform tests #3285
Conversation
/platform_tests ref=fix_platform_tests |
/platform_tests ref=fix_platform_tests |
/platform_tests ref=fix_platform_tests |
Job PR-3285-91407e4 is done. |
Job PR-3285-2611808 is done. |
/platform_tests ref=fix_platform_tests |
Job PR-3285-a3d5e4f is done. |
1 similar comment
Job PR-3285-a3d5e4f is done. |
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.
Thank you for looking into this!
Added some comments/suggestions, but feel free to merge if my suggestion doesn't work.
732e681
to
83ec4c0
Compare
/platform_tests ref=fix_platform_tests |
/platform_tests ref=fix_platform_tests |
Job PR-3285-732e681 is done. |
Job PR-3285-83ec4c0 is done. |
/platform_tests ref=fix_platform_tests |
Job PR-3285-0dd9473 is done. |
Job PR-3285-ecbd613 is done. |
/platform_tests ref=fix_platform_tests |
/platform_tests ref=fix_platform_tests |
Job PR-3285-2026846 is done. |
Issue #, if available:
Description of changes:
str(tempfile)
doesn't give a string but still the object itself for some reason causing issue on Windows. Tried to usetempfile.name
instead but the underlying issue withTemporaryFile
is that it opens the file automatically, while our saving logic tries to open it again. This will fail on Windows. Change to create a tempdir insteadBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.