Skip to content

Migrate the SetExecRunner tests onto ContextWithRunner #502

Description

@josegonzalez

Description of feature

No test in the repo calls t.Parallel(), and the reason was a single package variable.

subprocess.SetExecRunner swaps the package-level execRunner so a unit test can return canned responses without spawning a process, and its doc comment says outright that the swap mutates package state and is not safe under t.Parallel(). subprocess.ContextWithRunner is the per-invocation replacement, added in #504: it puts the executor on the context the task already receives, takes precedence over the package variable, and lets two tests with different fakes run at the same time. The package variable stays as the fallback, so nothing had to change at once.

This is the migration, and it splits into two passes with very different prerequisites.

Pass one - tasks/ unit tests. Unblocked today. Twenty-nine unit test files use SetExecRunner, across 149 call sites, and there is no t.Setenv and no t.Chdir anywhere in the tasks/ unit suite, so nothing forbids t.Parallel() outright. Twenty-six of the twenty-nine landed in #509. The three exceptions - tasks/properties_test.go, tasks/service_create_task_test.go and tasks/scheduler_k3s_autoscaling_auth_task_test.go - were held back because they set the process-wide mask registry directly. #511 made that registry per-invocation, so they are unblocked and belong to pass two below. Each defer subprocess.SetExecRunner(fake)() becomes a per-test ctx := subprocess.ContextWithRunner(context.Background(), fake) and that test's testCtx() calls take ctx. The 76 *_integration_test.go files are out of scope: none uses the seam, and they share one real Dokku.

Pass two - commands/, plus the three tasks/ files pass one held back. Those three now build a masker of their own rather than mutating a global, so parallelising them is the same mechanical change as the other twenty-six. The commands/ half is blocked on three separate things, none of them the exec seam. The mask registry was the largest and is now handled: #511 gave each run its own masker, so the 34 of 44 test files that drive a command no longer interleave a replace and a clear on one slice. What remains is more than first counted. In rough order of size:

  • The shared stub fixture map (Context-scope the command stub fixtures #506), 14 test files. Keys collide hard - "a" alone is used 38 times - and stubReset() wipes the whole map while another test could be mid-run.
  • subprocess.SetExecRunner in the three export test files, 31 sites. Mechanical: ContextWithRunner already exists and commands/play_target_test.go already uses it through Ctx.
  • The os.Args, working-directory, stdin and stdout coupling (Take the command layer off os.Args and the working directory #505).
  • color.NoColor, 2 test files. This one is not a commands problem: applyColorMode writes the process-wide color.NoColor, and subprocess/exec.go reads it to decide whether a child may use the terminal. So a fmt --color never test running beside an apply test changes how that apply test dispatches. Fixing it means threading the colour decision through the diff renderer rather than the global.
  • commands/ssh_flags_test.go stays serial regardless: it uses t.Setenv, which panics in a parallel test.

One more turned up only once a parallel test actually ran: go-defaults builds its filler lazily into an unsynchronised package variable, and every recipe decode goes through defaults.SetDefaults, so any two tests that load a recipe at the same time race on a library global neither of them can see. #506 settles it by warming the filler in a tasks package init, before any test starts a goroutine of its own. Worth knowing it was invisible to every -race run until something in this package ran concurrently.

One property makes this all-or-nothing rather than incremental. t.Chdir panics if the calling test is parallel, but it does not stop a serial test's chdir moving the working directory out from under a parallel test running beside it. So commands/ cannot be parallelised file by file while any chdir remains - the working-directory work has to land as one sweep before the first t.Parallel() goes on here.

Two pieces of global state leaked past the serial phase and had to be fixed before the first t.Parallel() could go on, since Go resumes parallel tests only after every serial test has finished and its cleanups have run. Both are handled in #507: four property tests were leaving a secret registered for every test that ran after them, hidden because the tests that touched the registry directly cleared it on the way out, and commands/fmt_test.go restored color.NoColor by assuming the previous value rather than saving it. TestMain now fails the tasks run if the registry is dirty at the end, so this cannot come back silently.

Once both passes land, SetExecRunner can be deleted along with its warning, and the -race step added in #504 covers a suite that is actually concurrent.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions