Skip to content

Changes to better support random seeds for flaky tests. #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 1, 2025
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions s2/cell_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package s2

import (
"math"
"math/rand"
"testing"
"unsafe"

Expand Down Expand Up @@ -559,9 +558,8 @@ func TestCellContainsPoint(t *testing.T) {
}

func TestCellContainsPointConsistentWithS2CellIDFromPoint(t *testing.T) {
// About 1% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(1)
// TODO: Is it still about 1% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

// Construct many points that are nearly on a Cell edge, and verify that
// CellFromCellID(cellIDFromPoint(p)).Contains(p) is always true.
Expand All @@ -571,7 +569,7 @@ func TestCellContainsPointConsistentWithS2CellIDFromPoint(t *testing.T) {
i2 := (i1 + 1) & 3
v1 := cell.Vertex(i1)
v2 := samplePointFromCap(CapFromCenterAngle(cell.Vertex(i2), s1.Angle(epsilon)))
p := Interpolate(randomFloat64(), v1, v2)
p := Interpolate(randomUniformFloat64(0, 1.0), v1, v2)
if !CellFromCellID(cellIDFromPoint(p)).ContainsPoint(p) {
t.Errorf("For p=%v, CellFromCellID(cellIDFromPoint(p)).ContainsPoint(p) was false", p)
}
Expand Down Expand Up @@ -732,9 +730,8 @@ func maxDistanceToEdgeBruteForce(cell Cell, a, b Point) s1.ChordAngle {
}

func TestCellDistanceToEdge(t *testing.T) {
// About 0.1% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(2)
// TODO: Is it still about 0.1% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

for iter := 0; iter < 1000; iter++ {
cell := CellFromCellID(randomCellID())
Expand Down Expand Up @@ -814,6 +811,6 @@ func TestCellMaxDistanceToCell(t *testing.T) {
}
}

// TODO(roberts): Differences from C++.
// TODO(rsned): Differences from C++.
// CellVsLoopRectBound
// RectBoundIsLargeEnough
10 changes: 6 additions & 4 deletions s2/cellunion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package s2

import (
"math"
"math/rand"
"reflect"
"testing"

Expand Down Expand Up @@ -370,10 +369,13 @@ func addCells(id CellID, selected bool, input *[]CellID, expected *[]CellID, t *
}
}

// TODO(rsned): This test case doesn't match any specific test or set of tests in
// C++. It's also somewhat flaky. It might make sense to remove or refactor this
// to stay more inline with what's current in C++.

func TestCellUnionNormalizePseudoRandom(t *testing.T) {
// About 2.4% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(2)
// TODO: Verify if it's still about 2.4% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

// Try a bunch of random test cases, and keep track of average statistics
// for normalization (to see if they agree with the analysis above).
Expand Down
8 changes: 3 additions & 5 deletions s2/convex_hull_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package s2

import (
"math"
"math/rand"
"testing"

"github.com/golang/geo/s1"
Expand Down Expand Up @@ -194,9 +193,8 @@ func TestConvexHullQueryLoopsAroundNorthPole(t *testing.T) {
}

func TestConvexHullQueryPointsInsideHull(t *testing.T) {
// About 0.3% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(1)
// TODO: Verify if it's still about 0.3% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

// Repeatedly build the convex hull of a set of points, then add more points
// inside that loop and build the convex hull again. The result should
Expand All @@ -223,7 +221,7 @@ func TestConvexHullQueryPointsInsideHull(t *testing.T) {
// test pass reliably it means that we need to reject convex hulls whose
// bounding cap (when computed from a bounding rectangle) is not convex.
//
// TODO(roberts): This test can still fail (about 1 iteration in 500,000)
// TODO(rsned): This test can still fail (about 1 iteration in 500,000)
// because the Rect.CapBound implementation does not guarantee
// that A.Contains(B) implies A.CapBound().Contains(B.CapBound()).
if hull.CapBound().Height() >= 1 {
Expand Down
6 changes: 2 additions & 4 deletions s2/edge_clipping_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package s2
import (
"fmt"
"math"
"math/rand"
"testing"

"github.com/golang/geo/r1"
Expand Down Expand Up @@ -248,9 +247,8 @@ func testClipToPaddedFace(t *testing.T, a, b Point) {
}

func TestEdgeClippingClipToPaddedFace(t *testing.T) {
// About 1.2% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(1)
// TODO: Verify if it's still about 1.2% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

// Start with a few simple cases.
// An edge that is entirely contained within one cube face:
Expand Down
11 changes: 8 additions & 3 deletions s2/edge_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,13 +242,17 @@ func testEdgeQueryWithGenerator(t *testing.T,
var indexCaps []Cap
var indexes []*ShapeIndex
for i := 0; i < numIndexes; i++ {
// TODO(rsned): Replace with:
// r := rand.New(rand.NewSource(i))
rand.Seed(int64(i))
indexCaps = append(indexCaps, CapFromCenterAngle(randomPoint(), testCapRadius))
indexes = append(indexes, NewShapeIndex())
gen(indexCaps[i], numEdges, indexes[i])
}

for i := 0; i < numQueries; i++ {
// TODO(rsned): Replace with:
// r := rand.New(rand.NewSource(i))
rand.Seed(int64(i))
iIndex := randomUniformInt(numIndexes)
indexCap := indexCaps[iIndex]
Expand Down Expand Up @@ -332,7 +336,7 @@ func testEdgeQueryWithGenerator(t *testing.T,
// - If maxErrorFraction > 0, then MaxError is set to the given
// fraction of the index radius.
//
// TODO(roberts): If there is a need to benchmark Furthest as well, this will need
// TODO(rsned): If there is a need to benchmark Furthest as well, this will need
// some changes to not use just the Closest variants of parts.
// Furthest isn't doing anything different under the covers than Closest, so there
// isn't really a huge need for benchmarking both.
Expand Down Expand Up @@ -360,12 +364,12 @@ func benchmarkEdgeQueryFindClosest(b *testing.B, bmOpts *edgeQueryBenchmarkOptio
bmOpts.numIndexEdges *= 4
b.Run(fmt.Sprintf("%d", bmOpts.numIndexEdges),
func(b *testing.B) {
// TODO(roberts): Return value 2 here is the slice of target
// TODO(rsned): Return value 2 here is the slice of target
// ShapeIndexes. Incorporate it once ShapeIndexTargets
// are able to be used in tests.
targets, _ = generateEdgeQueryWithTargets(bmOpts, query, index)
for i := 0; i < b.N; i++ {
// TODO(roberts): In the reference C++ benchmark
// TODO(rsned): In the reference C++ benchmark
// they use the tooling to split the benchmark
// run iterations up into kNumIndexSamples (8)
// times and pause to generate a new geometry
Expand Down Expand Up @@ -433,6 +437,7 @@ func generateEdgeQueryWithTargets(opts *edgeQueryBenchmarkOptions, query *EdgeQu
const maxTargetsPerIndex = 100

// Set a specific seed to allow repeatability
// Replace with r := rand.New(rand.NewSource(opts.randomSeed)) and pass through.
rand.Seed(opts.randomSeed)
opts.randomSeed++
indexCap := CapFromCenterAngle(randomPoint(), opts.radiusKm)
Expand Down
8 changes: 3 additions & 5 deletions s2/paddedcell_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package s2

import (
"math"
"math/rand"
"testing"

"github.com/golang/geo/r1"
Expand Down Expand Up @@ -133,9 +132,8 @@ func TestPaddedCellEntryExitVertices(t *testing.T) {
}

func TestPaddedCellShrinkToFit(t *testing.T) {
// About 0.2% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(1)
// TODO: Verify if it's still about 0.2% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

for iter := 0; iter < 1000; iter++ {
// Start with the desired result and work backwards.
Expand All @@ -146,7 +144,7 @@ func TestPaddedCellShrinkToFit(t *testing.T) {
// Find the biggest rectangle that fits in "result" after padding.
// (These calculations ignore numerical errors.)
maxPadding := 0.5 * math.Min(sizeUV.X, sizeUV.Y)
padding := maxPadding * randomFloat64()
padding := randomUniformFloat64(0, maxPadding)
maxRect := resultUV.ExpandedByMargin(-padding)

// Start with a random subset of the maximum rectangle.
Expand Down
8 changes: 3 additions & 5 deletions s2/point_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package s2

import (
"math"
"math/rand"
"testing"

"github.com/golang/geo/r3"
Expand Down Expand Up @@ -95,11 +94,10 @@ func TestPointDistance(t *testing.T) {
}

func TestChordAngleBetweenPoints(t *testing.T) {
// About 0.2% flaky with a random seed.
// TODO: https://github.com/golang/geo/issues/120
rand.Seed(1)
// TODO: Verify if it's still about 0.2% flaky with a random seed.
// TODO(rsned): https://github.com/golang/geo/issues/120

for iter := 0; iter < 10; iter++ {
for iter := 0; iter < 100; iter++ {
m := randomFrame()
x := m.col(0)
y := m.col(1)
Expand Down
11 changes: 5 additions & 6 deletions s2/point_vector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package s2

import (
"math/rand"
"testing"
)

Expand Down Expand Up @@ -43,9 +42,6 @@ func TestPointVectorEmpty(t *testing.T) {
}

func TestPointVectorBasics(t *testing.T) {
const seed = 8675309
rand.Seed(seed)

const numPoints = 100
var p PointVector = make([]Point, numPoints)

Expand All @@ -70,7 +66,6 @@ func TestPointVectorBasics(t *testing.T) {
t.Errorf("shape.IsFull() = true, want false")
}

rand.Seed(seed)
for i := 0; i < numPoints; i++ {
if got, want := shape.Chain(i).Start, i; got != want {
t.Errorf("shape.Chain(%d).Start = %d, want %d", i, got, want)
Expand All @@ -79,7 +74,7 @@ func TestPointVectorBasics(t *testing.T) {
t.Errorf("shape.Chain(%d).Length = %v, want %d", i, got, want)
}
edge := shape.Edge(i)
pt := randomPoint()
pt := p[i]

if !pt.ApproxEqual(edge.V0) {
t.Errorf("shape.Edge(%d).V0 = %v, want %v", i, edge.V0, pt)
Expand All @@ -89,3 +84,7 @@ func TestPointVectorBasics(t *testing.T) {
}
}
}

// TODO(rsned): Differences from C++
// TEST(S2PointVectorShape, ChainIteratorWorks) {
// TEST(S2PointVectorShape, ChainVertexIteratorWorks) {
Loading