-
Notifications
You must be signed in to change notification settings - Fork 739
Fix: copy recursively from read only location. (fixes #98) #555
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
Conversation
@Fransz thanks so much for the PR! Can you investigate the windows failure? This may be due to chmod not working correctly on Windows. If so, feel free to skip the tests on Windows. Can you rewrite the tests to not modify Can you please add a test for an individual read-only file as well? |
Tests are adjusted, and skipped on windows as asked by @nfischer I'm not sure to change the file permissions on the copied directories/files. |
@Fransz can you also verify that the permissions are still correct after copying? This should do the trick: assert.equal(fs.statSync('tmp/cp_cp').mode & parseInt('777', 8), parseInt('555', 8)); |
I've added assertions for the permissions being correct after copying as @nfischer requested. The test fails on a single job (not the complete travis build); I've tried several times. The only thing in common seems to be that all failures happend on OS X jobs. I'm new to travis, so i really do not know how to fix this. |
@Fransz I'll look into Travis. Sounds like flakiness, either in the test or in Travis. I'll ping this if there's any action we need from you. Thanks for pointing it out! |
To me, this doesn't look like a bug that is due to Travis. I can't reproduce it locally though, so I restarted the whole build. If it passes with no failures, we can merge now and fix it the next time the failure arises. Otherwise we'll investigate further. I may mark the test with a TODO to examine the flakes later. |
@nfischer, i rewrote the tests in ava. |
@Fransz thanks for doing the refactor. It looks like other tests in I'm going to see if I can resolve the general flakiness, and then I'll try to merge PR in. |
Bumping this back to v0.8, since we're focusing efforts there. |
Bumping to v0.9 milestone |
Unfortunately, this has sat unmerged for a long time. Looking at the code, this doesn't seem to be too hard to cherry pick to a new branch. I'll try this out. |
Close in favor of #870. Thanks for this fix! |
I think this fixes #98
Tested on OS X, but not on windows/linux.
A test was added.