std.fs: tests for makePath and symlinks #17499
Conversation
|
Thanks for the links, I'm happy to put my test cases into #16878 if you'd rather see that fix. I find the flow control with the inline named block harder to reason about ... and its not clear to me that #16878 handles the case where statFile() returns FileNotFound (which I believe is the error returned from statFile() when a dangling symlink is found). I guess this would be made clear one way or another by the test cases. :) After fixing some embarassing fmt and wasi platform errors, I'm still seeing an odd (but consistent) failure on aarch64-windows. I'll look into those. I guess I need to figure out how to run tests locally. I'll try digging into the fstatat bug a bit too. Mostly a good exercise for me to learn about the syscall wrapping works in Zig (vs. actually being finding the root cause), but I'll update the PR with anything I learn. |
I don't think we can say for certain that |
Ah, yeah, it could be a race with a parallel delete. Is that sort of race condition something makePath() should worry about though? I'm not sure that the behavior of this change (failing the makePath() operation) is unacceptable. Wouldn't that happen anyway in many cases? E.g., if I makePath("a/b/c"), and something else deletes "b" at just the wrong time, the makePath() attempt to create "c" would fail obtusely? I can tone down my overconfident comment about the FileNotFound a bit though. You're right its not the only possibility, but its probably good enough... |
|
I'm just saying converting |
|
I've cherry-picked most of the tests from this PR into #18453. #17702 being merged means that both this and #16878 no longer need to skip any tests when targeting GNU libc (excellent work on that, btw, it'll be a huge relief to not have to worry about Note that neither this nor #16878 is currently sufficient to resolve #17330, since resolving it on Windows is going to take a bit more effort (see #17330 (comment)). So, my current thoughts are:
Let me know if that sounds okay to you. |
|
Sounds like a good plan. Once the dust settles a bit, I'll trim this down to the test dangling symlink test. Thanks for getting the other tests merged and isolating the Windows problems! |
The test runner uses "." in its output between the test module and the test name, so quote the leading '.' in these test names to make them easier to read.
Because creation of a symlink can fail on Windows with an Access Denied error (https://learn.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links) any tests that need a symbolic link "skip" if they run into this problem. This change factors out a "setupSymbolicLink()" routine to make this clearer, a bit tighter, and easier to use in future tests. I also collapsed the "symlink in parent directory" test into the existing "Dir.readlink" test, because the latter uses the more comprehensive testWithAllSupportedPathTypes wrapper.
Create a dangling symlink and check that statFile works with it.
2ec2b12 to
ff56138
Compare
|
@squeek502 Can you give this a quick look over? All that really remained from this CL after separating out the makepath fix and the already-present tests was a bunch of misc cleanup. Add to that some cleanup I inserted when trying to get these tests to pass on Wine (that's not happening). Not sure if I should resurrect this on this PR, or close this PR and start a new one. Thanks! |
|
Test changes look good to me, I'd say mark this as ready for review and it can be merged as-is. We can tackle fixing #17330 in a separate PR. |
Refactor tests around makePath and symlinks.