test: run the commands and tasks suites in parallel - #514
Merged
Conversation
Closes the second pass of #502. The command layer's tests were the last ones held serial, and the three globals standing in their way are gone: export's tests move off `subprocess.SetExecRunner` onto the run context, the formatter builds its own color objects instead of reading `color.NoColor`, and the three `tasks/` files that #502a left behind convert with the rest. Decoupling the formatter from `color.NoColor` means that flag no longer answers two unrelated questions. `noColorDefault` picks up the `TERM=dumb` case it was leaning on fatih/color for, and `subprocess` asks isatty directly rather than reading a colouring decision to decide whether a child may inherit stdin - which is why `NO_COLOR=1` used to stop it. `ssh_flags_test.go` stays serial: it sets environment variables, which `t.Setenv` forbids a parallel test from doing.
Every test now carries its fake on the run context, so the swappable package variable behind `SetExecRunner` has no users left. Its mutex went with it: nothing but a test ever wrote the variable, and a fallback that cannot be reassigned needs no lock. The two tests written against the setter are rewritten rather than dropped. One pinned that a fake receives the input and that a bare call reaches the real executor, which is still worth having; the other pinned precedence between the two seams, and with one seam gone the only precedence left is between a context and the one it derives from.
The helper stopped assigning os.Args and os.Stdin when the command layer moved onto fields, and the memo reset it named no longer exists.
Closes #515. `parseDokkuHost` and `resolveSshFlags` take the environment lookup as a parameter, so a test can state its own answers rather than arrange the process environment with `t.Setenv`, which panics in a parallel test. Production passes `os.Getenv` and behaves as before. Four tests in `subprocess` were clearing `DOKKU_SUDO` and `DOKKU_SSH_ACCEPT_NEW_HOST_KEYS` before building an SSH argv. Nothing has read either variable there since they moved onto an explicit `Target`, so the clearing goes rather than moves. The fallback to the current user, which neither variable reaches, is now covered too. Two tests stay serial and say why: one asserts a child inherits docket's environment, which a stub cannot show, and the other reads its trace back through the standard logger's output, which is a second global.
Closes #516. The package was never swept: the tests that had `t.Parallel()` got it one at a time as other work happened to touch them, and the rest were serial for no reason - pure functions over their arguments, with no shared fixture and, since #502, no swappable runner to contend for. Five stay serial and now carry the reason. One counts `runtime.NumGoroutine()`, which is process-wide and would put a neighbour's goroutines in its delta; one asserts a wall-clock ceiling, which is how a suite acquires a flake; two are the environment cases #515 documented; and one writes `color.NoColor` deliberately, to show nothing reads it any more.
Rendering a recipe goes through sigil, which exports each template variable into the process environment and restores it on the way out by replaying a snapshot through `os.Clearenv`. Two renders at once interleave a wipe with a restore, and because each replays the snapshot it took, a variable set between the two is dropped from the process for good - with the command tests running in parallel an unlocked render emptied the whole environment, `PATH` and `HOME` included. Every render now goes through `tasks.RenderTemplate`, which holds a mutex. docket renders a handful of times per run over a file already in memory, so there is nothing here worth contending for. The transient half is filed as #517 and not fixed: between a render's `Clearenv` and its replay the environment is empty, and a lock held by renders does nothing for a concurrent reader. That is why the two tests asserting on the whole environment are serial.
Closes #517. `sigil.Execute` exports every string template variable with `os.Setenv` so its POSIX preprocessor can expand `$VAR`, then restores the environment on the way out by replaying a snapshot through `os.Clearenv`. The preprocessor is gated on `sigil.PosixPreprocess`, which only sigil's own command sets, so for docket every one of those writes was collateral damage - and they left a window, between the `Clearenv` and the replay, where any other goroutine reading the environment or spawning a child saw nothing. `tasks.RenderTemplate` is now a reimplementation of `Execute` that renders without writing the environment: the same `$var` scan, the same prelude, the same escaped-delimiter fixup, the same `SIGIL_DELIMS` override, and sigil's own builtins. `include` and `render` are docket's, because sigil's recurse into `Execute` and would reintroduce the writes at one remove. The trade is that docket no longer honours `PosixPreprocess`, which it never enabled. Owning the function map also settles which filters a render has. Sigil keeps one package-global map that `sigil/builtin` fills from an init, so a builtin existed only if something in the binary imported that package - `main.go` did and some test files did, but the commands test binary did not, leaving `{{ .app | upper }}` working in the CLI and failing to parse in those tests. There is one map now and it is the same everywhere. The render lock stays, for sigil's template path stack rather than the environment: `include` pushes and pops it through package state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #502. Closes #515. Closes #516. Closes #517. The process globals that kept
commands/andsubprocessserial are gone, and both suites now run in parallel.The command layer's tests were the ones held serial longest. Export's tests move off
subprocess.SetExecRunneronto the run context, the formatter builds its own color objects instead of readingcolor.NoColor, and the threetasks/files that #502a left behind convert with the rest.Decoupling the formatter from
color.NoColormeans that flag no longer answers two unrelated questions.noColorDefaultpicks up theTERM=dumbcase it was leaning on fatih/color for, andsubprocessasks isatty directly rather than reading a colouring decision to decide whether a child may inherit stdin - which is whyNO_COLOR=1used to stop it.With every test carrying its fake on the context, the swappable package variable behind
SetExecRunnerhas no users left and goes too, along with the mutex that only existed to keep a test writing it from racing a test reading it. The two tests written against the setter are rewritten rather than dropped: one pinned that a fake receives the input and that a bare call reaches the real executor, and the other pinned precedence between the two seams, which is now precedence between a context and the one it derives from.The last serial tests were the ones arranging environment variables, so
parseDokkuHostandresolveSshFlagstake the lookup as a parameter and production passesos.Getenv. Four of those tests were clearingDOKKU_SUDOandDOKKU_SSH_ACCEPT_NEW_HOST_KEYSagainst a function that has not read either since they moved onto an explicitTarget, so that clearing goes rather than moves.subprocessis then swept the waytasks/andcommands/were.Parallelising
commands/surfaced a real bug rather than a test problem. Rendering a recipe goes through sigil, which exports each template variable into the process environment and restores it by replaying a snapshot throughos.Clearenv. Two renders at once interleave a wipe with a restore, and because each replays the snapshot it took, an unlocked render emptied the entire environment -PATHandHOMEincluded. Those writes existed only to feed sigil's POSIX preprocessor, which is gated on a flag docket has never set, sotasks.RenderTemplateis now a reimplementation ofExecutethat renders without touching the environment at all: the same$varscan, the same prelude, the same escaped-delimiter fixup, the sameSIGIL_DELIMSoverride, and sigil's own builtins.includeandrenderare docket's, because sigil's recurse intoExecute. A render lock remains, for sigil's template path stack rather than the environment.Owning the function map settles a second inconsistency. Sigil keeps one package-global map that
sigil/builtinfills from an init, so a builtin existed only if something in the binary imported that package -main.godid and some test files did, but the commands test binary did not, leaving{{ .app | upper }}working in the CLI and failing to parse in those tests.Seven tests stay serial across the three packages, each saying why in place: two are the colour-detection cases, whose subject is the environment; one counts
runtime.NumGoroutine(), which is process-wide; one asserts a wall-clock ceiling; one asserts a child inherits docket's environment; one reads its trace back through the standard logger's output; and one writescolor.NoColordeliberately, to show nothing reads it any more. Go resumes parallel tests only once every serial test and its cleanups have finished, so those seven keep working beside the rest.This buys little wall-clock time - the suites are mocked and were already fast. The point is that
-racenow watches a concurrent program rather than a serial one.