unit tests that write files to cwd have racy failures

[andrewrk]

Extracted from #14647.

zig/lib/std/os/test.zig

Lines 24 to 29 in b4d58e9

	test "chdir smoke test" {
	if (native_os == .wasi) return error.SkipZigTest;
	// Get current working directory path
	var old_cwd_buf: [fs.MAX_PATH_BYTES]u8 = undefined;
	const old_cwd = try os.getcwd(old_cwd_buf[0..]);

zig/lib/std/os/test.zig

Lines 173 to 181 in b4d58e9

	test "symlink with relative paths" {
	if (native_os == .wasi and builtin.link_libc) return error.SkipZigTest;
	const cwd = fs.cwd();
	cwd.deleteFile("file.txt") catch {};
	cwd.deleteFile("symlinked") catch {};
	// First, try relative paths in cwd
	try cwd.writeFile("file.txt", "nonsense");

zig/lib/std/os/test.zig

Lines 263 to 276 in b4d58e9

	test "linkat with different directories" {
	if (native_os == .wasi and builtin.link_libc) return error.SkipZigTest;
	switch (native_os) {
	.wasi, .linux, .solaris => {},
	else => return error.SkipZigTest,
	}
	var cwd = fs.cwd();
	var tmp = tmpDir(.{});
	cwd.deleteFile("example.txt") catch {};
	tmp.dir.deleteFile("new.txt") catch {};
	try cwd.writeFile("example.txt", "example");

There are more in this file too.

These tests are all incorrect because they write garbage to the current working directory. This is racy because multiple instances of unit tests may be running at once (with different build options, or for different targets, for example).

An alternative solution to this would be to have the test runner create a new temporary directory for each independent unit test, and change to that directory as the current working directory before each unit test run. But that seems like a waste of (the computer's) time, so probably the tests should just be modified to not do problematic things.

Example failure:

run test std-native-Debug-bare-multi-default: error: 'test.symlink with relative paths' failed: C:\actions-runner\_work\zig\zig\lib\std\os\windows.zig:138:39: 0x7ff61b94f337 in OpenFile (test.exe.obj)
            .OBJECT_NAME_COLLISION => return error.PathAlreadyExists,
                                      ^
C:\actions-runner\_work\zig\zig\lib\std\os\windows.zig:738:21: 0x7ff61bd7c633 in CreateSymbolicLink (test.exe.obj)
        else => |e| return e,
                    ^
C:\actions-runner\_work\zig\zig\lib\std\os\test.zig:196:21: 0x7ff61bd7c07f in test.symlink with relative paths (test.exe.obj)
            else => return err,
                    ^
run test std-native-Debug-bare-multi-default: error: the following test command failed:
C:\actions-runner\_work\zig\zig\build-release\zig-local-cache\o\32dc1975e40afd93fbbf8a70ff27fd84\test.exe --listen=- 

[matu3ba]

1. Afaiu, a process is launched per file and each unit test within a file is run sequential.
2. Thus, the naive solution is to append the process id to all directory and file names (with small chance pids are reused).
3. One way or the other, one needs unique per configuration file names, if test artifacts are supposed to remain in the same paths.
4. chdir will break once the unit tests them-self are parallelized (needs build system annotation what unit test files are to be run parallel and which sequential).

[andrewrk]

1. This is not a valid assumption.
2. Bad idea. Use std.testing.tmpDir instead.
3. The cache system already does this.
4. Unit tests must not chdir unless they undo it before completing.

[matu3ba]

Unit tests must not chdir unless they undo it before completing.

Since chdir affects process global state,

• 1. we need to have a mutex for all relative path-based operations, unless based on Dir or Dir.handle
• 2. parallel executed TmpDir.clean() will break things, because tmpDir is based on

    var cwd = std.fs.cwd(); // <--------------------- offending line: chdir may generate nested tmpDirs
    var cache_dir = cwd.makeOpenPath("zig-cache", .{}) catch
        @panic("unable to make tmp dir for testing: unable to make and open zig-cache dir");
    defer cache_dir.close();
    var parent_dir = cache_dir.makeOpenPath("tmp", .{}) catch
        @panic("unable to make tmp dir for testing: unable to make and open zig-cache/tmp dir");
    var dir = parent_dir.makeOpenPath(&sub_path, opts) catch
        @panic("unable to make tmp dir for testing: unable to make and open the tmp dir");

• 3. I still dont quite understand, if unit tests from multiple files can be run at the same time in the same process by different threads.

Possible solutions:

• 1. move chdir tests into standalone tests
• 2. provide teststartCwd or testStartPath and move over all tmpDir usages to use this in unit tests + use a process-global mutex, where necessary (if 3.1 applies an "in-unit test mode-only" referencable mutex)
• 3. convention to annotate that unit tests of a file must be run single-threaded + configuration option how to run (specific) files in the test runner [allowing for more allows feature creep into something like proposal: improved compiler test suite (compiler errors, run and compare, translate-c, etc.) #11288].

Please let me know, if I am missing anything or what things are wrong.