Skip to content

Commit afef2d2

Browse files
craig[bot]tbg
andcommitted
Merge #150774
150774: asim: run no_rand tests in parallel r=tbg a=tbg Each simulation is single-threaded, so it makes sense to evaluate different testfiles in parallel. A complication is that each simulation logs using our default logging library, and since that is a singleton, we can't provide each simulation with its own logger. When a subtest is a parallel test (as is the case here), it waits for the parent test to return before running. This means that the `log.Scope` in `TestDataDriven` would be cleaned up by the time the parallel subtests would actually start simulating. I introduced a refcounting version of `log.Scope` instead, so the scope will be closed only when the refcount drops to zero. Each subtest will log the scope it's using, so the logs can be found, and should then be filtered by the log tag which is specific to the simulation of interest. ``` ./dev test pkg/kv/kvserver/asim/tests -f 'TestDataDriven' --ignore-cache --rewrite -v -- --test_env COCKROACH_RUN_ASIM_TESTS=true //pkg/kv/kvserver/asim/tests:tests_test PASSED in 202.1s # before //pkg/kv/kvserver/asim/tests:tests_test PASSED in 60.6s # after ``` Epic: CRDB-25222 Co-authored-by: Tobias Grieger <[email protected]>
2 parents 7301dbc + 12abfa7 commit afef2d2

File tree

2 files changed

+20
-7
lines changed

2 files changed

+20
-7
lines changed

pkg/kv/kvserver/asim/tests/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ go_test(
5656
"//pkg/util/leaktest",
5757
"//pkg/util/log",
5858
"@com_github_cockroachdb_datadriven//:datadriven",
59+
"@com_github_cockroachdb_logtags//:logtags",
5960
"@com_github_stretchr_testify//require",
6061
"@org_gonum_v1_plot//:plot",
6162
"@org_gonum_v1_plot//plotter",

pkg/kv/kvserver/asim/tests/datadriven_simulation_test.go

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
3636
"github.com/cockroachdb/cockroach/pkg/util/log"
3737
"github.com/cockroachdb/datadriven"
38+
"github.com/cockroachdb/logtags"
3839
"github.com/stretchr/testify/require"
3940
"gonum.org/v1/plot"
4041
"gonum.org/v1/plot/plotter"
@@ -182,17 +183,27 @@ var runAsimTests = envutil.EnvOrDefaultBool("COCKROACH_RUN_ASIM_TESTS", false)
182183
// ..US_3
183184
// ....└── [11 12 13 14 15]
184185
func TestDataDriven(t *testing.T) {
185-
defer leaktest.AfterTest(t)()
186-
defer log.Scope(t).Close(t)
187186
skip.UnderDuressWithIssue(t, 149875)
188-
ctx := context.Background()
187+
leakTestAfter := leaktest.AfterTest(t)
189188
dir := datapathutils.TestDataPath(t, "non_rand")
189+
190+
// NB: we must use t.Cleanup instead of deferring this
191+
// cleanup in the main test due to the use of t.Parallel
192+
// below.
193+
// See https://github.com/golang/go/issues/31651.
194+
scope := log.Scope(t)
195+
t.Cleanup(func() {
196+
scope.Close(t)
197+
leakTestAfter()
198+
})
190199
datadriven.Walk(t, dir, func(t *testing.T, path string) {
191-
if filepath.Ext(path) == ".png" {
192-
// TODO(tbg): make this != "txt" once all test files have that suffix.
200+
if filepath.Ext(path) != ".txt" {
193201
return
194202
}
195-
plotNum := 0
203+
ctx := logtags.AddTag(context.Background(), "name", filepath.Base(path))
204+
// The inline comment below is required for TestLint/TestTParallel.
205+
// We use t.Cleanup to work around the issue this lint is trying to prevent.
206+
t.Parallel() // SAFE FOR TESTING
196207
const defaultKeyspace = 10000
197208
loadGen := gen.MultiLoad{}
198209
var clusterGen gen.ClusterGen
@@ -201,7 +212,8 @@ func TestDataDriven(t *testing.T) {
201212
eventGen := gen.NewStaticEventsWithNoEvents()
202213
assertions := []assertion.SimulationAssertion{}
203214
var stateStrAcrossSamples []string
204-
runs := []history.History{}
215+
var runs []history.History
216+
plotNum := 0
205217
datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
206218
defer func() {
207219
require.Empty(t, d.CmdArgs, "leftover arguments for %s", d.Cmd)

0 commit comments

Comments
 (0)