tests, logictest, floatcmp: refactor comparison test util functions

This commit moves some float comparison test util functions from
logictest into the floatcmp package. It also moves a query result
comparison function from the tlp file to query_comparison_util in the
tests package.

This commit also marks roachtests as testonly targets.

Epic: none

Release note: None

pkg/cmd/roachtest/BUILD.bazel

@@ -2,6 +2,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")
 
 go_library(
     name = "roachtest_lib",
+    testonly = 1,
     srcs = [
         "cluster.go",
         "github.go",
@@ -61,13 +62,15 @@ go_library(
 
 go_binary(
     name = "roachtest",
+    testonly = 1,
     embed = [":roachtest_lib"],
     visibility = ["//visibility:public"],
 )
 
 go_test(
     name = "roachtest_test",
     size = "small",
+    testonly = 1,
     srcs = [
         "cluster_test.go",
         "github_test.go",

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "tests",
+    testonly = 1,
     srcs = [
         "acceptance.go",
         "activerecord.go",
@@ -224,6 +225,7 @@ go_library(
         "//pkg/storage",
         "//pkg/storage/enginepb",
         "//pkg/testutils",
+        "//pkg/testutils/floatcmp",
         "//pkg/testutils/jobutils",
         "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
@@ -298,6 +300,7 @@ go_test(
         "//pkg/roachprod/logger",
         "//pkg/roachprod/prometheus",
         "//pkg/testutils/skip",
+        "//pkg/util/leaktest",
         "//pkg/util/version",
         "@com_github_golang_mock//gomock",
         "@com_github_google_go_github//github",

pkg/cmd/roachtest/tests/query_comparison_util.go

@@ -17,6 +17,8 @@ import (
 	"math/rand"
 	"os"
 	"path/filepath"
+	"runtime"
+	"sort"
 	"strings"
 	"time"
 
@@ -26,10 +28,12 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/internal/sqlsmith"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
+	"github.com/cockroachdb/cockroach/pkg/testutils/floatcmp"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
+	"github.com/google/go-cmp/cmp"
 )
 
 type queryComparisonTest struct {
@@ -379,3 +383,18 @@ func (h *queryComparisonHelper) logVerboseOutput() {
 func (h *queryComparisonHelper) makeError(err error, msg string) error {
 	return errors.Wrapf(err, "%s. %d statements run", msg, h.stmtNo)
 }
+
+// unsortedMatricesDiff sorts and compares rows of data.
+func unsortedMatricesDiff(rowMatrix1, rowMatrix2 [][]string) string {
+	var rows1 []string
+	for _, row := range rowMatrix1 {
+		rows1 = append(rows1, strings.Join(row[:], ","))
+	}
+	var rows2 []string
+	for _, row := range rowMatrix2 {
+		rows2 = append(rows2, strings.Join(row[:], ","))
+	}
+	sort.Strings(rows1)
+	sort.Strings(rows2)
+	return cmp.Diff(rows1, rows2)
+}

pkg/cmd/roachtest/tests/tlp.go

@@ -16,7 +16,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"sort"
 	"strings"
 	"time"
 
@@ -29,7 +28,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/errors"
-	"github.com/google/go-cmp/cmp"
 )
 
 const statementTimeout = time.Minute
@@ -271,20 +269,6 @@ func runTLPQuery(conn *gosql.DB, smither *sqlsmith.Smither, logStmt func(string)
 	})
 }
 
-func unsortedMatricesDiff(rowMatrix1, rowMatrix2 [][]string) string {
-	var rows1 []string
-	for _, row := range rowMatrix1 {
-		rows1 = append(rows1, strings.Join(row[:], ","))
-	}
-	var rows2 []string
-	for _, row := range rowMatrix2 {
-		rows2 = append(rows2, strings.Join(row[:], ","))
-	}
-	sort.Strings(rows1)
-	sort.Strings(rows2)
-	return cmp.Diff(rows1, rows2)
-}
-
 func runWithTimeout(f func() error) error {
 	done := make(chan error, 1)
 	go func() {

pkg/sql/logictest/logic.go

@@ -18,7 +18,6 @@ import (
 	gosql "database/sql"
 	"flag"
 	"fmt"
-	"math"
 	"math/rand"
 	"net"
 	"net/url"
@@ -3651,9 +3650,9 @@ func (t *logicTest) finishExecQuery(query logicQuery, rows *gosql.Rows, err erro
 				// ('R') coltypes are approximately equal to take into account
 				// platform differences in floating point calculations.
 				if runtime.GOARCH == "s390x" && (colT == 'F' || colT == 'R') {
-					resultMatches, err = floatsMatchApprox(expected, actual)
+					resultMatches, err = floatcmp.FloatsMatchApprox(expected, actual)
 				} else if colT == 'F' {
-					resultMatches, err = floatsMatch(expected, actual)
+					resultMatches, err = floatcmp.FloatsMatch(expected, actual)
 				}
 				if err != nil {
 					return errors.CombineErrors(makeError(), err)
@@ -3721,93 +3720,6 @@ func (t *logicTest) finishExecQuery(query logicQuery, rows *gosql.Rows, err erro
 	return nil
 }
 
-// parseExpectedAndActualFloats converts the strings expectedString and
-// actualString to float64 values.
-func parseExpectedAndActualFloats(expectedString, actualString string) (float64, float64, error) {
-	expected, err := strconv.ParseFloat(expectedString, 64 /* bitSize */)
-	if err != nil {
-		return 0, 0, errors.Wrap(err, "when parsing expected")
-	}
-	actual, err := strconv.ParseFloat(actualString, 64 /* bitSize */)
-	if err != nil {
-		return 0, 0, errors.Wrap(err, "when parsing actual")
-	}
-	return expected, actual, nil
-}
-
-// floatsMatchApprox returns whether two floating point represented as
-// strings are equal within a tolerance.
-func floatsMatchApprox(expectedString, actualString string) (bool, error) {
-	expected, actual, err := parseExpectedAndActualFloats(expectedString, actualString)
-	if err != nil {
-		return false, err
-	}
-	return floatcmp.EqualApprox(expected, actual, floatcmp.CloseFraction, floatcmp.CloseMargin), nil
-}
-
-// floatsMatch returns whether two floating point numbers represented as
-// strings have matching 15 significant decimal digits (this is the precision
-// that Postgres supports for 'double precision' type).
-func floatsMatch(expectedString, actualString string) (bool, error) {
-	expected, actual, err := parseExpectedAndActualFloats(expectedString, actualString)
-	if err != nil {
-		return false, err
-	}
-	// Check special values - NaN, +Inf, -Inf, 0.
-	if math.IsNaN(expected) || math.IsNaN(actual) {
-		return math.IsNaN(expected) == math.IsNaN(actual), nil
-	}
-	if math.IsInf(expected, 0 /* sign */) || math.IsInf(actual, 0 /* sign */) {
-		bothNegativeInf := math.IsInf(expected, -1 /* sign */) == math.IsInf(actual, -1 /* sign */)
-		bothPositiveInf := math.IsInf(expected, 1 /* sign */) == math.IsInf(actual, 1 /* sign */)
-		return bothNegativeInf || bothPositiveInf, nil
-	}
-	if expected == 0 || actual == 0 {
-		return expected == actual, nil
-	}
-	// Check that the numbers have the same sign.
-	if expected*actual < 0 {
-		return false, nil
-	}
-	expected = math.Abs(expected)
-	actual = math.Abs(actual)
-	// Check that 15 significant digits match. We do so by normalizing the
-	// numbers and then checking one digit at a time.
-	//
-	// normalize converts f to base * 10**power representation where base is in
-	// [1.0, 10.0) range.
-	normalize := func(f float64) (base float64, power int) {
-		for f >= 10 {
-			f = f / 10
-			power++
-		}
-		for f < 1 {
-			f *= 10
-			power--
-		}
-		return f, power
-	}
-	var expPower, actPower int
-	expected, expPower = normalize(expected)
-	actual, actPower = normalize(actual)
-	if expPower != actPower {
-		return false, nil
-	}
-	// TODO(yuzefovich): investigate why we can't always guarantee deterministic
-	// 15 significant digits and switch back from 14 to 15 digits comparison
-	// here. See #56446 for more details.
-	for i := 0; i < 14; i++ {
-		expDigit := int(expected)
-		actDigit := int(actual)
-		if expDigit != actDigit {
-			return false, nil
-		}
-		expected -= (expected - float64(expDigit)) * 10
-		actual -= (actual - float64(actDigit)) * 10
-	}
-	return true, nil
-}
-
 func (t *logicTest) formatValues(vals []string, valsPerLine int) []string {
 	var buf bytes.Buffer
 	tw := tabwriter.NewWriter(&buf, 2, 1, 2, ' ', 0)

pkg/sql/logictest/main_test.go

@@ -20,7 +20,6 @@ import (
 	_ "github.com/cockroachdb/cockroach/pkg/sql/tests"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
-	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 )
 
@@ -33,41 +32,3 @@ func TestMain(m *testing.M) {
 	serverutils.InitTestClusterFactory(testcluster.TestClusterFactory)
 	os.Exit(m.Run())
 }
-
-// TestFloatsMatch is a unit test for floatsMatch() and floatsMatchApprox()
-// functions.
-func TestFloatsMatch(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	for _, tc := range []struct {
-		f1, f2 string
-		match  bool
-	}{
-		{f1: "NaN", f2: "+Inf", match: false},
-		{f1: "+Inf", f2: "+Inf", match: true},
-		{f1: "NaN", f2: "NaN", match: true},
-		{f1: "+Inf", f2: "-Inf", match: false},
-		{f1: "-0.0", f2: "0.0", match: true},
-		{f1: "0.0", f2: "NaN", match: false},
-		{f1: "123.45", f2: "12.345", match: false},
-		{f1: "0.1234567890123456", f2: "0.1234567890123455", match: true},
-		{f1: "0.1234567890123456", f2: "0.1234567890123457", match: true},
-		{f1: "-0.1234567890123456", f2: "0.1234567890123456", match: false},
-		{f1: "-0.1234567890123456", f2: "-0.1234567890123455", match: true},
-	} {
-		match, err := floatsMatch(tc.f1, tc.f2)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if match != tc.match {
-			t.Fatalf("floatsMatch: wrong result on %v", tc)
-		}
-
-		match, err = floatsMatchApprox(tc.f1, tc.f2)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if match != tc.match {
-			t.Fatalf("floatsMatchApprox: wrong result on %v", tc)
-		}
-	}
-}

pkg/testutils/floatcmp/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/testutils/floatcmp",
     visibility = ["//visibility:public"],
     deps = [
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_google_go_cmp//cmp",
         "@com_github_google_go_cmp//cmp/cmpopts",
     ],
@@ -18,4 +19,5 @@ go_test(
     srcs = ["floatcmp_test.go"],
     args = ["-test.timeout=55s"],
     embed = [":floatcmp"],
+    deps = ["//pkg/util/leaktest"],
 )