Skip to content

reexec: add reexectest package - #211

Open
thaJeztah wants to merge 2 commits into
moby:mainfrom
thaJeztah:reexectest
Open

thaJeztah wants to merge 2 commits into
moby:mainfrom
thaJeztah:reexectest

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

This package allows using the reexec functionality to execute child processes as part of a test.

@thaJeztah
thaJeztah force-pushed the reexectest branch 4 times, most recently from 1b6866e to fe0a892 Compare April 17, 2026 14:32
@thaJeztah
thaJeztah marked this pull request as ready for review April 17, 2026 15:51
@thaJeztah
thaJeztah force-pushed the reexectest branch 3 times, most recently from 5e3a915 to 9046543 Compare June 5, 2026 08:16
@thaJeztah
thaJeztah requested review from kolyshkin and vvoland June 8, 2026 13:28
@vvoland

vvoland commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Hmm does it only work against test binaries created by go test -c, or does it also work with direct go test?

@thaJeztah

Copy link
Copy Markdown
Member Author

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

This comment was marked as outdated.

@vvoland vvoland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thaJeztah
thaJeztah force-pushed the reexectest branch 4 times, most recently from 6483052 to cfded60 Compare September 8, 2026 21:03
@thaJeztah
thaJeztah requested a balanced review from Copilot September 8, 2026 21:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 in commandContext does not hide this exported wrapper, so fatal errors are attributed to package internals instead of the caller. Mark this wrapper with t.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

Comment thread reexec/reexectest/reexectest_test.go
Comment thread reexec/reexectest/reexectest_test.go Outdated
Comment thread reexec/reexectest/reexectest.go
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This comment was marked as outdated.

This comment was marked as outdated.

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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Only minor cleanup remains for unused platform shims.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@thaJeztah

Copy link
Copy Markdown
Member Author

Yay! Looks like I pleased CoPilot now.

@vvoland PTAL 😅

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.

3 participants