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 tests for windows #318

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

fix tests for windows #318

wants to merge 3 commits into from

Conversation

dllliu
Copy link
Collaborator

@dllliu dllliu commented May 21, 2024

Add delete=false parameter to temporary file objects in order to fix permission denied errors when running unit tests for windows and flush buffer in memory after data is written and before new data is written in order to fix invalid zip file errors

Copy link
Collaborator

@gtfierro gtfierro left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple small changes needed

Copy link
Collaborator Author

@dllliu dllliu left a comment

Choose a reason for hiding this comment

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

make tests standardized in flushing buffer

@dllliu dllliu requested a review from gtfierro July 1, 2024 16:12
@gtfierro
Copy link
Collaborator

gtfierro commented Jul 1, 2024

@MatthewSteen you develop mostly on Windows, right? Can you give this a test on your machine to see if it works?

@MatthewSteen
Copy link
Member

@MatthewSteen you develop mostly on Windows, right? Can you give this a test on your machine to see if it works?

Windows machine, but this repo in on the Ubuntu 22 side. I'll take a look.

@MatthewSteen
Copy link
Member

Passes on WSL but I'm not sure if that confirms anything. I think if it passes on Daniel's Windows machine (not WSL), then the issue could be considered fixed.

image

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

3 participants