Merge #106206 #106343 #106411

106206: prereqs: delete tests r=rail a=rickystewart These tests have always been skipped under Bazel because the implementation doesn't work in a Bazel world due to the dependency on `"golang.org/x/tools/go/packages"`. Since the command is only useful/ used in `make`, which is going to be deleted shortly, just delete the tests rather than waste time getting it working. Also this is the last `broken_in_bazel` test, so rip out all the corresponding logic too. Epic: none Release note: None Closes: #61924 Closes: #92814 106343: ci: don't do retries on tests in CI on master, release branches r=rail a=rickystewart First of all, test retries don't even have the correct behavior: #103042 This means that a successfully-retried test tramples the logs of previously-failed tests, which is very confusing and erases your ability to debug the test. Also, we are focusing on quality and wiping out flaky and skipped tests. This to me suggests we should not be retrying tests to let already-flaky tests through. Rather, we should be surfacing real failures immediately. For both of these reasons I turn off test retries for unit tests on `master` and release branches. We keep it for `staging` so `bors` is unaffected. Epic: none Release note: None 106411: kvflowhandle: fix mutex leak r=irfansharif a=irfansharif Fixes #106078. We were forgetting to unlock the mutex on the error path. Release note: None Co-authored-by: Ricky Stewart <[email protected]> Co-authored-by: irfan sharif <[email protected]>
cockroachdb · Jul 7, 2023 · 42922d0 · 42922d0
4 parents d802aaf + 9ed7b15 + b9f4159 + c7f3f8f
commit 42922d0
Show file tree

Hide file tree

Showing 10 changed files with 9 additions and 223 deletions.
diff --git a/build/bazelutil/check.sh b/build/bazelutil/check.sh
@@ -52,10 +52,6 @@ pkg/util/log/sinks.go://go:generate mockgen -package=log -destination=mocks_gene
 pkg/util/timeutil/zoneinfo.go://go:generate go run gen/main.go
 "
 
-EXISTING_BROKEN_TESTS_IN_BAZEL="
-pkg/cmd/prereqs/BUILD.bazel
-"
-
 EXISTING_CRDB_TEST_BUILD_CONSTRAINTS="
 pkg/util/buildutil/crdb_test_off.go://go:build !crdb_test || crdb_test_off
 pkg/util/buildutil/crdb_test_on.go://go:build crdb_test && !crdb_test_off
@@ -96,16 +92,6 @@ $GIT_GREP '//go:generate' 'pkg/**/*.go' | grep -v stringer | grep -v 'add-leakte
     exit 1
 done
 
-$GIT_GREP 'broken_in_bazel' pkg | grep BUILD.bazel: | grep -v pkg/BUILD.bazel | grep -v pkg/cli/BUILD.bazel | grep -v generate-bazel-extra | cut -d: -f1 | while read LINE; do
-    if [[ "$EXISTING_BROKEN_TESTS_IN_BAZEL" == *"$LINE"* ]]; then
-	# Grandfathered.
-	continue
-    fi
-    echo "A new broken test in Bazel was added in $LINE"
-    echo 'Ensure the test runs with Bazel, then remove the broken_in_bazel tag.'
-    exit 1
-done
-
 $GIT_GREP '//go:build' pkg | grep crdb_test | while read LINE; do
     if [[ "$EXISTING_CRDB_TEST_BUILD_CONSTRAINTS" == *"$LINE"* ]]; then
         # Grandfathered.

diff --git a/build/teamcity/cockroach/ci/tests/testrace_impl.sh b/build/teamcity/cockroach/ci/tests/testrace_impl.sh
@@ -25,7 +25,7 @@ do
         # Run affected tests.
         for test in $tests
         do
-            if [[ ! -z $(bazel query "attr(tags, \"broken_in_bazel\", $test)") ]] || [[ ! -z $(bazel query "attr(tags, \"integration\", $test)") ]]
+            if [[ ! -z $(bazel query "attr(tags, \"integration\", $test)") ]]
             then
                 echo "Skipping test $test"
                 continue

diff --git a/build/teamcity/cockroach/ci/tests/unit_tests_ccl_impl.sh b/build/teamcity/cockroach/ci/tests/unit_tests_ccl_impl.sh
@@ -10,7 +10,7 @@ bazel build //pkg/cmd/bazci --config=ci
 
 EXTRA_PARAMS=""
 
-if tc_release_branch || tc_bors_branch; then
+if tc_bors_branch; then
   # enable up to 1 retry (2 attempts, worst-case) per test executable to report flakes but only on release branches and staging.
   EXTRA_PARAMS=" --flaky_test_attempts=2"
 fi

diff --git a/build/teamcity/cockroach/ci/tests/unit_tests_impl.sh b/build/teamcity/cockroach/ci/tests/unit_tests_impl.sh
@@ -10,7 +10,7 @@ bazel build //pkg/cmd/bazci --config=ci
 
 EXTRA_PARAMS=""
 
-if tc_release_branch || tc_bors_branch; then
+if tc_bors_branch; then
   # enable up to 1 retry (2 attempts, worst-case) per test executable to report flakes but only on release branches and staging.
   EXTRA_PARAMS=" --flaky_test_attempts=2"
 fi

diff --git a/build/teamcity/cockroach/nightlies/stress_impl.sh b/build/teamcity/cockroach/nightlies/stress_impl.sh
@@ -15,11 +15,6 @@ bazel build //pkg/cmd/bazci --config=ci
 BAZEL_BIN=$(bazel info bazel-bin --config=ci)
 ARTIFACTS_DIR=/artifacts
 
