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

fsutil write_file atomic=True destroys created file on exit #121

Open
williwacker opened this issue May 4, 2024 · 13 comments
Open

fsutil write_file atomic=True destroys created file on exit #121

williwacker opened this issue May 4, 2024 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@williwacker
Copy link

williwacker commented May 4, 2024

Python version
3.10

Package version
latest

Current behavior (bug description)
write_file atomic=True destroys the created file on exit.
Obviously fsutil is not designed for running on Windows. Cannot handle directory names etc.

Expected behavior
created file should stay

Test Results:
pytest_result.txt

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@williwacker williwacker added the bug Something isn't working label May 4, 2024
@fabiocaccamo
Copy link
Owner

@williwacker actually all tests run successfully even on Windows.
Can you help me to figure how to make the CI fail (when it should) when running on Windows?

@williwacker
Copy link
Author

Hi Fabio,
I am wondering how the test can run on your windows machine where windows paths are always created wit backslashes and the tests comparing the given paths (using forward slashes) with the created ones are different.
I cannot say what is different on my system from yours.
My suggestion would be calling Pathlib.PurePath(path).as_posix(). With this approach
paths are always with forward slashes.
Regards Werner

@williwacker
Copy link
Author

How I was testing: copied the testcase file into my temp folder and started pytest from this folder.

@fabiocaccamo
Copy link
Owner

@williwacker I have not a Windows machine, I run tests locally using tox and remotely on Linux / MacOS / Windows using the GitHub CI.

The tests are based on unittest and not on pytest.

For testing, please follow the instructions in the README testing section.

@williwacker
Copy link
Author

williwacker commented May 13, 2024

@fabiocaccamo,
Simular result when running unittest.
unittest.txt

@fabiocaccamo
Copy link
Owner

@williwacker thank you for the feedback, this is probably just a tests problem because paths are hardcoded, I will try to fix tests as soon as possible and ask you to check again.

Anyway, I don't understand why the GitHub CI doesn't fail when running on Windows.

@fabiocaccamo
Copy link
Owner

@williwacker I improved some tests for Windows compatibility, could you please pull from master, run tests and send me the errors report?

@williwacker
Copy link
Author

Hi @fabiocaccamo,
Much better but not perfect yet. I guess the permission test cases can be ignored on windows, but the atomic write file tests are important.
Thanks
unittest.result.txt

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented May 16, 2024

@williwacker exactly what I expected, I also agree with skipping permission test cases on Windows.
I will try to figure out the atomic write file tests issue (if you have any hint let me know).
Thanks for the updated errors report.

@fabiocaccamo
Copy link
Owner

@williwacker I would ask you to pull from master and test it again.

All tests should succeed now (permissions tests skipped on Windows), except atomic write file tests that should still fail, but I would ask you to do a couple of tests locally:

@williwacker
Copy link
Author

Hi @fabiocaccamo,
have tested the new code with both delete=true and false and have pasted the output in here.
Thanks
test_result_delete_false.txt
test_result_delete_true.txt

fabiocaccamo added a commit that referenced this issue Oct 16, 2024
@fabiocaccamo
Copy link
Owner

@williwacker I improved the code for Windows compatibility, could you please pull from master, run tests and send me the errors report?

@fabiocaccamo fabiocaccamo moved this from Todo to In Progress in Open Source Oct 17, 2024
@williwacker
Copy link
Author

@fabiocaccamo I have run the tests on my windows machine and here is the test result
unittest_results.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

No branches or pull requests

2 participants