From 35fb0d3355f05d50425fafa8ac955e11d3126ed5 Mon Sep 17 00:00:00 2001 From: rharding6373 Date: Mon, 10 Jul 2023 13:58:40 -0700 Subject: [PATCH] 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 | 3 + pkg/cmd/roachtest/tests/BUILD.bazel | 3 + .../roachtest/tests/query_comparison_util.go | 19 ++++ pkg/cmd/roachtest/tests/tlp.go | 16 ---- pkg/sql/logictest/logic.go | 92 +------------------ pkg/sql/logictest/main_test.go | 39 -------- pkg/testutils/floatcmp/BUILD.bazel | 2 + pkg/testutils/floatcmp/floatcmp.go | 91 +++++++++++++++++- pkg/testutils/floatcmp/floatcmp_test.go | 40 ++++++++ 9 files changed, 159 insertions(+), 146 deletions(-) diff --git a/pkg/cmd/roachtest/BUILD.bazel b/pkg/cmd/roachtest/BUILD.bazel index 67542188b456..721978b7dbef 100644 --- a/pkg/cmd/roachtest/BUILD.bazel +++ b/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,6 +62,7 @@ go_library( go_binary( name = "roachtest", + testonly = 1, embed = [":roachtest_lib"], visibility = ["//visibility:public"], ) @@ -68,6 +70,7 @@ go_binary( go_test( name = "roachtest_test", size = "small", + testonly = 1, srcs = [ "cluster_test.go", "github_test.go", diff --git a/pkg/cmd/roachtest/tests/BUILD.bazel b/pkg/cmd/roachtest/tests/BUILD.bazel index 2e32e258ff8a..d25c811df89a 100644 --- a/pkg/cmd/roachtest/tests/BUILD.bazel +++ b/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", diff --git a/pkg/cmd/roachtest/tests/query_comparison_util.go b/pkg/cmd/roachtest/tests/query_comparison_util.go index 44be42337840..949b18f6144e 100644 --- a/pkg/cmd/roachtest/tests/query_comparison_util.go +++ b/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) +} diff --git a/pkg/cmd/roachtest/tests/tlp.go b/pkg/cmd/roachtest/tests/tlp.go index 713829f91360..11ff01bf500a 100644 --- a/pkg/cmd/roachtest/tests/tlp.go +++ b/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() { diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index d73352cf991d..df0a4574810e 100644 --- a/pkg/sql/logictest/logic.go +++ b/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) diff --git a/pkg/sql/logictest/main_test.go b/pkg/sql/logictest/main_test.go index 08b4893b5f76..ab3fbecaa74d 100644 --- a/pkg/sql/logictest/main_test.go +++ b/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) - } - } -} diff --git a/pkg/testutils/floatcmp/BUILD.bazel b/pkg/testutils/floatcmp/BUILD.bazel index 7b54e125c2da..62d098e08804 100644 --- a/pkg/testutils/floatcmp/BUILD.bazel +++ b/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"], ) diff --git a/pkg/testutils/floatcmp/floatcmp.go b/pkg/testutils/floatcmp/floatcmp.go index 9bfe97199304..8c7f0d9f298e 100644 --- a/pkg/testutils/floatcmp/floatcmp.go +++ b/pkg/testutils/floatcmp/floatcmp.go @@ -14,10 +14,12 @@ package floatcmp import ( "fmt" + "math" "regexp" "strconv" "strings" + "github.com/cockroachdb/errors" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" ) @@ -40,7 +42,7 @@ const ( // // CloseMargin is greater than 0 otherwise if either expected or actual were // 0 the calculated tolerance from the fraction would be 0. - CloseMargin float64 = CloseFraction * CloseFraction + CloseMargin = CloseFraction * CloseFraction ) // EqualApprox reports whether expected and actual are deeply equal with the @@ -81,6 +83,79 @@ func EqualApprox(expected interface{}, actual interface{}, fraction float64, mar return cmp.Equal(expected, actual, cmpopts.EquateApprox(fraction, margin), cmpopts.EquateNaNs()) } +// 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 EqualApprox(expected, actual, CloseFraction, 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 +} + // RoundFloatsInString rounds floats in a given string to the given number of significant figures. func RoundFloatsInString(s string, significantFigures int) string { return string(regexp.MustCompile(`(\d+\.\d+)`).ReplaceAllFunc([]byte(s), func(x []byte) []byte { @@ -103,3 +178,17 @@ func ParseRoundInStringsDirective(directive string) (int, error) { } return strconv.Atoi(kv[1]) } + +// 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 +} diff --git a/pkg/testutils/floatcmp/floatcmp_test.go b/pkg/testutils/floatcmp/floatcmp_test.go index a7f8d1490e56..a1a23879e643 100644 --- a/pkg/testutils/floatcmp/floatcmp_test.go +++ b/pkg/testutils/floatcmp/floatcmp_test.go @@ -13,6 +13,8 @@ package floatcmp import ( "math" "testing" + + "github.com/cockroachdb/cockroach/pkg/util/leaktest" ) // EqualApprox takes an interface, allowing it to compare equality of both @@ -156,3 +158,41 @@ func TestEqualClose(t *testing.T) { }) } } + +// 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) + } + } +}