From da55ad89933115b95c98d023503f2010dbc33846 Mon Sep 17 00:00:00 2001 From: Majed Alfaraj Date: Fri, 12 Jun 2026 16:47:16 -0700 Subject: [PATCH 1/3] test(magician): add interceptingRunner to reroute sandbox executions The `interceptingRunner` wraps around `exec.Runner` and reroutes commands (such as git, make) to fake scripts inside the sandbox, if they exist. This eliminates the need to modify environment variables and allows for safe parallel testing. --- .ci/magician/cmd/sandbox_test.go | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/.ci/magician/cmd/sandbox_test.go b/.ci/magician/cmd/sandbox_test.go index 444f100bef98..c4ff8ff2ad58 100644 --- a/.ci/magician/cmd/sandbox_test.go +++ b/.ci/magician/cmd/sandbox_test.go @@ -16,6 +16,9 @@ package cmd import ( + "os" + "path/filepath" + "strings" "testing" "magician/exec" @@ -26,14 +29,36 @@ type sandbox struct { Runner ExecRunner } +// Routes some commands to fake sandbox scripts, bypassing $PATH. +type interceptingRunner struct { + *exec.Runner + sbDir string +} + +func (s *interceptingRunner) Run(name string, args []string, env map[string]string) (string, error) { + // Only intercept bare commands (git, make, etc.) + if !strings.Contains(name, string(filepath.Separator)) { + sandboxBin := filepath.Join(s.sbDir, name) + if _, err := os.Stat(sandboxBin); err == nil { + name = sandboxBin + } + } + return s.Runner.Run(name, args, env) +} + func newSandbox(t *testing.T) *sandbox { dir := t.TempDir() - runner, err := exec.NewRunner() + realRunner, err := exec.NewRunner() if err != nil { t.Fatalf("Failed to create runner: %v", err) } + runner := &interceptingRunner{ + Runner: realRunner, + sbDir: dir, + } + if err := runner.PushDir(dir); err != nil { t.Fatalf("Failed to push dir: %v", err) } From c42734e29783162dc206bedbdf3534847be3b950 Mon Sep 17 00:00:00 2001 From: Majed Alfaraj Date: Fri, 12 Jun 2026 00:02:25 -0700 Subject: [PATCH 2/3] test(magician): refactor vcr-merge-test to use testing sandbox Refactors the test to use the testing sandbox rather than `mockRunner`. To avoid directly executing `gcloud` commands, this refactor includes a `gcloud` bash emulator. This emulator intercepts the CLI commands and translates them to filesys operations, verifying behavioural state without making HTTP requests. --- .ci/magician/cmd/vcr_merge_test.go | 100 ++++++++++++++++------------- 1 file changed, 54 insertions(+), 46 deletions(-) diff --git a/.ci/magician/cmd/vcr_merge_test.go b/.ci/magician/cmd/vcr_merge_test.go index 2eb0c5cdddc8..1f907f937558 100644 --- a/.ci/magician/cmd/vcr_merge_test.go +++ b/.ci/magician/cmd/vcr_merge_test.go @@ -5,8 +5,6 @@ import ( "os" "strings" "testing" - - "github.com/google/go-cmp/cmp" ) func TestExecVCRMerge(t *testing.T) { @@ -15,33 +13,16 @@ func TestExecVCRMerge(t *testing.T) { baseBranch string commitSha string lsReturnedError bool - calledMethods []string }{ { name: "base branch is main", baseBranch: "main", commitSha: "sha", - calledMethods: []string{ - "gcloud storage ls gs://ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures/", - "gcloud storage cp gs://ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures/* gs://ci-vcr-cassettes/fixtures/", - "gcloud storage rm --recursive gs://ci-vcr-cassettes/refs/heads/auto-pr-123/", - "gcloud storage ls gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures/", - "gcloud storage cp gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures/* gs://ci-vcr-cassettes/beta/fixtures/", - "gcloud storage rm --recursive gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/", - }, }, { name: "base branch is not main", baseBranch: "test-branch", commitSha: "sha", - calledMethods: []string{ - "gcloud storage ls gs://ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures/", - "gcloud storage cp gs://ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures/* gs://ci-vcr-cassettes/refs/branches/test-branch/fixtures/", - "gcloud storage rm --recursive gs://ci-vcr-cassettes/refs/heads/auto-pr-123/", - "gcloud storage ls gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures/", - "gcloud storage cp gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures/* gs://ci-vcr-cassettes/beta/refs/branches/test-branch/fixtures/", - "gcloud storage rm --recursive gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/", - }, }, { name: "pr not found", @@ -53,10 +34,6 @@ func TestExecVCRMerge(t *testing.T) { commitSha: "sha", lsReturnedError: true, baseBranch: "main", - calledMethods: []string{ - "gcloud storage ls gs://ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures/", - "gcloud storage ls gs://ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures/", - }, }, } @@ -67,34 +44,65 @@ func TestExecVCRMerge(t *testing.T) { }, calledMethods: make(map[string][][]any), } - for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { - runner := &mockRunner{ - cwd: "cwd", - calledMethods: make(map[string][]ParameterList), - cmdResults: make(map[string]string), - } - if test.lsReturnedError { - runner.notifyError = true + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + sb := newSandbox(t) + + // Intercepts the hardcoded `gcloud storage` commands and translates gs:// URLs into local directory operations. + fakeGcloud := `#!/bin/bash + if [ "$1" = "storage" ]; then + if [ "$2" = "ls" ]; then + TARGET=$(echo $3 | sed 's|gs://|gs/|') + ls "$TARGET" > /dev/null 2>&1 + exit $? + elif [ "$2" = "cp" ]; then + SRC=$(echo $3 | sed 's|gs://|gs/|' | sed 's|/\*$||') + DEST=$(echo $4 | sed 's|gs://|gs/|') + mkdir -p "$DEST" + cp -r "$SRC"/* "$DEST" + elif [ "$2" = "rm" ]; then + TARGET=$(echo $4 | sed 's|gs://|gs/|') + rm -r "$TARGET" + fi + fi` + sb.Runner.WriteFile("gcloud", fakeGcloud) + sb.Runner.MustRun("chmod", []string{"+x", "gcloud"}, nil) + + origPath := os.Getenv("PATH") + os.Setenv("PATH", sb.Dir+":"+origPath) + defer os.Setenv("PATH", origPath) + + if !tc.lsReturnedError && tc.name != "pr not found" { + sb.Runner.MustRun("mkdir", []string{"-p", "gs/ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures"}, nil) + sb.Runner.MustRun("mkdir", []string{"-p", "gs/ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures"}, nil) + sb.Runner.WriteFile("gs/ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures/dummy1.txt", "data") + sb.Runner.WriteFile("gs/ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures/dummy2.txt", "data") } - err := execVCRMerge(githubClient, test.commitSha, test.baseBranch, runner) + + err := execVCRMerge(githubClient, tc.commitSha, tc.baseBranch, sb.Runner) if err != nil { - t.Fatalf("execVCRMerge = %s, want = nil", err) + t.Fatalf("execVCRMerge() failed: %v", err) } - got, ok := runner.Calls("Run") - if !ok && test.calledMethods != nil { - t.Fatalf("execVCRMerge() expect %d calls, got none", len(test.calledMethods)) - } - var want []ParameterList - for _, cmd := range test.calledMethods { - words := strings.Split(cmd, " ") - if len(words) > 0 { - want = append(want, []any{"cwd", words[0], words[1:], map[string]string(nil)}) + if !tc.lsReturnedError && tc.name != "pr not found" { + destBranchPath := "" + if tc.baseBranch != "main" { + destBranchPath = "/refs/branches/" + tc.baseBranch + } + + if _, err := os.Stat(sb.Dir + "/gs/ci-vcr-cassettes" + destBranchPath + "/fixtures/dummy1.txt"); os.IsNotExist(err) { + t.Fatalf("Expected file to be copied to /gs/ci-vcr-cassettes%s/fixtures/dummy1.txt", destBranchPath) + } + if _, err := os.Stat(sb.Dir + "/gs/ci-vcr-cassettes/beta" + destBranchPath + "/fixtures/dummy2.txt"); os.IsNotExist(err) { + t.Fatalf("Expected file to be copied to /gs/ci-vcr-cassettes/beta%s/fixtures/dummy2.txt", destBranchPath) + } + + if _, err := os.Stat(sb.Dir + "/gs/ci-vcr-cassettes/refs/heads/auto-pr-123/"); !os.IsNotExist(err) { + t.Fatalf("Expected source directory /gs/ci-vcr-cassettes/refs/heads/auto-pr-123/ to be deleted") + } + if _, err := os.Stat(sb.Dir + "/gs/ci-vcr-cassettes/beta/refs/heads/auto-pr-123/"); !os.IsNotExist(err) { + t.Fatalf("Expected source directory /gs/ci-vcr-cassettes/beta/refs/heads/auto-pr-123/ to be deleted") } - } - if diff := cmp.Diff(want, got); diff != "" { - t.Fatalf("execVCRMerge() executed commands diff = %s\n want = %+v, got = %+v", diff, want, got) } }) } From e810ee5db6ed98b84a0036ca7d4f9c2d045c26d4 Mon Sep 17 00:00:00 2001 From: Majed Alfaraj Date: Mon, 15 Jun 2026 11:02:11 -0700 Subject: [PATCH 3/3] test(magician): remove PATH manipulation and replace with interceptingRunner Because the new sandbox framework can intercept and reroute binary calls to temporary directories, manipulating $PATH is no longer necessary. Allows for test to be safely run in parallel. --- .ci/magician/cmd/vcr_merge_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.ci/magician/cmd/vcr_merge_test.go b/.ci/magician/cmd/vcr_merge_test.go index 1f907f937558..85125c789ff2 100644 --- a/.ci/magician/cmd/vcr_merge_test.go +++ b/.ci/magician/cmd/vcr_merge_test.go @@ -68,10 +68,6 @@ func TestExecVCRMerge(t *testing.T) { sb.Runner.WriteFile("gcloud", fakeGcloud) sb.Runner.MustRun("chmod", []string{"+x", "gcloud"}, nil) - origPath := os.Getenv("PATH") - os.Setenv("PATH", sb.Dir+":"+origPath) - defer os.Setenv("PATH", origPath) - if !tc.lsReturnedError && tc.name != "pr not found" { sb.Runner.MustRun("mkdir", []string{"-p", "gs/ci-vcr-cassettes/refs/heads/auto-pr-123/fixtures"}, nil) sb.Runner.MustRun("mkdir", []string{"-p", "gs/ci-vcr-cassettes/beta/refs/heads/auto-pr-123/fixtures"}, nil)