From 9e6bf67d7dc7a29fc609ec819fae42130a8d7294 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 5 Nov 2026 07:06:33 +0100 Subject: [PATCH] test: avoid redundant fixture work Reuse immutable fixtures, combine Git queries and avoid large data copies to reduce test overhead while retaining behavior checks. Assisted-by: Codex:GPT-6 --- act/artifactcache/handler_test.go | 17 ++--- act/common/git/fetch_action_test.go | 17 ++++- act/common/git/git_test.go | 56 ++++++++--------- act/container/docker_build_test.go | 2 +- act/container/docker_run_test.go | 12 ++-- act/container/host_environment_test.go | 10 +-- act/container/parse_env_file_test.go | 32 +++++----- act/filecollector/file_collector_test.go | 16 ++--- act/runner/action_composite_test.go | 2 +- act/runner/action_test.go | 30 +++++---- act/runner/expression_test.go | 5 +- act/runner/job_executor_test.go | 7 ++- act/runner/patch_actions_e2e_test.go | 13 ++-- act/runner/patch_actions_test.go | 26 ++++---- act/runner/reusable_workflow_test.go | 24 +++---- act/runner/run_context_test.go | 12 ++-- act/runner/step_action_remote_test.go | 12 +--- internal/app/cmd/daemon_test.go | 9 +-- internal/app/cmd/exec_test.go | 62 ++++++++----------- internal/app/run/post_task_script_test.go | 17 +---- internal/app/run/runner_idle_cleanup_test.go | 9 +-- internal/app/run/setup_test.go | 3 +- internal/pkg/action/checkout/checkout_test.go | 47 ++++++++++---- internal/pkg/process/group_windows_test.go | 15 ++--- internal/pkg/process/killer_unix_test.go | 38 +----------- internal/pkg/process/killer_windows_test.go | 24 +++++-- internal/pkg/report/reporter_test.go | 32 +++------- internal/pkg/telemetry/telemetry_test.go | 3 +- 28 files changed, 261 insertions(+), 291 deletions(-) diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index 4a560eaa..dcf93a82 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -851,6 +851,9 @@ func TestHandler_touchCacheSkipsFreshCompletedEntries(t *testing.T) { func newTestHandler(t *testing.T, policy Policy, entries ...*Cache) *Handler { t.Helper() handler := startTestHandler(t, Options{Policy: policy}) + if len(entries) == 0 { + return handler + } db, err := handler.openDB() require.NoError(t, err) @@ -940,6 +943,10 @@ func TestHandler_RejectsBearer(t *testing.T) { func TestHandler_UnregisterRevokes(t *testing.T) { t.Parallel() + handler, err := StartHandler(Options{Dir: filepath.Join(t.TempDir(), "artifactcache")}) + require.NoError(t, err) + defer handler.Close() + for _, tc := range []struct { name string token string @@ -950,9 +957,6 @@ func TestHandler_UnregisterRevokes(t *testing.T) { {"reference counted", "shared", "v", 2}, } { t.Run(tc.name, func(t *testing.T) { - handler, err := StartHandler(Options{Dir: filepath.Join(t.TempDir(), "artifactcache")}) - require.NoError(t, err) - defer handler.Close() revokers := make([]func(), tc.registrations) for i := range revokers { revokers[i] = handler.RegisterJob(tc.token, JobCredential{Repo: testRepo}) @@ -1049,9 +1053,8 @@ func TestHandler_ArtifactSignature(t *testing.T) { base := handler.ExternalURL() + apiPath good := signArtifactURL(handler, 1) - other, err := StartHandler(Options{Dir: filepath.Join(t.TempDir(), "artifactcache2")}) - require.NoError(t, err) - defer other.Close() + secret := bytes.Clone(handler.secret) + secret[0] ^= 1 for _, tc := range []struct { name string @@ -1060,7 +1063,7 @@ func TestHandler_ArtifactSignature(t *testing.T) { {"missing signature", fmt.Sprintf("%s/artifacts/%d", base, 1)}, {"tampered signature", good[:len(good)-4] + "dead"}, {"expired signature", handler.signedArtifactURL(JobCredential{}, 1, time.Now().Add(-time.Second))}, - {"signature from a different server", base + strings.SplitN(signArtifactURL(other, 1), apiPath, 2)[1]}, + {"signature from a different server", base + strings.SplitN(signArtifactURL(&Handler{secret: secret}, 1), apiPath, 2)[1]}, } { t.Run(tc.name, func(t *testing.T) { resp, err := testClient.Get(tc.url) diff --git a/act/common/git/fetch_action_test.go b/act/common/git/fetch_action_test.go index 913d22fc..df4292b9 100644 --- a/act/common/git/fetch_action_test.go +++ b/act/common/git/fetch_action_test.go @@ -36,7 +36,7 @@ func TestFetchAction(t *testing.T) { root := t.TempDir() remote := filepath.Join(root, "org", "action") - require.NoError(t, gitCmd("init", "--initial-branch=main", remote)) + require.NoError(t, os.CopyFS(remote, os.DirFS(filepath.Join(gitFixtureDir, "work")))) require.NoError(t, os.WriteFile(filepath.Join(remote, "action.yml"), []byte("name: action\n"), 0o644)) require.NoError(t, os.WriteFile(filepath.Join(remote, "run.sh"), []byte("#!/bin/sh\n"), 0o755)) require.NoError(t, os.Symlink("action.yml", filepath.Join(remote, "link"))) @@ -53,6 +53,7 @@ func TestFetchAction(t *testing.T) { var archiveRequests []http.Header var mirror *httptest.Server archiveCache := map[string][]byte{} + revisions := map[string]string{} requests, archives, failures := 0, "", 0 handler := http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { mu.Lock() @@ -79,7 +80,14 @@ func TestFetchAction(t *testing.T) { http.Redirect(writer, request, mirror.URL+request.URL.Path, http.StatusFound) return } - sha, _ := gitOutput(request.Context(), remote, "rev-parse", strings.TrimSuffix(ref, ".tar.gz")) + sha, ok := revisions[ref] + if !ok { + var err error + sha, err = gitOutput(request.Context(), remote, "rev-parse", strings.TrimSuffix(ref, ".tar.gz")) + if err == nil { + revisions[ref] = sha + } + } etag := fmt.Sprintf(`W/"%s-tar.gz-%t"`, sha, archives != "unprefixed") if request.Header.Get("If-None-Match") == etag { writer.WriteHeader(http.StatusNotModified) @@ -208,6 +216,9 @@ func TestFetchAction(t *testing.T) { require.NoError(t, gitCmd("-C", remote, "commit", "--allow-empty", "-m", "moved")) require.NoError(t, gitCmd("-C", remote, "tag", "--force", "v1")) expectedSha = gitRevParse(t, remote, "v1") + mu.Lock() + clear(revisions) + mu.Unlock() moved, movedSha, err := FetchAction(t.Context(), input) require.NoError(t, err) assert.Equal(t, expectedSha, movedSha) @@ -253,7 +264,7 @@ func TestFetchAction(t *testing.T) { }) t.Run("downloads again over a tree whose files changed size or that has no record, not under a job copying it", func(t *testing.T) { - sha := gitRevParse(t, remote, "main") + sha := expectedSha for _, mode := range []string{"prefixed", "none"} { serve(mode, 0) input := newInput(sha) diff --git a/act/common/git/git_test.go b/act/common/git/git_test.go index eadeaaf6..825ec148 100644 --- a/act/common/git/git_test.go +++ b/act/common/git/git_test.go @@ -112,8 +112,7 @@ func TestFindGitMetadataOfSHA256RepositoryUsesOrigin(t *testing.T) { t.Parallel() basedir := t.TempDir() const remoteURL = "https://github.com/owner/repo.git" - require.NoError(t, gitCmd("init", "--object-format=sha256", "--initial-branch=main", basedir)) - require.NoError(t, os.RemoveAll(filepath.Join(basedir, ".git", "hooks"))) + require.NoError(t, gitCmd("init", "--template=", "--object-format=sha256", "--initial-branch=main", basedir)) require.NoError(t, gitCmd("-C", basedir, "commit", "--allow-empty", "-m", "init")) require.NoError(t, gitCmd("-C", basedir, "remote", "add", "origin", remoteURL)) require.NoError(t, gitCmd("-C", basedir, "config", "--add", "remote.origin.url", "https://github.com/other/repo.git")) @@ -205,16 +204,13 @@ func TestGitCloneExecutor(t *testing.T) { require.NoError(t, gitCmd("-C", workDir, "add", "action.sh", ".gitattributes")) require.NoError(t, gitCmd("-C", workDir, "commit", "-m", "initial")) require.NoError(t, gitCmd("-C", workDir, "tag", "v2")) - require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main")) - require.NoError(t, gitCmd("-C", workDir, "push", "origin", "v2")) require.NoError(t, gitCmd("-C", workDir, "checkout", "-b", "act-fails")) require.NoError(t, gitCmd("-C", workDir, "commit", "--allow-empty", "-m", "branch-commit")) - require.NoError(t, gitCmd("-C", workDir, "push", "origin", "act-fails")) fullSha := gitRevParse(t, workDir, "main") - require.NoError(t, gitCmd("-C", workDir, "push", "origin", "main:refs/heads/"+fullSha[:4])) + require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main", "v2", "act-fails", "main:refs/heads/"+fullSha[:4])) hostConfig := filepath.Join(t.TempDir(), "gitconfig") require.NoError(t, os.WriteFile(hostConfig, []byte("[core]\n\tautocrlf = true\n\teol = crlf\n[url "+strconv.Quote(remoteDir)+"]\n\tinsteadOf = https://example.invalid/action\n"), 0o644)) @@ -282,10 +278,10 @@ func TestGitCloneExecutor(t *testing.T) { func TestGitCloneExecutorReclonesWhenOriginURLChanges(t *testing.T) { t.Parallel() createRemote := func(message string) string { - remoteDir, workDir := newGitRemote(t, message) - require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main")) - - return remoteDir + dir := t.TempDir() + require.NoError(t, os.CopyFS(dir, os.DirFS(filepath.Join(gitFixtureDir, "work")))) + require.NoError(t, gitCmd("-C", dir, "commit", "--allow-empty", "-m", message)) + return dir } oldRemoteDir := createRemote("old-action") @@ -405,31 +401,30 @@ func TestGitCloneExecutorShallow(t *testing.T) { remoteDir, workDir := newGitRemote(t, "c1", "c2", "c3") require.NoError(t, gitCmd("-C", workDir, "tag", "v1")) unadvertisedSHA := gitRevParse(t, workDir, "HEAD~1") - require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main")) - require.NoError(t, gitCmd("-C", workDir, "push", "origin", "v1")) + require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main", "v1")) shallowMarker := func(dir string) string { return filepath.Join(dir, ".git", "shallow") } for name, ref := range map[string]string{"branch is cloned shallowly": "main", "tag is cloned shallowly": "v1"} { t.Run(name, func(t *testing.T) { + t.Parallel() dir := t.TempDir() require.NoError(t, NewGitCloneExecutor(NewGitCloneExecutorInput{ URL: remoteDir, Ref: ref, Dir: dir, Depth: 1, })(t.Context())) assert.FileExists(t, shallowMarker(dir), "clone should be shallow") - assert.Equal(t, 1, gitRevCount(t, dir), "only the tip commit should be present") - assert.Equal(t, "c3", gitHeadSubject(t, dir)) + assert.Equal(t, []string{"c3"}, gitSubjects(t, dir), "only the tip commit should be present") }) } unadvertisedRemote := func(t *testing.T, allowUnadvertised bool) string { remote := filepath.Join(t.TempDir(), "remote.git") - require.NoError(t, gitCmd("clone", "--bare", remoteDir, remote)) - require.NoError(t, gitCmd("-C", remote, "config", "uploadpack.allowAnySHA1InWant", strconv.FormatBool(allowUnadvertised))) + require.NoError(t, gitCmd("clone", "--bare", "--template=", "--config", "uploadpack.allowAnySHA1InWant="+strconv.FormatBool(allowUnadvertised), remoteDir, remote)) return remote } t.Run("commit hash resolves when the remote accepts a direct shallow fetch", func(t *testing.T) { + t.Parallel() dir := t.TempDir() require.NoError(t, NewGitCloneExecutor(NewGitCloneExecutorInput{ URL: unadvertisedRemote(t, false), Ref: unadvertisedSHA, Dir: dir, Depth: 1, @@ -438,6 +433,7 @@ func TestGitCloneExecutorShallow(t *testing.T) { }) t.Run("commit hash is fetched shallowly, moved to another hash, then reused without reaching the remote", func(t *testing.T) { + t.Parallel() remote := unadvertisedRemote(t, true) dir := t.TempDir() cloneAt := func(ref string) error { @@ -448,7 +444,7 @@ func TestGitCloneExecutorShallow(t *testing.T) { require.NoError(t, cloneAt(unadvertisedSHA)) assert.FileExists(t, shallowMarker(dir)) - assert.Equal(t, 1, gitRevCount(t, dir)) + assert.Len(t, gitSubjects(t, dir), 1) assert.Equal(t, unadvertisedSHA, gitRevParse(t, dir, "HEAD")) olderSHA := gitRevParse(t, workDir, "HEAD~2") @@ -461,6 +457,8 @@ func TestGitCloneExecutorShallow(t *testing.T) { }) t.Run("moving hexadecimal-named branch updates while staying shallow", func(t *testing.T) { + t.Parallel() + remoteDir, workDir := newGitRemote(t, "c2", "c3") require.NoError(t, gitCmd("-C", workDir, "push", "origin", "main:deadbeef")) dir := t.TempDir() clone := NewGitCloneExecutor(NewGitCloneExecutorInput{ @@ -473,19 +471,17 @@ func TestGitCloneExecutorShallow(t *testing.T) { require.NoError(t, gitCmd("-C", workDir, "push", "origin", "main:deadbeef")) require.NoError(t, clone(t.Context())) - assert.Equal(t, "c4", gitHeadSubject(t, dir), "reused shallow clone should update to the new tip") + assert.Equal(t, []string{"c4"}, gitSubjects(t, dir), "reused shallow clone should update to the new tip") assert.FileExists(t, shallowMarker(dir), "repo should remain shallow after update") - assert.Equal(t, 1, gitRevCount(t, dir)) }) t.Run("sha256 full hash is cloned shallowly, reused without the remote, and its 40-character prefix is short", func(t *testing.T) { + t.Parallel() remote := filepath.Join(t.TempDir(), "remote.git") - require.NoError(t, gitCmd("init", "--bare", "--object-format=sha256", "--initial-branch=main", remote)) work := t.TempDir() - require.NoError(t, gitCmd("init", "--object-format=sha256", "--initial-branch=main", work)) - require.NoError(t, gitCmd("-C", work, "remote", "add", "origin", remote)) + require.NoError(t, gitCmd("init", "--template=", "--object-format=sha256", "--initial-branch=main", work)) require.NoError(t, gitCmd("-C", work, "commit", "--allow-empty", "-m", "pinned")) - require.NoError(t, gitCmd("-C", work, "push", "origin", "main")) + require.NoError(t, gitCmd("clone", "--bare", "--template=", work, remote)) sha := gitRevParse(t, work, "HEAD") dir := t.TempDir() clone := NewGitCloneExecutor(NewGitCloneExecutorInput{URL: remote, Ref: sha, Dir: dir, Depth: 1}) @@ -504,11 +500,11 @@ func TestGitCloneExecutorPinnedHashIgnoresShadowingRef(t *testing.T) { pinned := gitRevParse(t, workDir, "HEAD") require.NoError(t, gitCmd("-C", workDir, "commit", "--allow-empty", "-m", "decoy")) require.NoError(t, gitCmd("-C", workDir, "tag", pinned)) - require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main")) - require.NoError(t, gitCmd("-C", workDir, "push", "origin", pinned)) + require.NoError(t, gitCmd("-C", workDir, "push", "-u", "origin", "main", pinned)) for name, depth := range map[string]int{"shallow": 1, "full clone": 0} { t.Run(name, func(t *testing.T) { + t.Parallel() dir := t.TempDir() require.NoError(t, NewGitCloneExecutor(NewGitCloneExecutorInput{ URL: remoteDir, Ref: pinned, Dir: dir, Depth: depth, @@ -540,13 +536,11 @@ func gitRevParse(t *testing.T, dir, rev string) string { return strings.TrimSpace(string(out)) } -func gitRevCount(t *testing.T, dir string) int { +func gitSubjects(t *testing.T, dir string) []string { t.Helper() - out, err := exec.Command("git", "-C", dir, "rev-list", "--count", "HEAD").Output() + out, err := exec.Command("git", "-C", dir, "log", "--format=%s").Output() require.NoError(t, err) - n, err := strconv.Atoi(strings.TrimSpace(string(out))) - require.NoError(t, err) - return n + return strings.Split(strings.TrimSpace(string(out)), "\n") } func gitHeadSubject(t *testing.T, dir string) string { @@ -680,7 +674,7 @@ func TestNewGitCloneExecutorFetchSendsTokenAndHonoursContext(t *testing.T) { }) dir := filepath.Join(t.TempDir(), "cached-action") - require.NoError(t, gitCmd("init", dir)) + require.NoError(t, os.CopyFS(dir, os.DirFS(filepath.Join(gitFixtureDir, "work")))) require.NoError(t, gitCmd("-C", dir, "remote", "add", "origin", server.URL)) ctx, cancel := context.WithCancel(t.Context()) diff --git a/act/container/docker_build_test.go b/act/container/docker_build_test.go index 2b71efe2..f7762d8e 100644 --- a/act/container/docker_build_test.go +++ b/act/container/docker_build_test.go @@ -42,11 +42,11 @@ func TestDockerBuildLabelsImageWithItsTagAndReportsDaemonError(t *testing.T) { func TestCreateBuildContextFollowsDockerignoreAndSymlinkedRootButKeepsDockerfile(t *testing.T) { dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, "build"), 0o755)) for name, content := range map[string]string{ ".dockerignore": "*.log\nbuild\n!build/keep.txt\nDockerfile", "Dockerfile": "FROM scratch", "a.txt": "a", "a.log": "log", "build/drop.txt": "drop", "build/keep.txt": "keep", } { - require.NoError(t, os.MkdirAll(filepath.Join(dir, filepath.Dir(name)), 0o755)) require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644)) } require.NoError(t, os.Mkdir(filepath.Join(dir, "empty"), 0o755)) diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 635b1e34..1f5ee085 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -540,6 +540,9 @@ func TestRejectsMissingContainer(t *testing.T) { } func TestPublicCopyPipelineHandlesStaleID(t *testing.T) { + source := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(source, "x"), nil, 0o644)) + for _, testcase := range []struct { name, destination, extractionRoot, entry string directory bool @@ -567,8 +570,6 @@ func TestPublicCopyPipelineHandlesStaleID(t *testing.T) { cr := &containerReference{id: "stale", cli: client, input: &NewContainerInput{Name: "job-1"}} if testcase.directory { - source := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(source, "x"), nil, 0o644)) require.NoError(t, cr.CopyDir(testcase.destination, source+string(filepath.Separator), false, true)(ctx)) } else { require.NoError(t, cr.Copy(testcase.destination, &FileEntry{Name: "x", Mode: 0o644})(ctx)) @@ -1108,6 +1109,10 @@ func TestConnectTakesDaemonOSFromPing(t *testing.T) { } func TestContainerCopyModes(t *testing.T) { + source := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(source, "run.sh"), []byte("#!/bin/sh\n"), 0o666)) + require.NoError(t, os.Chmod(filepath.Join(source, "run.sh"), 0o666)) + for _, testcase := range []struct { hostOS, serverOS string expected int64 @@ -1117,9 +1122,6 @@ func TestContainerCopyModes(t *testing.T) { {"linux", "linux", 0o666}, } { t.Run(testcase.hostOS+"-"+testcase.serverOS, func(t *testing.T) { - source := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(source, "run.sh"), []byte("#!/bin/sh\n"), 0o666)) - require.NoError(t, os.Chmod(filepath.Join(source, "run.sh"), 0o666)) file := &FileEntry{Name: "run.sh", Mode: 0o600, Body: "#!/bin/sh\n"} original := hostOS hostOS = testcase.hostOS diff --git a/act/container/host_environment_test.go b/act/container/host_environment_test.go index 4cfe3009..cf3a8abe 100644 --- a/act/container/host_environment_test.go +++ b/act/container/host_environment_test.go @@ -32,6 +32,11 @@ func TestActionPlatformNames(t *testing.T) { } func TestCopyDir(t *testing.T) { + src := filepath.Join(t.TempDir(), "action") + require.NoError(t, os.MkdirAll(filepath.Join(src, ".git", "objects"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(src, ".git", "objects", "pack"), []byte("x"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(src, "action.yml"), []byte("name: x\n"), 0o600)) + for _, test := range []struct { name string useGitIgnore, skipGitDir bool @@ -41,11 +46,6 @@ func TestCopyDir(t *testing.T) { {name: "unfiltered outside a git repository", useGitIgnore: true}, } { t.Run(test.name, func(t *testing.T) { - src := filepath.Join(t.TempDir(), "action") - require.NoError(t, os.MkdirAll(filepath.Join(src, ".git", "objects"), 0o700)) - require.NoError(t, os.WriteFile(filepath.Join(src, ".git", "objects", "pack"), []byte("x"), 0o600)) - require.NoError(t, os.WriteFile(filepath.Join(src, "action.yml"), []byte("name: x\n"), 0o600)) - dest := filepath.Join(t.TempDir(), "dest") require.NoError(t, (&HostEnvironment{}).CopyDir(dest, src+string(filepath.Separator), test.useGitIgnore, test.skipGitDir)(t.Context())) diff --git a/act/container/parse_env_file_test.go b/act/container/parse_env_file_test.go index e011bdb1..d1e9e66e 100644 --- a/act/container/parse_env_file_test.go +++ b/act/container/parse_env_file_test.go @@ -5,6 +5,7 @@ package container import ( "bufio" + "bytes" "os" "path/filepath" "strings" @@ -15,14 +16,9 @@ import ( "golang.org/x/text/encoding/unicode" ) -func newTestHostEnv(t *testing.T) (*HostEnvironment, string) { - t.Helper() - e := &HostEnvironment{Path: t.TempDir()} - return e, filepath.Join(e.Path, "envfile") -} - func TestParseEnvFile(t *testing.T) { t.Parallel() + hostEnv := &HostEnvironment{Path: t.TempDir()} largeValue := strings.Repeat("x", 2*1024*1024) encodeUTF16 := func(endianness unicode.Endianness) string { content, err := unicode.UTF16(endianness, unicode.UseBOM).NewEncoder().Bytes([]byte("FOO=bar\r\nMULTI< %q; sleep 600`, pidFile) - cmd := exec.Command("/bin/sh", "-c", script) - // Launch as its own process-group leader, exactly like a real process does - // (see SysProcAttr), so the killer's PGID == the process PID. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd, ready := newTestProcess(t, "parent", false) require.NoError(t, cmd.Start()) t.Cleanup(func() { _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) @@ -73,25 +56,10 @@ func TestKillerKillsTree(t *testing.T) { require.NoError(t, err) defer killer.Close() - // Wait for the backgrounded child PID to be reported. - var childPID int - require.Eventually(t, func() bool { - b, e := os.ReadFile(pidFile) - if e != nil { - return false - } - s := strings.TrimSpace(string(b)) - if s == "" { - return false - } - childPID, _ = strconv.Atoi(s) - return childPID > 0 && processAlive(childPID) - }, 20*time.Second, 100*time.Millisecond, "child process should start") + childPID := ready() + require.True(t, processAlive(childPID), "child process should start") - // Killing the group must terminate both the parent and the backgrounded child. require.NoError(t, killer.Kill()) - // Reap the parent so it does not linger as a zombie (which would still report - // as alive); SIGKILL makes Wait return promptly. _ = cmd.Wait() require.Eventually(t, func() bool { diff --git a/internal/pkg/process/killer_windows_test.go b/internal/pkg/process/killer_windows_test.go index 0aed87c6..6978faea 100644 --- a/internal/pkg/process/killer_windows_test.go +++ b/internal/pkg/process/killer_windows_test.go @@ -5,7 +5,6 @@ package process import ( "testing" - "time" "github.com/stretchr/testify/require" "golang.org/x/sys/windows" @@ -25,6 +24,23 @@ func processAlive(pid int) bool { return code == stillActive } +func newProcessExitWaiter(t *testing.T, pids ...int) func() { + t.Helper() + handles := make([]windows.Handle, 0, len(pids)) + for _, pid := range pids { + handle, err := windows.OpenProcess(windows.SYNCHRONIZE, false, uint32(pid)) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, windows.CloseHandle(handle)) }) + handles = append(handles, handle) + } + return func() { + t.Helper() + result, err := windows.WaitForMultipleObjects(handles, true, 20000) + require.NoError(t, err) + require.EqualValues(t, windows.WAIT_OBJECT_0, result, "processes should exit: %v", pids) + } +} + func TestKillerKillsTree(t *testing.T) { cmd, ready := newTestProcess(t, "parent", false) require.NoError(t, cmd.Start()) @@ -39,9 +55,7 @@ func TestKillerKillsTree(t *testing.T) { childPID := ready() require.True(t, processAlive(childPID), "child process should start") + waitForExit := newProcessExitWaiter(t, cmd.Process.Pid, childPID) require.NoError(t, killer.Kill()) - - require.Eventually(t, func() bool { - return !processAlive(cmd.Process.Pid) && !processAlive(childPID) - }, 20*time.Second, 200*time.Millisecond, "parent and child should both be terminated") + waitForExit() } diff --git a/internal/pkg/report/reporter_test.go b/internal/pkg/report/reporter_test.go index 64c1e9d0..f1afd227 100644 --- a/internal/pkg/report/reporter_test.go +++ b/internal/pkg/report/reporter_test.go @@ -173,11 +173,9 @@ func TestReporter_RunDaemonClose_Race(t *testing.T) { ) ctx, cancel := context.WithCancel(t.Context()) - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) cfg, _ := config.LoadDefault("") reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{ - Context: taskCtx, + Context: &structpb.Struct{}, }, cfg) reporter.ResetSteps(newTestSteps(1)) @@ -474,10 +472,8 @@ func TestReporter_ServerCancelStillFlushesFinal(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) defer cancel() - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) cfg, _ := config.LoadDefault("") - reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: taskCtx}, cfg) + reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: &structpb.Struct{}}, cfg) reporter.ResetSteps(newTestSteps(1)) reporter.stateMu.Lock() @@ -527,10 +523,8 @@ func TestReporter_CloseReportsCancelledOnCanceledCtx(t *testing.T) { ) ctx, cancel := context.WithCancel(t.Context()) - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) cfg, _ := config.LoadDefault("") - reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: taskCtx}, cfg) + reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: &structpb.Struct{}}, cfg) reporter.ResetSteps(newTestSteps(1)) cancel() @@ -671,19 +665,15 @@ func TestReporter_SetOutputs(t *testing.T) { require.True(t, ok) assert.Equal(t, "caf\xff", got.value) - maxValue := strings.Repeat("v", maxOutputValueLen) - r.SetOutputs(map[string]string{"atlimit": maxValue}) + r.SetOutputs(map[string]string{"atlimit": longValue[:maxOutputValueLen]}) got, ok = r.outputs["atlimit"] require.True(t, ok) assert.Len(t, got.value, maxOutputValueLen) } func TestReporter_DebugSettingsAreNotMasked(t *testing.T) { - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) - reporter := NewReporter(t.Context(), nil, nil, &runnerv1.Task{ - Context: taskCtx, + Context: &structpb.Struct{}, Secrets: map[string]string{"ACTIONS_STEP_DEBUG": "true", "ACTIONS_RUNNER_DEBUG": "true", "TOKEN": "s3cr3t"}, }, &config.Config{}) defer deregisterGlobalMasks(reporter) @@ -861,10 +851,8 @@ func TestReporter_ServerFailureResultIsReportedAsFailure(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) defer cancel() - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) cfg, _ := config.LoadDefault("") - reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: taskCtx}, cfg) + reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: &structpb.Struct{}}, cfg) close(reporter.daemon) require.NoError(t, reporter.ReportState(false)) @@ -1031,9 +1019,7 @@ func newTestReporter(t *testing.T, client *mocks.Client, cfg *config.Config) *Re t.Helper() ctx, cancel := context.WithCancel(t.Context()) t.Cleanup(cancel) - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) - return NewReporter(ctx, cancel, client, &runnerv1.Task{Context: taskCtx}, cfg) + return NewReporter(ctx, cancel, client, &runnerv1.Task{Context: &structpb.Struct{}}, cfg) } func newTestSteps(n int) []*model.Step { @@ -1051,11 +1037,9 @@ func TestReporter_ReportedSteps(t *testing.T) { return &runnerv1.UpdateLogResponse{AckIndex: req.Index + int64(len(req.Rows))}, nil }) client.On("UpdateTask", mock.Anything, mock.Anything).Maybe().Return(&runnerv1.UpdateTaskResponse{}, nil) - taskCtx, err := structpb.NewStruct(map[string]any{}) - require.NoError(t, err) cfg, _ := config.LoadDefault("") ctx, cancel := context.WithCancel(t.Context()) - reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: taskCtx, Secrets: map[string]string{"TOKEN": "s3cr3t"}}, cfg) + reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: &structpb.Struct{}, Secrets: map[string]string{"TOKEN": "s3cr3t"}}, cfg) reporter.RunDaemon() reporter.Logf("runner version") reporter.ResetSteps(steps) diff --git a/internal/pkg/telemetry/telemetry_test.go b/internal/pkg/telemetry/telemetry_test.go index 97ba635d..5145225f 100644 --- a/internal/pkg/telemetry/telemetry_test.go +++ b/internal/pkg/telemetry/telemetry_test.go @@ -117,7 +117,8 @@ func TestExportRetriesAndMatchesOTLP(t *testing.T) { certificate := server.TLS.Certificates[0] key, err := x509.MarshalPKCS8PrivateKey(certificate.PrivateKey) require.NoError(t, err) - certificatePath, keyPath := filepath.Join(t.TempDir(), "cert.pem"), filepath.Join(t.TempDir(), "key.pem") + dir := t.TempDir() + certificatePath, keyPath := filepath.Join(dir, "cert.pem"), filepath.Join(dir, "key.pem") require.NoError(t, os.WriteFile(certificatePath, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate.Certificate[0]}), 0o600)) require.NoError(t, os.WriteFile(keyPath, pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: key}), 0o600)) setEnv(t, map[string]string{