Conversation
1b6866e to
fe0a892
Compare
5e3a915 to
9046543
Compare
|
Hmm does it only work against test binaries created by |
|
Yup, should still work; adding a quick patch to show the output; diff --git a/reexec/reexectest/reexectest_test.go b/reexec/reexectest/reexectest_test.go
index c8dd5cd..1b57bf3 100644
--- a/reexec/reexectest/reexectest_test.go
+++ b/reexec/reexectest/reexectest_test.go
@@ -33,6 +33,8 @@ func TestRun(t *testing.T) {
}
if got := strings.TrimSpace(strings.TrimSuffix(string(out), "PASS\n")); got != expected {
t.Errorf("env-and-output output: got %q, want %q", got, expected)
+ } else {
+ t.Logf("env-and-output output: got %q", out)
}
})Then run; go test -v -run TestRun
=== RUN TestRun
=== RUN TestRun/env-and-output
reexectest_test.go:37: env-and-output output: got "child-env-and-output-ok\nPASS\n"
=== RUN TestRun/exit-code
=== RUN TestRun/args-passthrough
=== RUN TestRun/context
--- PASS: TestRun (0.05s)
--- PASS: TestRun/env-and-output (0.02s)
--- PASS: TestRun/exit-code (0.01s)
--- PASS: TestRun/args-passthrough (0.01s)
--- PASS: TestRun/context (0.01s)
=== RUN TestRunNonSubtest
--- PASS: TestRunNonSubtest (0.01s)
PASS
ok github.com/moby/sys/reexec/reexectest 0.401s |
9046543 to
845a186
Compare
vvoland
left a comment
There was a problem hiding this comment.
AI review:
[High] Anchor every -test.run path component
reexec/reexectest/reexectest.go:103
Go’s test runner splits -test.run expressions at unbracketed /. For a subtest named TestTarget/child, the generated expression:
^TestTarget/child$
is treated as separate ^TestTarget and child$ expressions. The first also matches TestTargetExtra, so TestTargetExtra/child runs in the re-exec child. Its Run call rejects the
child because argv[0] differs, after which its parent branch can execute and start further subprocesses.
I reproduced this deterministically: targeting TestTarget/child produced output from both the intended test and TestTargetExtra/child.
Construct an exact expression for each slash-delimited test-name component rather than anchoring the complete name only.
[High] Preserve the module’s Go 1.20 compatibility
reexec/reexectest/reexectest.go:73
testing.T.Context was added in Go 1.24, while reexec/go.mod declares Go 1.20. Consequently, consumers cannot compile the new package with Go 1.20–1.23.
A Go 1.20 container reported:
reexectest/reexectest.go:73:29: t.Context undefined
The green CI matrix does not catch this because its Go 1.18 job skips the reexec module and oldstable is already new enough. This is also raised in an existing review thread.
[High] Separate test-runner flags from child arguments
reexec/reexectest/reexectest.go:107
The test binary parses child arguments beginning with - before the selected test runs. For example:
reexectest.Command(t, "worker", "-user-flag")exits with status 2:
flag provided but not defined: -user-flag
Recognized -test.* arguments can instead alter the child test runner, including overriding test selection. Add a -- terminator before user arguments and have Run remove both the
injected test flag and separator. This was noted in the existing Copilot review summary.
[Medium] Do not corrupt the saved os.Args
reexec/reexectest/reexectest.go:59
origArgs := os.Args preserves only the slice header. The subsequent append reuses the same backing array because the result is shorter, overwriting the arguments that origArgs is meant
to restore. With child arguments, deferred cleanup restores a list with the test flag removed and the final argument duplicated.
Allocate a separate scrubbed slice before assigning it to os.Args. This is also covered by an existing review thread.
6483052 to
cfded60
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The run-pattern coverage is ineffective, and the new package’s tests are excluded from repository CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
reexec/reexectest/reexectest.go:100
- As with
Command, the helper marker incommandContextdoes not hide this exported wrapper, so fatal errors are attributed to package internals instead of the caller. Mark this wrapper witht.Helper()before delegating.
func CommandContext(t *testing.T, ctx context.Context, name string, args ...string) *exec.Cmd {
return commandContext(t, ctx, name, args...)
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
cfded60 to
b5efaab
Compare
fa97fc3 to
eb5d868
Compare
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
eb5d868 to
63eee86
Compare
63eee86 to
5634dac
Compare
5634dac to
5b36db5
Compare
This package allows using the reexec functionality to execute child processes as part of a test. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
5b36db5 to
6ff8880
Compare
|
Yay! Looks like I pleased CoPilot now. @vvoland PTAL 😅 |
This package allows using the reexec functionality to execute child processes as part of a test.