test: move the tasks unit tests onto a per-context exec seam - #509
Merged
Conversation
Twenty-six of the twenty-nine `tasks` unit test files installed their fake executor by swapping a package variable, which is why nothing in this repo could call `t.Parallel()`. They now build the fake into the context they already pass to `Plan` and `Execute`, through `subprocess.ContextWithRunner`, and every test in those files runs in parallel - 149 call sites, 280 tests. This buys no wall-clock time: the suite is mocked end to end and finishes in about a second either way. What it buys is the `-race` step actually exercising concurrency. Until now every test ran alone, so the detector was watching a serial program and the mutex the package runner grew when the seam moved onto the context was never exercised by anything. It is now, along with the concurrent readers of the mask registry. The three files left behind are the ones that set the mask registry directly - `properties_test.go`, `service_create_task_test.go` and `scheduler_k3s_autoscaling_auth_task_test.go`. Isolating the registry per test is #501; they keep `SetExecRunner` until then, which is also what keeps that seam alive for the `commands` package. Two tests needed more than the mechanical change. `exportResource` performs the `ExportRecipe` call for its callers, so it takes their context rather than building its own. And the two context-propagation tests build a context that carries a sentinel or a cancellation, so the runner goes onto that context instead of a fresh one - which is the more honest shape anyway, since the point of both is that the caller's context is the one that reaches the probe. Verified with `-shuffle=on` across seeds and at `-parallel 32`, both under `-race`.
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.
Twenty-six of the twenty-nine
tasksunit test files installed their fake executor by swapping a package variable, which is why nothing in this repo could callt.Parallel(). They now build the fake into the context they already pass toPlanandExecute, throughsubprocess.ContextWithRunner, and every test in those files runs in parallel - 149 call sites, 280 tests.This buys no wall-clock time: the suite is mocked end to end and finishes in about a second either way. What it buys is the
-racestep actually exercising concurrency. Until now every test ran alone, so the detector was watching a serial program and the mutex the package runner grew when the seam moved onto the context was never exercised by anything. It is now, along with the concurrent readers of the mask registry.The three files left behind are the ones that set the mask registry directly -
properties_test.go,service_create_task_test.goandscheduler_k3s_autoscaling_auth_task_test.go. Isolating the registry per test is #501; they keepSetExecRunneruntil then, which is also what keeps that seam alive for thecommandspackage.Two tests needed more than the mechanical change.
exportResourceperforms theExportRecipecall for its callers, so it takes their context rather than building its own. And the two context-propagation tests build a context that carries a sentinel or a cancellation, so the runner goes onto that context instead of a fresh one - which is the more honest shape anyway, since the point of both is that the caller's context is the one that reaches the probe.Verified with
-shuffle=onacross seeds and at-parallel 32, both under-race.First of the two passes in #502. The
commandshalf needs #501, #505 and #506 first.