-
Notifications
You must be signed in to change notification settings - Fork 774
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
Remove unneeded and buggy stats check #976
Conversation
As per nodejs/node#39372 (comment) Resolves #918
I have been trying to write a test case for this; I cannot get to a place where this condition is ever reached. Do we need this at all? Or what am I missing? |
@RyanZim thank you for your efforts! I really appreciate it! This is basically a wild edge case where we're trying to avoid broken copy when src is a link and its resolved path is inside the dest directory. We already have this test node-fs-extra/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js Lines 282 to 296 in e6a9505
so, what's wrong with this test? |
@manidlou The problem is that that test doesn't actually exercise this condition. We'd expect the error to be of the format:
Instead, if you insert a log statement in that test, the actual error thrown is:
|
I'm going to merge this as-is, without an explicit test, as it's certainly better than what we have now. However, we should still look into properly testing this later. |
Thank you @RyanZim! That makes sense. |
As per nodejs/node#39372 (comment); haven't gotten a response from @bcoe there yet
Resolves #918
Hope this logic is right.