From 184db2654ceea7ae233171effa2581163d05507d Mon Sep 17 00:00:00 2001 From: Anund Date: Thu, 26 Dec 2024 14:48:03 +1100 Subject: [PATCH 1/3] testing: handle execveZ failing during test execution Duplicating a test process via fork does unexepected things. zig build test will hang A test binary created via -Demit-test-exe will run 2 copies of the test suite --- src/Command.zig | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/Command.zig b/src/Command.zig index 2b600979f..09ae86d84 100644 --- a/src/Command.zig +++ b/src/Command.zig @@ -182,9 +182,10 @@ fn startPosix(self: *Command, arena: Allocator) !void { _ = posix.execveZ(pathZ, argsZ, envp) catch null; // If we are executing this code, the exec failed. In that scenario, - // we return a very specific error that can be detected to determine - // we're in the child. - return error.ExecFailedInChild; + // terminate so we don't duplicate the original process + // note: returning to test code from this point would run 2 copies of the test suite + std.debug.print("failed to execveZ as child process, terminating", .{}); + std.process.exit(1); } fn startWindows(self: *Command, arena: Allocator) !void { @@ -722,3 +723,32 @@ test "Command: custom working directory" { try testing.expectEqualStrings("/usr/bin\n", contents); } } + +// Duplicating a test process via fork does unexepected things. +// zig build test will hang +// test binary created via -Demit-test-exe will run 2 copies of the test suite +// +// This test relys on cmd.start -> posix.start terminating the child process rather +// than returning to avoid those two strange behaviours +test "Command: posix fork handles execveZ failure" { + if (builtin.os.tag == .windows) { + return error.SkipZigTest; + } + var td = try TempDir.init(); + defer td.deinit(); + var stdout = try createTestStdout(td.dir); + defer stdout.close(); + + var cmd: Command = .{ + .path = "/not/a/binary", + .args = &.{ "/not/a/binary", "" }, + .stdout = stdout, + .cwd = "/bin", + }; + + try cmd.start(testing.allocator); + try testing.expect(cmd.pid != null); + const exit = try cmd.wait(true); + try testing.expect(exit == .Exited); + try testing.expect(exit.Exited == 1); +} From b2cb80dfbbde72262be7fc00591694673b240331 Mon Sep 17 00:00:00 2001 From: Anund Date: Thu, 26 Dec 2024 23:12:35 +1100 Subject: [PATCH 2/3] testing: move cleanup of execveZ into the test code --- src/Command.zig | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/Command.zig b/src/Command.zig index 09ae86d84..6b42e12a1 100644 --- a/src/Command.zig +++ b/src/Command.zig @@ -182,10 +182,9 @@ fn startPosix(self: *Command, arena: Allocator) !void { _ = posix.execveZ(pathZ, argsZ, envp) catch null; // If we are executing this code, the exec failed. In that scenario, - // terminate so we don't duplicate the original process - // note: returning to test code from this point would run 2 copies of the test suite - std.debug.print("failed to execveZ as child process, terminating", .{}); - std.process.exit(1); + // we return a very specific error that can be detected to determine + // we're in the child. + return error.ExecFailedInChild; } fn startWindows(self: *Command, arena: Allocator) !void { @@ -599,7 +598,7 @@ test "Command: pre exec" { }).do, }; - try cmd.start(testing.allocator); + try cmd.testingStart(); try testing.expect(cmd.pid != null); const exit = try cmd.wait(true); try testing.expect(exit == .Exited); @@ -635,7 +634,7 @@ test "Command: redirect stdout to file" { .stdout = stdout, }; - try cmd.start(testing.allocator); + try cmd.testingStart(); try testing.expect(cmd.pid != null); const exit = try cmd.wait(true); try testing.expect(exit == .Exited); @@ -670,7 +669,7 @@ test "Command: custom env vars" { .env = &env, }; - try cmd.start(testing.allocator); + try cmd.testingStart(); try testing.expect(cmd.pid != null); const exit = try cmd.wait(true); try testing.expect(exit == .Exited); @@ -706,7 +705,7 @@ test "Command: custom working directory" { .cwd = "/usr/bin", }; - try cmd.start(testing.allocator); + try cmd.testingStart(); try testing.expect(cmd.pid != null); const exit = try cmd.wait(true); try testing.expect(exit == .Exited); @@ -724,12 +723,12 @@ test "Command: custom working directory" { } } -// Duplicating a test process via fork does unexepected things. +// Test validate an execveZ failure correctly terminates when error.ExecFailedInChild is correctly handled +// +// Incorrectly handling an error.ExecFailedInChild results in a second copy of the test process running. +// Duplicating the test process leads to weird behavior // zig build test will hang // test binary created via -Demit-test-exe will run 2 copies of the test suite -// -// This test relys on cmd.start -> posix.start terminating the child process rather -// than returning to avoid those two strange behaviours test "Command: posix fork handles execveZ failure" { if (builtin.os.tag == .windows) { return error.SkipZigTest; @@ -746,9 +745,22 @@ test "Command: posix fork handles execveZ failure" { .cwd = "/bin", }; - try cmd.start(testing.allocator); + try cmd.testingStart(); try testing.expect(cmd.pid != null); const exit = try cmd.wait(true); try testing.expect(exit == .Exited); try testing.expect(exit.Exited == 1); } + +// If cmd.start fails with error.ExecFailedInChild it's the _child_ process that is running. If it does not +// terminate in response to that error both the parent and child will continue as if they _are_ the test suite +// process. +fn testingStart(self: *Command) !void { + self.start(testing.allocator) catch |err| { + if (err == error.ExecFailedInChild) { + // I am a child process, I must not get confused and continue running the rest of the test suite. + posix.exit(1); + } + return err; + }; +} From 600eea08cdcf1957373ccab2da8b7f5c4c85d6f3 Mon Sep 17 00:00:00 2001 From: Anund Date: Fri, 27 Dec 2024 11:50:29 +1100 Subject: [PATCH 3/3] testing: point Command.zig at ~more universal external binaries The `Command.zig` tests reach outside the local source tree and look for files on the host os machine. This introduces some portability issues for the tests. The nix build sandbox doesn't include `/usr/bin/env` making it error out when `zig build test` runs `Command.zig` tests as part of a `nix build`. Current ci and local development relies on `nix develop` sharing a host os file system that includes `/usr/bin/env`. Turns out `/tmp` and `/bin/sh` are available in the build sandbox in nix so we swap these in to enable nixpkg builds to include testing ghostty as part of any update cycle. --- src/Command.zig | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/Command.zig b/src/Command.zig index 6b42e12a1..82b48fa18 100644 --- a/src/Command.zig +++ b/src/Command.zig @@ -587,8 +587,8 @@ test "createNullDelimitedEnvMap" { test "Command: pre exec" { if (builtin.os.tag == .windows) return error.SkipZigTest; var cmd: Command = .{ - .path = "/usr/bin/env", - .args = &.{ "/usr/bin/env", "-v" }, + .path = "/bin/sh", + .args = &.{ "/bin/sh", "-v" }, .pre_exec = (struct { fn do(_: *Command) void { // This runs in the child, so we can exit and it won't @@ -629,8 +629,8 @@ test "Command: redirect stdout to file" { .args = &.{"C:\\Windows\\System32\\whoami.exe"}, .stdout = stdout, } else .{ - .path = "/usr/bin/env", - .args = &.{ "/usr/bin/env", "-v" }, + .path = "/bin/sh", + .args = &.{ "/bin/sh", "-c", "echo hello" }, .stdout = stdout, }; @@ -663,8 +663,8 @@ test "Command: custom env vars" { .stdout = stdout, .env = &env, } else .{ - .path = "/usr/bin/env", - .args = &.{ "/usr/bin/env", "sh", "-c", "echo $VALUE" }, + .path = "/bin/sh", + .args = &.{ "/bin/sh", "-c", "echo $VALUE" }, .stdout = stdout, .env = &env, }; @@ -699,10 +699,10 @@ test "Command: custom working directory" { .stdout = stdout, .cwd = "C:\\Windows\\System32", } else .{ - .path = "/usr/bin/env", - .args = &.{ "/usr/bin/env", "sh", "-c", "pwd" }, + .path = "/bin/sh", + .args = &.{ "/bin/sh", "-c", "pwd" }, .stdout = stdout, - .cwd = "/usr/bin", + .cwd = "/tmp", }; try cmd.testingStart(); @@ -718,8 +718,10 @@ test "Command: custom working directory" { if (builtin.os.tag == .windows) { try testing.expectEqualStrings("C:\\Windows\\System32\r\n", contents); + } else if (builtin.os.tag == .macos) { + try testing.expectEqualStrings("/private/tmp\n", contents); } else { - try testing.expectEqualStrings("/usr/bin\n", contents); + try testing.expectEqualStrings("/tmp\n", contents); } }