-if [[ ! -z $(bazel query "attr(tags, \"broken_in_bazel\", $TARGET)") ]]
-then
-    echo "Skipping test $TARGET as it is broken in bazel"
-    exit 0
-fi
 if [[ ! -z $(bazel query "attr(tags, \"integration\", $TARGET)") ]]
 then
     echo "Skipping test $TARGET as it is an integration test"

diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel
@@ -133,7 +133,6 @@ ALL_TESTS = [
     "//pkg/cmd/label-merged-pr:label-merged-pr_test",
     "//pkg/cmd/mirror/go:go_test",
     "//pkg/cmd/mirror/go:mirror_test",
-    "//pkg/cmd/prereqs:prereqs_test",
     "//pkg/cmd/publish-artifacts:publish-artifacts_test",
     "//pkg/cmd/publish-provisional-artifacts:publish-provisional-artifacts_test",
     "//pkg/cmd/reduce/reduce/reducesql:reducesql_test",
@@ -1054,7 +1053,6 @@ GO_TARGETS = [
     "//pkg/cmd/mirror/npm:ui_lib",
     "//pkg/cmd/prereqs:prereqs",
     "//pkg/cmd/prereqs:prereqs_lib",
-    "//pkg/cmd/prereqs:prereqs_test",
     "//pkg/cmd/protoc-gen-gogoroach:protoc-gen-gogoroach",
     "//pkg/cmd/protoc-gen-gogoroach:protoc-gen-gogoroach_lib",
     "//pkg/cmd/publish-artifacts:publish-artifacts",
@@ -2457,16 +2455,15 @@ GO_TARGETS = [
 ]
 
 # These suites run only the tests with the appropriate "size" (excepting those
-# tagged "broken_in_bazel", "flaky", or "integration") [1]. Note that tests have
-# a default timeout depending on the size [2].
+# tagged "flaky" or "integration") [1]. Note that tests have a default timeout
+# depending on the size [2].
 
 # [1] https://docs.bazel.build/versions/master/be/general.html#test_suite
 # [2] https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-tests
 
 test_suite(
     name = "all_tests",
     tags = [
-        "-broken_in_bazel",
         "-integration",
     ],
     tests = ALL_TESTS,
@@ -2475,7 +2472,6 @@ test_suite(
 test_suite(
     name = "ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-integration",
         "ccl_test",
     ],
@@ -2485,7 +2481,6 @@ test_suite(
 test_suite(
     name = "small_non_ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-ccl_test",
         "-flaky",
         "-integration",
@@ -2497,7 +2492,6 @@ test_suite(
 test_suite(
     name = "medium_non_ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-ccl_test",
         "-flaky",
         "-integration",
@@ -2509,7 +2503,6 @@ test_suite(
 test_suite(
     name = "large_non_ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-ccl_test",
         "-flaky",
         "-integration",
@@ -2521,7 +2514,6 @@ test_suite(
 test_suite(
     name = "enormous_non_ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-ccl_test",
         "-flaky",
         "-integration",

diff --git a/pkg/cmd/generate-bazel-extra/main.go b/pkg/cmd/generate-bazel-extra/main.go
@@ -196,8 +196,8 @@ GO_TARGETS = [`)
 	fmt.Fprintln(w, `]
 
 # These suites run only the tests with the appropriate "size" (excepting those
-# tagged "broken_in_bazel", "flaky", or "integration") [1]. Note that tests have
-# a default timeout depending on the size [2].
+# tagged "flaky" or "integration") [1]. Note that tests have a default timeout
+# depending on the size [2].
 
 # [1] https://docs.bazel.build/versions/master/be/general.html#test_suite
 # [2] https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-tests`)
@@ -206,7 +206,6 @@ GO_TARGETS = [`)
 test_suite(
     name = "all_tests",
     tags = [
-        "-broken_in_bazel",
         "-integration",
     ],
     tests = ALL_TESTS,
@@ -216,7 +215,6 @@ test_suite(
 test_suite(
     name = "ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-integration",
         "ccl_test",
     ],
@@ -228,7 +226,6 @@ test_suite(
 test_suite(
     name = "%[1]s_non_ccl_tests",
     tags = [
-        "-broken_in_bazel",
         "-ccl_test",
         "-flaky",
         "-integration",

diff --git a/pkg/cmd/prereqs/BUILD.bazel b/pkg/cmd/prereqs/BUILD.bazel
@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")
+load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
 
 go_library(
     name = "prereqs_lib",
@@ -13,14 +13,3 @@ go_binary(
     embed = [":prereqs_lib"],
     visibility = ["//visibility:public"],
 )
-
-go_test(
-    name = "prereqs_test",
-    size = "small",
-    srcs = ["prereqs_test.go"],
-    args = ["-test.timeout=55s"],
-    data = glob(["testdata/**"]),
-    embed = [":prereqs_lib"],
-    tags = ["broken_in_bazel"],
-    deps = ["//pkg/testutils"],
-)
diff --git a/pkg/cmd/prereqs/prereqs_test.go b/pkg/cmd/prereqs/prereqs_test.go
diff --git a/pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go b/pkg/kv/kvserver/kvflowcontrol/kvflowhandle/kvflowhandle.go
@@ -97,6 +97,7 @@ func (h *Handle) Admit(ctx context.Context, pri admissionpb.WorkPriority, ct tim
 
 	h.mu.Lock()
 	if h.mu.closed {
+		h.mu.Unlock()
 		log.Errorf(ctx, "operating on a closed handle")
 		return nil
 	}