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

std.fs: Improve tests and fix some bugs that were uncovered #16847

Merged
merged 6 commits into from
Aug 17, 2023

Conversation

squeek502
Copy link
Collaborator

@squeek502 squeek502 commented Aug 15, 2023

This PR changes many tests in fs/test.zig to use a helper that will run the test code for each path type that is supported on the target.

For example:

test "openDir" {
    try testWithAllSupportedPathTypes(struct {
        fn impl(ctx: *TestContext) !void {
            const subdir_path = try ctx.transformPath("subdir");
            try ctx.dir.makeDir(subdir_path);

            for ([_][]const u8{ "", ".", ".." }) |sub_path| {
                const dir_path = try fs.path.join(testing.allocator, &[_][]const u8{ subdir_path, sub_path });
                defer testing.allocator.free(dir_path);
                var dir = try ctx.dir.openDir(dir_path, .{});
                defer dir.close();
            }
        }
    }.impl);
}
  • When targeting WASI, this will only run the function once with the relative path type
  • On Linux, it will run twice with both the relative and absolute path type
  • On Windows, it will run three times with relative, absolute, and unc path types.

Doing this also uncovered some Windows bugs in certain functions for certain path types, which have been fixed:

  • UNC paths were not supported in windows.GetFinalPathNameByHandle
  • windows.CreateSymbolicLink/windows.ReadLink did not handle non-relative paths correctly

Tangentially related to #16736 since it will allow us to be confident that the fs.Dir functions can handle all path types.

This fixes a few things:
- Previously, CreateSymbolicLink would always create a relative link if a `dir` was provided, but the relative-ness of a link should be determined by the target path, not the null-ness of the `dir`.
- Special handling is now done to symlink to 'rooted' paths correctly (they are treated as a relative link, which is different than how the xToPrefixedFileW functions treat them)
- ReadLink now correctly supports UNC paths via a new `ntToWin32Namespace` function which intends to be an analog of `RtlNtPathNameToDosPathName` (RtlNtPathNameToDosPathName is not used because it seems to heap allocate as it takes an RTL_UNICODE_STRING_BUFFER)
(which path types will depend on which the target supports)
@andrewrk andrewrk merged commit 8c1329b into ziglang:master Aug 17, 2023
@andrewrk
Copy link
Member

CI failed on master branch:

run test std-native-ReleaseSmall-single: error: 'test.Dir.rename directories' failed: path type: unc
run test std-native-ReleaseSmall-single: error: while executing test 'decltest.buildFseTable', the following test command failed:
C:\actions-runner1\_work\zig\zig\build-release\zig-local-cache\o\39422ac24d75000e506f3254d9545fbf\test.exe --listen=- 
Build Summary: 3037/3177 steps succeeded; 137 skipped; 1 failed; 54896/57798 tests passed; 2901 skipped; 1 failed (disable with --summary none)
test transitive failure
+- test-std transitive failure
   +- run test std-native-ReleaseSmall-single 2442/2568 passed, 1 failed, 125 skipped
error: the following build command failed with exit code 1:

Do you want me to revert it, or do you want to work on a follow-up fix?

@squeek502
Copy link
Collaborator Author

squeek502 commented Aug 18, 2023

I'm unable to reproduce it locally, unfortunately (I have been running the Dir.rename directories test continuously for a while locally and it hasn't failed once for me). Some thoughts:

  • I don't understand why the actual error that was returned wasn't printed anywhere in the log

    zig/lib/std/fs/test.zig

    Lines 119 to 122 in 7ef1eb1

    test_func(&ctx) catch |err| {
    std.debug.print("path type: {s}\n", .{enum_field.name});
    return err;
    };
  • Seems like it might be aarch64-windows specific, since I can't reproduce it on x86_64
  • Seems like it might be intermittent, since the CI passed on this PR branch (and now on master too)
  • The UNC path type in these tests were something of an experiment, as I'm not fully sure how reliable the C:\ -> \\localhost\C$\ transformation actually is

If fails again, I'll submit a PR to disable/remove the UNC path type from these tests (unless you'd like me to do that now anyway).

@squeek502
Copy link
Collaborator Author

squeek502 commented Sep 12, 2023

@andrewrk the intermittent aarch64-windows failure is still happening. I thought it would go away after #16717 (since it successfully mitigates the intermittent builtin.zig AccessDenied we were hitting), but it doesn't seem to have affected this case. Here's what I know about this failure:

  • jacobly was able to find a reproduction that doesn't depend on aarch64 (basically, just run the test code in a loop for a thousand iterations, it's fairly likely to fail on one of them): https://discord.com/channels/605571803288698900/906306963942543370/1143156919910027294
  • The reproduction only works when the "Real-time protection" of the Windows "Virus & threat protection" is enabled on the system
  • It seems to only affect the unc path type in CI, but that may just be a coincidence (it's not the case for the local reproduction)

So, I'm unsure what to do. It's essentially a 'real' failure (in that the rename in the test case could fail in the same way in real code), and therefore I'm not totally sure what the best way to go about mitigating it would be.

@andrewrk
Copy link
Member

Thanks for looking into it. I think the standard procedure here works fine - file an issue to track the bug, and then disable the failing test case, with a link to the open bug report. Having non-flaky tests is more important than coverage for any one test in particular - especially if the implementation has a real bug.

"steps to reproduce" in this bug report will be particularly valuable (and perhaps should contain the contents of @jacobly0's script)

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.

2 participants