From 5c488a1402be83f198cc29f23f448313001a9a1c Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Thu, 4 May 2023 16:34:17 +0200 Subject: [PATCH 1/4] test: improve test case 1 and 2 In test case 1 and 2, remove the `i` variable and use `ex.number()` instead. In test case 2, when checking the exercise output from stderr, also check that stdout is empty and vice versa. --- test/tests.zig | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/test/tests.zig b/test/tests.zig index 752ca50..060b553 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -32,21 +32,23 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { // Test that `zig build -Dhealed -Dn=n test` selects the nth exercise. const case_step = createCase(b, "case-1"); - var i: usize = 0; for (exercises[0 .. exercises.len - 1]) |ex| { - i += 1; + const n = ex.number(); if (ex.skip) continue; const cmd = b.addSystemCommand( - &.{ b.zig_exe, "build", "-Dhealed", b.fmt("-Dn={}", .{i}), "test" }, + &.{ b.zig_exe, "build", "-Dhealed", b.fmt("-Dn={}", .{n}), "test" }, ); - cmd.setName(b.fmt("zig build -Dhealed -Dn={} test", .{i})); + cmd.setName(b.fmt("zig build -Dhealed -Dn={} test", .{n})); cmd.expectExitCode(0); - if (ex.check_stdout) - expectStdOutMatch(cmd, ex.output) - else + if (ex.check_stdout) { + expectStdOutMatch(cmd, ex.output); + cmd.expectStdErrEqual(""); + } else { expectStdErrMatch(cmd, ex.output); + cmd.expectStdOutEqual(""); + } cmd.step.dependOn(&heal_step.step); @@ -60,15 +62,14 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { // Test that `zig build -Dhealed -Dn=n test` skips disabled esercises. const case_step = createCase(b, "case-2"); - var i: usize = 0; for (exercises[0 .. exercises.len - 1]) |ex| { - i += 1; + const n = ex.number(); if (!ex.skip) continue; const cmd = b.addSystemCommand( - &.{ b.zig_exe, "build", "-Dhealed", b.fmt("-Dn={}", .{i}), "test" }, + &.{ b.zig_exe, "build", "-Dhealed", b.fmt("-Dn={}", .{n}), "test" }, ); - cmd.setName(b.fmt("zig build -Dhealed -Dn={} test", .{i})); + cmd.setName(b.fmt("zig build -Dhealed -Dn={} test", .{n})); cmd.expectExitCode(0); cmd.expectStdOutEqual(""); expectStdErrMatch(cmd, b.fmt("{s} skipped", .{ex.main_file})); From 277c95db7af09732f5179677fcbd191e36fefc81 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Thu, 4 May 2023 18:48:35 +0200 Subject: [PATCH 2/4] test: fix doc-comments CheckStep, FailStep, fail, HealStep and heal incorrectly used a normal comment, instead of a doc-comment. Additionally, improve the documentation for FailStep and HealStep. --- test/tests.zig | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/tests.zig b/test/tests.zig index 060b553..dbf237b 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -158,7 +158,7 @@ fn createCase(b: *Build, name: []const u8) *Step { return case_step; } -// Check the output of `zig build` or `zig build -Dn=1 start`. +/// Checks the output of `zig build` or `zig build -Dn=1 start`. const CheckStep = struct { step: Step, exercises: []const Exercise, @@ -282,7 +282,7 @@ const CheckStep = struct { } }; -// A step that will fail. +/// Fails with a custom error message. const FailStep = struct { step: Step, error_msg: []const u8, @@ -311,9 +311,9 @@ const FailStep = struct { } }; -// A variant of `std.Build.Step.fail` that does not return an error so that it -// can be used in the configuration phase. It returns a FailStep, so that the -// error will be cleanly handled by the build runner. +/// A variant of `std.Build.Step.fail` that does not return an error so that it +/// can be used in the configuration phase. It returns a FailStep, so that the +/// error will be cleanly handled by the build runner. fn fail(step: *Step, comptime format: []const u8, args: anytype) *Step { const b = step.owner; @@ -323,7 +323,7 @@ fn fail(step: *Step, comptime format: []const u8, args: anytype) *Step { return step; } -// A step that heals exercises. +/// Heals the exercises. const HealStep = struct { step: Step, exercises: []const Exercise, @@ -353,7 +353,7 @@ const HealStep = struct { } }; -// Heals all the exercises. +/// Heals all the exercises. fn heal(allocator: Allocator, exercises: []const Exercise, work_path: []const u8) !void { const join = fs.path.join; From eda73b2fa8dcb5ca4fc96903f8d1f46ac464acab Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Thu, 4 May 2023 19:01:48 +0200 Subject: [PATCH 3/4] test: remove obsolete comment in the heal function Remove the comment about using POSIX patch variant, since we now use the -s option. --- test/tests.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/tests.zig b/test/tests.zig index dbf237b..1fda49d 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -7,6 +7,7 @@ const fs = std.fs; const mem = std.mem; const Allocator = std.mem.Allocator; +const Child = std.process.Child; const Build = std.build; const FileSource = std.Build.FileSource; const Reader = fs.File.Reader; @@ -363,7 +364,6 @@ fn heal(allocator: Allocator, exercises: []const Exercise, work_path: []const u8 for (exercises) |ex| { const name = ex.name(); - // Use the POSIX patch variant. const file = try join(allocator, &.{ exercises_path, ex.main_file }); const patch = b: { const patch_name = try fmt.allocPrint(allocator, "{s}.patch", .{name}); @@ -373,7 +373,7 @@ fn heal(allocator: Allocator, exercises: []const Exercise, work_path: []const u8 const argv = &.{ "patch", "-i", patch, "-o", output, "-s", file }; - var child = std.process.Child.init(argv, allocator); + var child = Child.init(argv, allocator); _ = try child.spawnAndWait(); } } From 0cd86d2f9b8a372c7c8ab2a253e3643cc2ab6212 Mon Sep 17 00:00:00 2001 From: Manlio Perillo Date: Fri, 5 May 2023 16:13:57 +0200 Subject: [PATCH 4/4] build: add the healed-path option This is necessary in the unit tests, to ensure each test case use a different exercises directory. Update test/tests.zig to use the new healed-path option, ensuring that each temp directory is removed. In test case 3, 4 and 5, move case_step as the first step in the dependency chain. This will improve the build summary tree. In test case 5, remove the dependency to heal_step, since it is not necessary. --- build.zig | 9 ++-- test/tests.zig | 119 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 89 insertions(+), 39 deletions(-) diff --git a/build.zig b/build.zig index 64968b9..a880450 100644 --- a/build.zig +++ b/build.zig @@ -118,11 +118,12 @@ pub fn build(b: *Build) !void { \\ ; - const use_healed = b.option(bool, "healed", "Run exercises from patches/healed") orelse false; + const healed = b.option(bool, "healed", "Run exercises from patches/healed") orelse false; + const override_healed_path = b.option([]const u8, "healed-path", "Override healed path"); const exno: ?usize = b.option(usize, "n", "Select exercise"); - const healed_path = "patches/healed"; - const work_path = if (use_healed) healed_path else "exercises"; + const healed_path = if (override_healed_path) |path| path else "patches/healed"; + const work_path = if (healed) healed_path else "exercises"; const header_step = PrintStep.create(b, logo); @@ -168,7 +169,7 @@ pub fn build(b: *Build) !void { start_step.dependOn(&prev_step.step); return; - } else if (use_healed and false) { + } else if (healed and false) { // Special case when healed by the eowyn script, where we can make the // code more efficient. // diff --git a/test/tests.zig b/test/tests.zig index 1fda49d..0fd3286 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -19,27 +19,28 @@ const Exercise = root.Exercise; pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { const step = b.step("test-cli", "Test the command line interface"); - // We should use a temporary path, but it will make the implementation of - // `build.zig` more complex. - const work_path = "patches/healed"; - - fs.cwd().makePath(work_path) catch |err| { - return fail(step, "unable to make '{s}': {s}\n", .{ work_path, @errorName(err) }); - }; - - const heal_step = HealStep.create(b, exercises, work_path); - { // Test that `zig build -Dhealed -Dn=n test` selects the nth exercise. const case_step = createCase(b, "case-1"); + const tmp_path = makeTempPath(b) catch |err| { + return fail(step, "unable to make tmp path: {s}\n", .{@errorName(err)}); + }; + + const heal_step = HealStep.create(b, exercises, tmp_path); + for (exercises[0 .. exercises.len - 1]) |ex| { const n = ex.number(); if (ex.skip) continue; - const cmd = b.addSystemCommand( - &.{ b.zig_exe, "build", "-Dhealed", b.fmt("-Dn={}", .{n}), "test" }, - ); + const cmd = b.addSystemCommand(&.{ + b.zig_exe, + "build", + "-Dhealed", + b.fmt("-Dhealed-path={s}", .{tmp_path}), + b.fmt("-Dn={}", .{n}), + "test", + }); cmd.setName(b.fmt("zig build -Dhealed -Dn={} test", .{n})); cmd.expectExitCode(0); @@ -56,20 +57,34 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { case_step.dependOn(&cmd.step); } - step.dependOn(case_step); + const cleanup = b.addRemoveDirTree(tmp_path); + cleanup.step.dependOn(case_step); + + step.dependOn(&cleanup.step); } { // Test that `zig build -Dhealed -Dn=n test` skips disabled esercises. const case_step = createCase(b, "case-2"); + const tmp_path = makeTempPath(b) catch |err| { + return fail(step, "unable to make tmp path: {s}\n", .{@errorName(err)}); + }; + + const heal_step = HealStep.create(b, exercises, tmp_path); + for (exercises[0 .. exercises.len - 1]) |ex| { const n = ex.number(); if (!ex.skip) continue; - const cmd = b.addSystemCommand( - &.{ b.zig_exe, "build", "-Dhealed", b.fmt("-Dn={}", .{n}), "test" }, - ); + const cmd = b.addSystemCommand(&.{ + b.zig_exe, + "build", + "-Dhealed", + b.fmt("-Dhealed-path={s}", .{tmp_path}), + b.fmt("-Dn={}", .{n}), + "test", + }); cmd.setName(b.fmt("zig build -Dhealed -Dn={} test", .{n})); cmd.expectExitCode(0); cmd.expectStdOutEqual(""); @@ -80,15 +95,30 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { case_step.dependOn(&cmd.step); } - step.dependOn(case_step); + const cleanup = b.addRemoveDirTree(tmp_path); + cleanup.step.dependOn(case_step); + + step.dependOn(&cleanup.step); } { // Test that `zig build -Dhealed` process all the exercises in order. const case_step = createCase(b, "case-3"); + const tmp_path = makeTempPath(b) catch |err| { + return fail(step, "unable to make tmp path: {s}\n", .{@errorName(err)}); + }; + + const heal_step = HealStep.create(b, exercises, tmp_path); + heal_step.step.dependOn(case_step); + // TODO: when an exercise is modified, the cache is not invalidated. - const cmd = b.addSystemCommand(&.{ b.zig_exe, "build", "-Dhealed" }); + const cmd = b.addSystemCommand(&.{ + b.zig_exe, + "build", + "-Dhealed", + b.fmt("-Dhealed-path={s}", .{tmp_path}), + }); cmd.setName("zig build -Dhealed"); cmd.expectExitCode(0); cmd.step.dependOn(&heal_step.step); @@ -97,9 +127,10 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { const verify = CheckStep.create(b, exercises, stderr, true); verify.step.dependOn(&cmd.step); - case_step.dependOn(&verify.step); + const cleanup = b.addRemoveDirTree(tmp_path); + cleanup.step.dependOn(&verify.step); - step.dependOn(case_step); + step.dependOn(&cleanup.step); } { @@ -107,10 +138,22 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { // in order. const case_step = createCase(b, "case-4"); + const tmp_path = makeTempPath(b) catch |err| { + return fail(step, "unable to make tmp path: {s}\n", .{@errorName(err)}); + }; + + const heal_step = HealStep.create(b, exercises, tmp_path); + heal_step.step.dependOn(case_step); + // TODO: when an exercise is modified, the cache is not invalidated. - const cmd = b.addSystemCommand( - &.{ b.zig_exe, "build", "-Dhealed", "-Dn=1", "start" }, - ); + const cmd = b.addSystemCommand(&.{ + b.zig_exe, + "build", + "-Dhealed", + b.fmt("-Dhealed-path={s}", .{tmp_path}), + "-Dn=1", + "start", + }); cmd.setName("zig build -Dhealed -Dn=1 start"); cmd.expectExitCode(0); cmd.step.dependOn(&heal_step.step); @@ -119,9 +162,10 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { const verify = CheckStep.create(b, exercises, stderr, false); verify.step.dependOn(&cmd.step); - case_step.dependOn(&verify.step); + const cleanup = b.addRemoveDirTree(tmp_path); + cleanup.step.dependOn(&verify.step); - step.dependOn(case_step); + step.dependOn(&cleanup.step); } { @@ -133,18 +177,11 @@ pub fn addCliTests(b: *std.Build, exercises: []const Exercise) *Step { cmd.expectExitCode(1); expectStdErrMatch(cmd, exercises[0].hint); - cmd.step.dependOn(&heal_step.step); + cmd.step.dependOn(case_step); - case_step.dependOn(&cmd.step); - - step.dependOn(case_step); + step.dependOn(&cmd.step); } - // Don't add the cleanup step, since it may delete work_path while a test - // case is running. - //const cleanup = b.addRemoveDirTree(work_path); - //step.dependOn(&cleanup.step); - return step; } @@ -378,6 +415,18 @@ fn heal(allocator: Allocator, exercises: []const Exercise, work_path: []const u8 } } +/// This function is the same as the one in std.Build.makeTempPath, with the +/// difference that returns an error when the temp path cannot be created. +pub fn makeTempPath(b: *Build) ![]const u8 { + const rand_int = std.crypto.random.int(u64); + const tmp_dir_sub_path = "tmp" ++ fs.path.sep_str ++ Build.hex64(rand_int); + const path = b.cache_root.join(b.allocator, &.{tmp_dir_sub_path}) catch + @panic("OOM"); + try b.cache_root.handle.makePath(tmp_dir_sub_path); + + return path; +} + // // Missing functions from std.Build.RunStep //