From aaa68947d1a7a9faf38487edebf24fe14b5d246f Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Mon, 16 Aug 2021 19:53:49 -0400 Subject: [PATCH] rttanalysis: add rttanalysisccl.TestBenchmarkExpectations 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 --- .../rttanalysis/validate_benchmark_data.go | 61 ++++++++++++++++++- .../validate_benchmark_data_test.go | 54 +--------------- pkg/ccl/benchccl/rttanalysisccl/BUILD.bazel | 2 + .../testdata/benchmark_expectations | 18 +++--- .../validate_benchmark_data_test.go | 30 +++++++++ 5 files changed, 102 insertions(+), 63 deletions(-) create mode 100644 pkg/ccl/benchccl/rttanalysisccl/validate_benchmark_data_test.go diff --git a/pkg/bench/rttanalysis/validate_benchmark_data.go b/pkg/bench/rttanalysis/validate_benchmark_data.go index f84f74b3bae4..b56ec0cb507d 100644 --- a/pkg/bench/rttanalysis/validate_benchmark_data.go +++ b/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, "/") diff --git a/pkg/bench/rttanalysis/validate_benchmark_data_test.go b/pkg/bench/rttanalysis/validate_benchmark_data_test.go index e3a9f734ea7e..52cea4b144fc 100644 --- a/pkg/bench/rttanalysis/validate_benchmark_data_test.go +++ b/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) } diff --git a/pkg/ccl/benchccl/rttanalysisccl/BUILD.bazel b/pkg/ccl/benchccl/rttanalysisccl/BUILD.bazel index 6cc8e798ee02..67a2ec223987 100644 --- a/pkg/ccl/benchccl/rttanalysisccl/BUILD.bazel +++ b/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", ], diff --git a/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations b/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations index 51835a4441e4..d311d65fc262 100644 --- a/pkg/ccl/benchccl/rttanalysisccl/testdata/benchmark_expectations +++ b/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 diff --git a/pkg/ccl/benchccl/rttanalysisccl/validate_benchmark_data_test.go b/pkg/ccl/benchccl/rttanalysisccl/validate_benchmark_data_test.go new file mode 100644 index 000000000000..08f306210822 --- /dev/null +++ b/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) +}