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

rsned · 2025-03-26T03:53:12Z

Add flag (--s2_random_seed) to specify the seed used for random generation in tests.
Add new testing flag to specify the s2 random seed to allow repeatability between test runs.
Refactor random test methods to allow optional seeded generators.
Fix up all the functions in s2_test.go to take an optional random generator.
Change the places that actually generate the random values to test for the optional parameter and use it if set.
Minor fixes to a couple tests to match the current logic and values in their corresponding C++ tests. (paddedcell_test to use uniform float vs random float, fractal test cases relative error size)
Update comments on random seeds to reference current rand form.
With the addition of the random seed flag in testing now, update the comments on the cases that were triggering flaky tests. If they still show flakes, the specific cases can now be pushed through to the underlying random calls.

Fixes #120

Add new testing flag to specify the s2 random seed to allow repeatability between test runs.

Fix up all the functions in s2_test.go to take an optional random generator. Change the places that actually generate the random values to test for the optional parameter and use it if set.

With the addition of the random seed flag in testing now, update the comments on the cases that were triggering flaky tests. If they still show flakes, the specific cases can now be pushed through to the underlying random calls.

s2/s2_test_test.go

s2/cellunion_test.go

jmr · 2025-03-26T07:59:00Z

s2/cell_test.go

@@ -560,8 +559,10 @@ func TestCellContainsPoint(t *testing.T) {

 func TestCellContainsPointConsistentWithS2CellIDFromPoint(t *testing.T) {
 	// About 1% flaky with a random seed.


Is this still true?

Not from my limited random seed testing. If you want to try patching it and running a few thousand rounds as well and see if you can get it to trigger like before or not.

s2/cell_test.go

s2/edge_query_test.go

s2/point_vector_test.go

jmr · 2025-03-26T08:04:15Z

s2/s2_test.go

+
+	// To set in go testing add "--s2_random_seed=123" to your test command.
+	// When using blaze/bazel add "--test_arg=--s2_random_seed=123"
+	s2RandomSeed = flag.Int64("s2_random_seed", 1,


Having a fixed seed seems bad, since then we're always testing the same seed. Can we have a default that means "pick a random seed"?

Typically a non-fixed seed should be used, such as time.Now().UnixNano(). The closest option would be to try seeding it with a rand.Int63() from whatever the runtime chooses as a random value and then only if a specific seed is set, use that.

Yes, that sounds reasonable.

jmr · 2025-03-26T08:07:47Z

s2/s2_test.go

+	// set up a random generator for use in tests.
+	// Prior to Go 1.20, the generator was seeded like Seed(1) at program startup.
+	// Using the default seed flag value will produce the prior behavior.
+	random = rand.New(rand.NewSource(*s2RandomSeed))


How is this going to interact with test_filter? If there's a failure, we don't want it to go away when we change the test filter.

It seems we don't want one random source shared with all test cases, it should be re-initialized in each one.

Best we can do is create specific new ones when we think we really need it. Otherwise we would need to go into every single Test... function in every file and add the NewSource(...) line to each method to do this. For most tests that's overkill since they really don't care, they usually just need whatever random value comes next.

If we end up with a growth of flaky tests then we may need to go down that route, but for now I would prefer to just use the one (basically what it was before only explicitly using the shared one rather than relying on the shared one from package rand.)

Otherwise we would need to go into every single Test... function in every file and add the NewSource(...) line to each method to do this. For most tests that's overkill since they really don't care, they usually just need whatever random value comes next.

This is what we did in C++. More flaky tests will almost certainly be found this way. It's a bigger project, though.

So far it seems to have highlighted a couple more while I think I've tracked down what was wrong with a couple of the others.

s2/s2_test.go

Drop repeated issue comments. Replace static seed with a somewhat random seed. Test if user set a seed and if so use that. Fix a few more test bugs. Update TODO comment assignee.

s2/cell_test.go

Add TODO for a couple places where the helper differs from the corresponding C++ helpers. Touch up a couple method comments.

TestCellUnionNormalizePseudoRandom less flaky than before but still somewhat, so set an explicit seed on it for now. Add comments on more detected differences from C++ that will need to be synced up and resolved to maybe fix the flaky differences.

rsned added 7 commits March 25, 2025 11:29

Add flag to specify random seed in tests.

1f51ae5

Add new testing flag to specify the s2 random seed to allow repeatability between test runs.

Refactor random test methods to allow optional seeded generators.

6284544

Fix up all the functions in s2_test.go to take an optional random generator. Change the places that actually generate the random values to test for the optional parameter and use it if set.

Touch up paddedcell_test to use uniform float vs random float.

8505d53

Touch up Fractal test case relative error size to match C++ tests.

81ade3e

Fix scale of padding in shrink to fit test

32144cb

Typo

9644ba4

jmr requested changes Mar 26, 2025

View reviewed changes

rsned added 3 commits March 26, 2025 22:22

Add fixes based on review comments.

824f897

Drop repeated issue comments. Replace static seed with a somewhat random seed. Test if user set a seed and if so use that. Fix a few more test bugs. Update TODO comment assignee.

Fix test to use randomUniformFloat(0, 1.0) instead of randomFloat

33dcaf7

Add TODOs to fix perturbedCornerOrMidpoint

2737ed0

jmr reviewed Mar 27, 2025

View reviewed changes

s2/cell_test.go Show resolved Hide resolved

Add comment and fix outdated test case for S2 latlng rect.

49fea04

jmr approved these changes Mar 31, 2025

View reviewed changes

rsned and others added 4 commits March 31, 2025 11:50

Update comments in s2_test on some methods.

0fcff54

Add TODO for a couple places where the helper differs from the corresponding C++ helpers. Touch up a couple method comments.

Merge branch 'golang:master' into rand

d4f6b47

Merge branch 'golang:master' into rand

138e34d

rsned merged commit 45267f9 into golang:master Apr 1, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rsned commented Mar 26, 2025

jmr Mar 26, 2025

rsned Mar 27, 2025

jmr Mar 26, 2025

rsned Mar 27, 2025

jmr Mar 27, 2025

jmr Mar 26, 2025

rsned Mar 27, 2025

jmr Mar 27, 2025

rsned Mar 27, 2025

		@@ -560,8 +559,10 @@ func TestCellContainsPoint(t *testing.T) {

		func TestCellContainsPointConsistentWithS2CellIDFromPoint(t *testing.T) {
		// About 1% flaky with a random seed.

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

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

Conversation

rsned commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment