Merge cockroachdb#69020

69020: rttanalysis: add rttanalysisccl.TestBenchmarkExpectations r=postamar a=postamar

Previously, we had no convenient way to adjust the roundtrip benchmark
expectations in the CCL package. This commit adds a new test for this
purpose and in so doing exports a bunch of test utilities in
rttanalysis.

Release note: None

Co-authored-by: Marius Posta <[email protected]>

pkg/bench/rttanalysis/validate_benchmark_data.go

@@ -22,6 +22,7 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -56,6 +57,63 @@ var (
 			"determine the range of possible values")
 )
 
+// RunBenchmarkExpectationTests runs tests to validate or rewrite the contents
+// of the benchmark expectations file.
+func RunBenchmarkExpectationTests(t *testing.T) {
+	expectations := readExpectationsFile(t)
+
+	benchmarks := getBenchmarks(t)
+	if *rewriteFlag != "" {
+		rewriteBenchmarkExpectations(t, benchmarks)
+		return
+	}
+
+	defer func() {
+		if t.Failed() {
+			t.Log("see the --rewrite flag to re-run the benchmarks and adjust the expectations")
+		}
+	}()
+
+	var g sync.WaitGroup
+	run := func(b string) {
+		tf := func(t *testing.T) {
+			flags := []string{
+				"--test.run=^$",
+				"--test.bench=" + b,
+				"--test.benchtime=1x",
+			}
+			if testing.Verbose() {
+				flags = append(flags, "--test.v")
+			}
+			results := runBenchmarks(t, flags...)
+
+			for _, r := range results {
+				exp, ok := expectations.find(r.name)
+				if !ok {
+					t.Logf("no expectation for benchmark %s, got %d", r.name, r.result)
+					continue
+				}
+				if !exp.matches(r.result) {
+					t.Errorf("fail: expected %s to perform KV lookups in [%d, %d], got %d",
+						r.name, exp.min, exp.max, r.result)
+				} else {
+					t.Logf("success: expected %s to perform KV lookups in [%d, %d], got %d",
+						r.name, exp.min, exp.max, r.result)
+				}
+			}
+		}
+		g.Add(1)
+		go func() {
+			defer g.Done()
+			t.Run(b, tf)
+		}()
+	}
+	for _, b := range benchmarks {
+		run(b)
+	}
+	g.Wait()
+}
+
 func getBenchmarks(t *testing.T) (benchmarks []string) {
 	cmd := exec.Command(os.Args[0], "--test.list", "^Benchmark")
 	var out bytes.Buffer
@@ -68,6 +126,7 @@ func getBenchmarks(t *testing.T) (benchmarks []string) {
 	require.NoError(t, sc.Err())
 	return benchmarks
 }
+
 func runBenchmarks(t *testing.T, flags ...string) []benchmarkResult {
 	cmd := exec.Command(os.Args[0], flags...)
 
@@ -120,7 +179,7 @@ func readBenchmarkResults(t *testing.T, benchmarkOutput io.Reader) []benchmarkRe
 
 // rewriteBenchmarkExpectations re-runs the specified benchmarks and throws out
 // the existing values in the results file. All other values are preserved.
-func rewriteBenchmarkExpecations(t *testing.T, benchmarks []string) {
+func rewriteBenchmarkExpectations(t *testing.T, benchmarks []string) {
 
 	// Split off the filter so as to avoid spinning off unnecessary subprocesses.
 	slashIdx := strings.Index(*rewriteFlag, "/")

pkg/bench/rttanalysis/validate_benchmark_data_test.go

@@ -11,7 +11,6 @@
 package rttanalysis
 
 import (
-	"sync"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
@@ -28,56 +27,5 @@ func TestBenchmarkExpectation(t *testing.T) {
 	skip.UnderRace(t)
 	skip.UnderShort(t)
 
-	expecations := readExpectationsFile(t)
-
-	benchmarks := getBenchmarks(t)
-	if *rewriteFlag != "" {
-		rewriteBenchmarkExpecations(t, benchmarks)
-		return
-	}
-
-	defer func() {
-		if t.Failed() {
-			t.Log("see the --rewrite flag to re-run the benchmarks and adjust the expectations")
-		}
-	}()
-
-	var g sync.WaitGroup
-	run := func(b string) {
-		tf := func(t *testing.T) {
-			flags := []string{
-				"--test.run=^$",
-				"--test.bench=" + b,
-				"--test.benchtime=1x",
-			}
-			if testing.Verbose() {
-				flags = append(flags, "--test.v")
-			}
-			results := runBenchmarks(t, flags...)
-
-			for _, r := range results {
-				exp, ok := expecations.find(r.name)
-				if !ok {
-					t.Logf("no expectation for benchmark %s, got %d", r.name, r.result)
-					continue
-				}
-				if !exp.matches(r.result) {
-					t.Errorf("fail: expected %s to perform KV lookups in [%d, %d], got %d",
-						r.name, exp.min, exp.max, r.result)
-				} else {
-					t.Logf("success: expected %s to perform KV lookups in [%d, %d], got %d",
-						r.name, exp.min, exp.max, r.result)
-				}
-			}
-		}
-		g.Add(1)
-		go func() {
-			defer g.Done()
-			t.Run(b, tf)
-		}()
-	}
-	for _, b := range benchmarks {
-		run(b)
-	}
-	g.Wait()
+	RunBenchmarkExpectationTests(t)
 }

pkg/ccl/benchccl/rttanalysisccl/BUILD.bazel

@@ -22,6 +22,7 @@ go_test(
     srcs = [
         "bench_test.go",
         "multi_region_bench_test.go",
+        "validate_benchmark_data_test.go",
     ],
     data = glob(["testdata/**"]),
     embed = [":rttanalysisccl"],
@@ -33,6 +34,7 @@ go_test(
         "//pkg/security/securitytest",
         "//pkg/server",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/testcluster",
         "//pkg/util/randutil",
     ],

pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations

@@ -1,19 +1,19 @@
 exp,benchmark
+25,AlterPrimaryRegion/alter_empty_database_alter_primary_region
+25,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region
+25,AlterPrimaryRegion/alter_populated_database_alter_primary_region
+26,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region
 22,AlterRegions/alter_empty_database_add_region
 23,AlterRegions/alter_empty_database_drop_region
 22,AlterRegions/alter_populated_database_add_region
 23,AlterRegions/alter_populated_database_drop_region
-25,AlterPrimaryRegion/alter_empty_database_set_initial_primary_region
-25,AlterPrimaryRegion/alter_empty_database_alter_primary_region
-26,AlterPrimaryRegion/alter_populated_database_set_initial_primary_region
-25,AlterPrimaryRegion/alter_populated_database_alter_primary_region
-25,AlterSurvivalGoals/alter_empty_database_from_zone_to_region
 25,AlterSurvivalGoals/alter_empty_database_from_region_to_zone
-65,AlterSurvivalGoals/alter_populated_database_from_zone_to_region
+25,AlterSurvivalGoals/alter_empty_database_from_zone_to_region
 65,AlterSurvivalGoals/alter_populated_database_from_region_to_zone
+65,AlterSurvivalGoals/alter_populated_database_from_zone_to_region
+24,AlterTableLocality/alter_from_global_to_rbr
 28,AlterTableLocality/alter_from_global_to_regional_by_table
+22,AlterTableLocality/alter_from_rbr_to_global
+22,AlterTableLocality/alter_from_rbr_to_regional_by_table
 28,AlterTableLocality/alter_from_regional_by_table_to_global
-24,AlterTableLocality/alter_from_global_to_rbr
 24,AlterTableLocality/alter_from_regional_by_table_to_rbr
-22,AlterTableLocality/alter_from_rbr_to_regional_by_table
-22,AlterTableLocality/alter_from_rbr_to_global

pkg/ccl/benchccl/rttanalysisccl/validate_benchmark_data_test.go

@@ -0,0 +1,30 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package rttanalysisccl
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/bench/rttanalysis"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
+)
+
+// TestBenchmarkExpectation runs all of the benchmarks and
+// one iteration and validates that the number of RPCs meets
+// the expectation.
+//
+// It takes a long time and thus is skipped under stress, race
+// and short.
+func TestBenchmarkExpectation(t *testing.T) {
+	skip.UnderStress(t)
+	skip.UnderRace(t)
+	skip.UnderShort(t)
+
+	rttanalysis.RunBenchmarkExpectationTests(t)
+}