From 4512b10c1c98f3758fdb688ef4f40c9ec46de320 Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Wed, 1 May 2024 23:24:54 +0000 Subject: [PATCH] skip,metamorphic: break dependency on `metamorphic` from `skip` It would be nice to have a way to clearly determine whether any one package uses metamorphic constants, however that's difficult since `pkg/testutils/skip` (which many test packages use) depends on `metamorphic`. This PR breaks that dependency, so we can query the dependency graph to the `metamorphic` dependency to determine exactly which packages use metamorphic constants. Instead, we have `skip` depend on a new package `metamorphicutil` which is very restricted in use. `metamorphic` can also do setup in its `init()` function that we know for sure is not needed unless metamorphic constants are specifically requested. Epic: none Release note: None --- pkg/BUILD.bazel | 2 ++ pkg/testutils/skip/BUILD.bazel | 11 +++++++- pkg/testutils/skip/skip.go | 6 ++--- pkg/util/metamorphic/BUILD.bazel | 1 + pkg/util/metamorphic/constants.go | 25 +++++++++---------- .../metamorphic/metamorphicutil/BUILD.bazel | 11 ++++++++ .../metamorphicutil/is_metamorphic.go | 23 +++++++++++++++++ 7 files changed, 62 insertions(+), 17 deletions(-) create mode 100644 pkg/util/metamorphic/metamorphicutil/BUILD.bazel create mode 100644 pkg/util/metamorphic/metamorphicutil/is_metamorphic.go diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 9aa682818394..d62700254ce3 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -642,6 +642,7 @@ ALL_TESTS = [ "//pkg/testutils/listenerutil:listenerutil_test", "//pkg/testutils/release:release_test", "//pkg/testutils/serverutils:serverutils_test", + "//pkg/testutils/skip:skip_disallowed_imports_test", "//pkg/testutils/sqlutils:sqlutils_test", "//pkg/testutils/testcluster:testcluster_test", "//pkg/testutils/zerofields:zerofields_test", @@ -2464,6 +2465,7 @@ GO_TARGETS = [ "//pkg/util/log:log", "//pkg/util/log:log_test", "//pkg/util/memzipper:memzipper", + "//pkg/util/metamorphic/metamorphicutil:metamorphicutil", "//pkg/util/metamorphic:metamorphic", "//pkg/util/metamorphic:metamorphic_test", "//pkg/util/metric/aggmetric:aggmetric", diff --git a/pkg/testutils/skip/BUILD.bazel b/pkg/testutils/skip/BUILD.bazel index 719d9803b890..d1d03f197e80 100644 --- a/pkg/testutils/skip/BUILD.bazel +++ b/pkg/testutils/skip/BUILD.bazel @@ -1,4 +1,5 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("//pkg/testutils:buildutil/buildutil.bzl", "disallowed_imports_test") go_library( name = "skip", @@ -14,7 +15,15 @@ go_library( "//pkg/util", "//pkg/util/buildutil", "//pkg/util/envutil", - "//pkg/util/metamorphic", + "//pkg/util/metamorphic/metamorphicutil", "//pkg/util/syncutil", ], ) + +disallowed_imports_test( + "skip", + disallow_cdeps = True, + disallowed_list = [ + "//pkg/util/metamorphic", + ], +) diff --git a/pkg/testutils/skip/skip.go b/pkg/testutils/skip/skip.go index 38aec69a6b9c..8e1e3c47be67 100644 --- a/pkg/testutils/skip/skip.go +++ b/pkg/testutils/skip/skip.go @@ -21,7 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/envutil" - "github.com/cockroachdb/cockroach/pkg/util/metamorphic" + "github.com/cockroachdb/cockroach/pkg/util/metamorphic/metamorphicutil" "github.com/cockroachdb/cockroach/pkg/util/syncutil" ) @@ -160,7 +160,7 @@ func UnderStressRaceWithIssue(t SkippableTest, githubIssueID int, args ...interf // run with the metamorphic build tag. func UnderMetamorphic(t SkippableTest, args ...interface{}) { t.Helper() - if metamorphic.IsMetamorphicBuild() { + if metamorphicutil.IsMetamorphicBuild { maybeSkip(t, "disabled under metamorphic", args...) } } @@ -170,7 +170,7 @@ func UnderMetamorphic(t SkippableTest, args ...interface{}) { // reason. func UnderMetamorphicWithIssue(t SkippableTest, githubIssueID int, args ...interface{}) { t.Helper() - if metamorphic.IsMetamorphicBuild() { + if metamorphicutil.IsMetamorphicBuild { maybeSkip(t, withIssue("disabled under metamorphic", githubIssueID), args...) } } diff --git a/pkg/util/metamorphic/BUILD.bazel b/pkg/util/metamorphic/BUILD.bazel index 8a00dc00ed94..f9fd4b5416e8 100644 --- a/pkg/util/metamorphic/BUILD.bazel +++ b/pkg/util/metamorphic/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//pkg/build/bazel", "//pkg/util/buildutil", "//pkg/util/envutil", + "//pkg/util/metamorphic/metamorphicutil", "//pkg/util/randutil", "//pkg/util/syncutil", ], diff --git a/pkg/util/metamorphic/constants.go b/pkg/util/metamorphic/constants.go index 82078a785d74..fed457df7655 100644 --- a/pkg/util/metamorphic/constants.go +++ b/pkg/util/metamorphic/constants.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/build/bazel" "github.com/cockroachdb/cockroach/pkg/util/buildutil" + "github.com/cockroachdb/cockroach/pkg/util/metamorphic/metamorphicutil" "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/cockroachdb/cockroach/pkg/util/syncutil" ) @@ -24,24 +25,22 @@ import ( // IsMetamorphicBuild returns whether this build is metamorphic. By build being // "metamorphic" we mean that some magic constants in the codebase might get // initialized to non-default value. -// A build will become metamorphic with metamorphicBuildProbability probability +// A build will become metamorphic with metamorphicutil.IsMetamorphicBuildProbability probability // if 'crdb_test' build flag is specified (this is the case for all test // targets). func IsMetamorphicBuild() bool { - return metamorphicBuild + return metamorphicutil.IsMetamorphicBuild } -var metamorphicBuild bool - const ( - metamorphicBuildProbability = 0.8 - metamorphicValueProbability = 0.75 - metamorphicBoolProbability = 0.5 + IsMetamorphicBuildProbability = 0.8 + metamorphicValueProbability = 0.75 + metamorphicBoolProbability = 0.5 ) // ConstantWithTestValue should be used to initialize "magic constants" that // should be varied during test scenarios to check for bugs at boundary -// conditions. When metamorphicBuild is true, the test value will be used with +// conditions. When metamorphicutil.IsMetamorphicBuild is true, the test value will be used with // metamorphicValueProbability probability. In all other cases, the production // ("default") value will be used. // The constant must be a "metamorphic variable": changing it cannot affect the @@ -67,7 +66,7 @@ const ( // // The given name is used for logging. func ConstantWithTestValue(name string, defaultValue, metamorphicValue int) int { - if metamorphicBuild { + if metamorphicutil.IsMetamorphicBuild { rng.Lock() defer rng.Unlock() if rng.r.Float64() < metamorphicValueProbability { @@ -110,7 +109,7 @@ func init() { if metamorphicEligible() { if !disableMetamorphicTesting { rng.r, _ = randutil.NewTestRand() - metamorphicBuild = rng.r.Float64() < metamorphicBuildProbability + metamorphicutil.IsMetamorphicBuild = rng.r.Float64() < IsMetamorphicBuildProbability } } } @@ -121,7 +120,7 @@ func init() { // // The given name is used for logging. func ConstantWithTestRange(name string, defaultValue, min, max int) int { - if metamorphicBuild { + if metamorphicutil.IsMetamorphicBuild { rng.Lock() defer rng.Unlock() if rng.r.Float64() < metamorphicValueProbability { @@ -145,7 +144,7 @@ func ConstantWithTestBool(name string, defaultValue bool) bool { } func constantWithTestBoolInternal(name string, defaultValue bool, doLog bool) bool { - if metamorphicBuild { + if metamorphicutil.IsMetamorphicBuild { rng.Lock() defer rng.Unlock() if rng.r.Float64() < metamorphicBoolProbability { @@ -175,7 +174,7 @@ func ConstantWithTestBoolWithoutLogging(name string, defaultValue bool) bool { func ConstantWithTestChoice( name string, defaultValue interface{}, otherValues ...interface{}, ) interface{} { - if metamorphicBuild { + if metamorphicutil.IsMetamorphicBuild { values := append([]interface{}{defaultValue}, otherValues...) rng.Lock() defer rng.Unlock() diff --git a/pkg/util/metamorphic/metamorphicutil/BUILD.bazel b/pkg/util/metamorphic/metamorphicutil/BUILD.bazel new file mode 100644 index 000000000000..2de68dcf10a9 --- /dev/null +++ b/pkg/util/metamorphic/metamorphicutil/BUILD.bazel @@ -0,0 +1,11 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "metamorphicutil", + srcs = ["is_metamorphic.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/util/metamorphic/metamorphicutil", + visibility = [ + "//pkg/testutils/skip:__pkg__", + "//pkg/util/metamorphic:__pkg__", + ], +) diff --git a/pkg/util/metamorphic/metamorphicutil/is_metamorphic.go b/pkg/util/metamorphic/metamorphicutil/is_metamorphic.go new file mode 100644 index 000000000000..566abbf8ba8b --- /dev/null +++ b/pkg/util/metamorphic/metamorphicutil/is_metamorphic.go @@ -0,0 +1,23 @@ +// Copyright 2024 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package metamorphicutil + +// NB: init() in pkg/util/metamorphic/constants.go may set this value to true. +// We don't put this variable in that package as we would like to have a way to +// reliably determine which packages use metamorphic constants, and +// `bazel query somepath(_, //pkg/util/metamorphic)` should be a good way to do +// that. However, some packages (like pkg/testutils/skip) need to check whether +// we're running a metamorphic build, without necessarily depending on the +// metamorphic package. +// +// Generally, you should use metamorphic.IsMetaMorphicBuild() instead of checking +// this value. +var IsMetamorphicBuild bool