From 0e05711d8ed33677109efa40ca23824bca57f446 Mon Sep 17 00:00:00 2001 From: Stephen Rees-Carter Date: Mon, 8 Jun 2026 11:14:25 +1000 Subject: [PATCH] Make tests less fragile --- tests/Assertions.php | 31 ++++++++++++++ tests/EngineTest.php | 17 ++++++-- tests/NumberTest.php | 11 +++-- tests/PickTest.php | 28 +++++++++--- tests/ShuffleTest.php | 99 +++++++++++++++++++++---------------------- 5 files changed, 120 insertions(+), 66 deletions(-) diff --git a/tests/Assertions.php b/tests/Assertions.php index 04d7f87..e6d2811 100644 --- a/tests/Assertions.php +++ b/tests/Assertions.php @@ -12,4 +12,35 @@ protected function assertRegExpCustom($expression, $string, $message = '') $this->assertRegExp($expression, $string, $message); } + + /** + * Assert that a generator is actually random by confirming it doesn't keep + * returning the same value. + * + * Comparing just two random draws is fragile: for a small value space (e.g. + * picking one of 26 letters) two draws legitimately collide often enough to + * fail CI. Sampling many draws and only failing when *every* one matches the + * reference drives that false-positive chance down to effectively zero while + * still catching a generator that returns a constant. + * + * The $reference can be a known static value (e.g. another generator's seeded + * output) or simply a sample produced by calling the generator once. + * + * @param mixed $reference Value the generator should differ from at least once. + * @param callable $generator Returns a comparable value (scalar or array) each call. + */ + protected function assertVaries($reference, callable $generator, $message = '', $attempts = 50) + { + for ($i = 0; $i < $attempts; $i++) { + if ($generator() !== $reference) { + $this->assertTrue(true); + + return; + } + } + + $this->fail($message !== '' + ? $message + : "Generator returned the same value on every one of {$attempts} attempts; expected varied results."); + } } diff --git a/tests/EngineTest.php b/tests/EngineTest.php index 648c3a0..0b93e6b 100644 --- a/tests/EngineTest.php +++ b/tests/EngineTest.php @@ -8,17 +8,28 @@ class EngineTest extends TestCase { + use Assertions; + public function testSeededEngineIsntGlobal() { $generator = Random::use(new Mt19937(3791)); - $this->assertNotEquals(14065, Random::number(1, 100000), 'The random generator shouldn\'t pick the seeded value (but it may occasionally).'); + // The global generator is unseeded, so it should not be locked to the + // seeded values. Sampling several draws keeps this from flaking on the + // rare occasion a single global draw happens to match the seeded value. + $this->assertVaries(14065, function () { + return Random::number(1, 100000); + }, 'The global generator should not always pick the seeded value.'); $this->assertEquals(14065, $generator->number(1, 100000)); - $this->assertNotEquals(847994, Random::otp(), 'The random generator shouldn\'t pick the seeded value (but it may occasionally).'); + $this->assertVaries('847994', function () { + return Random::otp(); + }, 'The global generator should not always pick the seeded value.'); $this->assertEquals(847994, $generator->otp()); - $this->assertNotEquals('hw8kXvG060UyLKq8oKyVyXsmPC5ED9pa', Random::token(), 'The random generator shouldn\'t pick the seeded value (but it may occasionally).'); + $this->assertVaries('hw8kXvG060UyLKq8oKyVyXsmPC5ED9pa', function () { + return Random::token(); + }, 'The global generator should not always pick the seeded value.'); $this->assertEquals('hw8kXvG060UyLKq8oKyVyXsmPC5ED9pa', $generator->token()); } diff --git a/tests/NumberTest.php b/tests/NumberTest.php index 7f45f41..46f56fe 100644 --- a/tests/NumberTest.php +++ b/tests/NumberTest.php @@ -7,6 +7,8 @@ class NumberTest extends TestCase { + use Assertions; + public function testNumbersWithinLimits() { for ($i = 0; $i < 10; $i++) { @@ -20,11 +22,8 @@ public function testNumbersWithinLimits() public function testDifferentNumbers() { - for ($i = 0; $i < 10; $i++) { - $this->assertNotEquals( - Random::number(1, 100000), - Random::number(1, 100000) - ); - } + $this->assertVaries(Random::number(1, 100000), function () { + return Random::number(1, 100000); + }, 'Random::number() returned the same value on every attempt.'); } } diff --git a/tests/PickTest.php b/tests/PickTest.php index 450c7ec..9798f70 100644 --- a/tests/PickTest.php +++ b/tests/PickTest.php @@ -69,7 +69,9 @@ public function testPickSingleFromArray() $this->assertIsArray($picked); $this->assertCount(1, $picked); $this->assertContains($picked[0], $array); - $this->assertNotSame(Random::pick($array, 1), $picked, 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked, function () use ($array) { + return Random::pick($array, 1); + }, 'Picks should vary across draws.'); } public function testPickMultipleFromArray() @@ -79,7 +81,9 @@ public function testPickMultipleFromArray() $this->assertIsArray($picked); $this->assertCount(3, $picked); - $this->assertNotSame(Random::pick($array, 3), $picked, 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked, function () use ($array) { + return Random::pick($array, 3); + }, 'Picks should vary across draws.'); } public function testPickSingleFromCollection() @@ -89,7 +93,9 @@ public function testPickSingleFromCollection() $this->assertInstanceOf(Collection::class, $picked); $this->assertCount(1, $picked); - $this->assertNotSame(Random::pick($collection, 1)->toArray(), $picked->toArray(), 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked->toArray(), function () use ($collection) { + return Random::pick($collection, 1)->toArray(); + }, 'Picks should vary across draws.'); } public function testPickMultipleFromCollection() @@ -99,7 +105,9 @@ public function testPickMultipleFromCollection() $this->assertInstanceOf(Collection::class, $picked); $this->assertCount(3, $picked); - $this->assertNotSame(Random::pick($collection, 3)->toArray(), $picked->toArray(), 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked->toArray(), function () use ($collection) { + return Random::pick($collection, 3)->toArray(); + }, 'Picks should vary across draws.'); } public function testPickOneFromArray() @@ -109,7 +117,9 @@ public function testPickOneFromArray() $this->assertIsString($picked); $this->assertContains($picked, $array); - $this->assertNotSame(Random::pickOne($array), $picked, 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked, function () use ($array) { + return Random::pickOne($array); + }, 'Picks should vary across draws.'); } public function testPickOneFromString() @@ -120,7 +130,9 @@ public function testPickOneFromString() $this->assertIsString($picked); $this->assertEquals(1, strlen($picked)); $this->assertStringContainsString($picked, $string); - $this->assertNotSame(Random::pickOne($string), $picked, 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked, function () use ($string) { + return Random::pickOne($string); + }, 'Picks should vary across draws.'); } public function testPickOneFromCollection() @@ -130,6 +142,8 @@ public function testPickOneFromCollection() $this->assertIsString($picked); $this->assertTrue($collection->contains($picked)); - $this->assertNotSame(Random::pickOne($collection), $picked, 'Picks should be different. (Low chance of false positives.)'); + $this->assertVaries($picked, function () use ($collection) { + return Random::pickOne($collection); + }, 'Picks should vary across draws.'); } } diff --git a/tests/ShuffleTest.php b/tests/ShuffleTest.php index 0513968..72abd2d 100644 --- a/tests/ShuffleTest.php +++ b/tests/ShuffleTest.php @@ -8,76 +8,75 @@ class ShuffleTest extends TestCase { + use Assertions; + public function testShuffleString() { - for ($i = 0; $i < 10; $i++) { - $string = 'original'; - $shuffled = Random::shuffle($string); - - $this->assertIsString($shuffled); - $this->assertEquals(strlen($string), strlen($shuffled)); - $this->assertNotSame($string, $shuffled); - $this->assertNotSame(Random::shuffle($string), $shuffled); - } + $string = 'original'; + $shuffled = Random::shuffle($string); + + $this->assertIsString($shuffled); + $this->assertEquals(strlen($string), strlen($shuffled)); + + $this->assertVaries($string, function () use ($string) { + return Random::shuffle($string); + }, 'Shuffling should not always reproduce the original order.'); } public function testShuffleArrayWithoutPreservingKeys() { - for ($i = 0; $i < 10; $i++) { - $array = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; - $shuffled = Random::shuffle($array, $preserveKeys = false); - - $this->assertIsArray($shuffled); - $this->assertEquals(count($array), count($shuffled)); - $this->assertNotSame($array, $shuffled); - $this->assertNotSame(Random::shuffle($array), $shuffled); - $this->assertSame(range(0, 8), array_keys($shuffled), 'Keys were not re-indexed.'); - } + $array = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + $shuffled = Random::shuffle($array, $preserveKeys = false); + + $this->assertIsArray($shuffled); + $this->assertEquals(count($array), count($shuffled)); + $this->assertSame(range(0, 8), array_keys($shuffled), 'Keys were not re-indexed.'); + + $this->assertVaries($array, function () use ($array) { + return Random::shuffle($array, false); + }, 'Shuffling should not always reproduce the original order.'); } public function testShuffleArrayPreservingKeys() { - for ($i = 0; $i < 10; $i++) { - $array = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; - $shuffled = Random::shuffle($array, $preserveKeys = true); - - $this->assertNotSame($array, $shuffled); - $this->assertNotSame(Random::shuffle($array), $shuffled); - $this->assertNotSame(range(0, 8), array_keys($shuffled), 'Keys were re-indexed.'); + $array = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']; + $shuffled = Random::shuffle($array, $preserveKeys = true); - for ($j = 0; $j < 9; $j++) { - $this->assertSame($array[$j], $shuffled[$j], "Key {$j} was not preserved."); - } + for ($j = 0; $j < 9; $j++) { + $this->assertSame($array[$j], $shuffled[$j], "Key {$j} was not preserved."); } + + $this->assertVaries(range(0, 8), function () use ($array) { + return array_keys(Random::shuffle($array, true)); + }, 'Shuffling should vary the order while preserving keys.'); } public function testShuffleCollectionWithoutPreservingKey() { - for ($i = 0; $i < 10; $i++) { - $collection = new Collection(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']); - $shuffled = Random::shuffle($collection, $preserveKeys = false); - - $this->assertInstanceOf(Collection::class, $shuffled); - $this->assertNotSame($collection->toArray(), $shuffled->toArray()); - $this->assertNotSame(Random::shuffle($collection)->toArray(), $shuffled->toArray()); - $this->assertSame(range(0, 8), $shuffled->keys()->toArray(), 'Keys were not re-indexed.'); - } + $collection = new Collection(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']); + $shuffled = Random::shuffle($collection, $preserveKeys = false); + + $this->assertInstanceOf(Collection::class, $shuffled); + $this->assertSame(range(0, 8), $shuffled->keys()->toArray(), 'Keys were not re-indexed.'); + + $this->assertVaries($collection->toArray(), function () use ($collection) { + return Random::shuffle($collection, false)->toArray(); + }, 'Shuffling should not always reproduce the original order.'); } public function testShuffleCollectionPreservingKey() { - for ($i = 0; $i < 10; $i++) { - $collection = new Collection(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']); - $shuffled = Random::shuffle($collection, $preserveKeys = true); - - $this->assertInstanceOf(Collection::class, $shuffled); - $this->assertNotSame($collection->toArray(), $shuffled->toArray()); - $this->assertNotSame(Random::shuffle($collection)->toArray(), $shuffled->toArray()); - $this->assertNotSame(range(0, 8), $shuffled->keys(), 'Keys were re-indexed.'); - - for ($j = 0; $j < 9; $j++) { - $this->assertSame($collection[$j], $shuffled[$j], "Key {$j} was not preserved."); - } + $collection = new Collection(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i']); + $shuffled = Random::shuffle($collection, $preserveKeys = true); + + $this->assertInstanceOf(Collection::class, $shuffled); + + for ($j = 0; $j < 9; $j++) { + $this->assertSame($collection[$j], $shuffled[$j], "Key {$j} was not preserved."); } + + $this->assertVaries(range(0, 8), function () use ($collection) { + return Random::shuffle($collection, true)->keys()->toArray(); + }, 'Shuffling should vary the order while preserving keys.'); } }