From 5cfb4133bf5eaa8e2003b6ac4b3b0f86e69c7752 Mon Sep 17 00:00:00 2001 From: Norby Baruani Date: Sun, 14 Jun 2026 14:09:30 +0200 Subject: [PATCH] [5.x] Make time-based sharding namespace-agnostic and forward --test-directory (#1677) * refactor(shard): extract parseListTestsOutput for testability * test(shard): characterize parseListTestsOutput current behavior * fix(shard): parse any PHP FQCN namespace from --list-tests * fix(shard): forward --test-directory to list-tests subprocess * chore: lint + snapshot fixups Co-Authored-By: Claude Opus 4.6 (1M context) * refactor(shard): make extracted helpers private, test via reflection Co-Authored-By: Claude Opus 4.6 (1M context) * revert visual_snapshot_of_help_command_output * revert visual_snapshot_of_help_command_output * keep function removeParallelArguments * strip --processes argument when building list-tests command The removeParallelArguments method was not filtering --processes flags, causing the list-tests subprocess to fail when parallel execution was enabled. This prevented time-based sharding from working correctly with the --parallel option. Now both --parallel/-p and --processes arguments are removed from the command used to enumerate tests, ensuring the subprocess runs successfully. * test: re-add namespace-agnostic sharding tests - 5.x merge kept describe()-style test file, dropped PR #1677 tests for parseListTestsOutput + buildListTestsCommand. Re-add them in matching style. - Also fix removeParallelArguments test broken by merge: source array_values() + strips --processes, so expects ['bin/pest','tests/']. --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/Plugins/Shard.php | 44 ++++++-- tests/.snapshots/success.txt | 2 +- tests/Unit/Plugins/Shard.php | 202 ++++++++++++++++++++++++++++++++++- 3 files changed, 237 insertions(+), 11 deletions(-) diff --git a/src/Plugins/Shard.php b/src/Plugins/Shard.php index 988b3a4b..fa5f14c7 100644 --- a/src/Plugins/Shard.php +++ b/src/Plugins/Shard.php @@ -199,15 +199,14 @@ final class Shard implements AddsOutput, HandlesArguments, Terminable */ private function allTests(array $arguments): array { - $output = new Process([ - 'php', - ...$this->removeParallelArguments($arguments), - '--list-tests', - ])->setTimeout(120)->mustRun()->getOutput(); + $command = $this->buildListTestsCommand( + $arguments, + TestSuite::getInstance()->testPath, + ); - preg_match_all('/ - (?:P\\\\)?(Tests\\\\[^:]+)::/', $output, $matches); + $output = (new Process($command))->setTimeout(120)->mustRun()->getOutput(); - return array_values(array_unique($matches[1])); + return $this->parseListTestsOutput($output); } /** @@ -216,7 +215,36 @@ final class Shard implements AddsOutput, HandlesArguments, Terminable */ private function removeParallelArguments(array $arguments): array { - return array_filter($arguments, fn (string $argument): bool => ! in_array($argument, ['--parallel', '-p'], strict: true)); + return array_values(array_filter( + $arguments, + fn (string $argument): bool => ! in_array($argument, ['--parallel', '-p'], strict: true) + && ! str_starts_with($argument, '--processes'), + )); + } + + /** + * Builds the subprocess command used to enumerate tests via `--list-tests`. + * + * @param list $arguments + * @return list + */ + private function buildListTestsCommand(array $arguments, string $testPath): array + { + $filtered = $this->removeParallelArguments($arguments); + + return ['php', ...$filtered, '--test-directory='.$testPath, '--list-tests']; + } + + /** + * Parses `--list-tests` output into a unique list of test class FQCNs. + * + * @return list + */ + private function parseListTestsOutput(string $output): array + { + preg_match_all('/ - (?:P\\\\)?([A-Za-z_]\w*(?:\\\\[A-Za-z_]\w*)*)::/', $output, $matches); + + return array_values(array_unique($matches[1])); } /** diff --git a/tests/.snapshots/success.txt b/tests/.snapshots/success.txt index e7ebe7b8..7d9ce277 100644 --- a/tests/.snapshots/success.txt +++ b/tests/.snapshots/success.txt @@ -1983,4 +1983,4 @@ ✓ pass with dataset with ('my-datas-set-value') ✓ within describe → pass with dataset with ('my-datas-set-value') - Tests: 2 deprecated, 4 warnings, 5 incomplete, 2 notices, 40 todos, 35 skipped, 1370 passed (3068 assertions) \ No newline at end of file + Tests: 2 deprecated, 4 warnings, 5 incomplete, 2 notices, 40 todos, 35 skipped, 1370 passed (3068 assertions) diff --git a/tests/Unit/Plugins/Shard.php b/tests/Unit/Plugins/Shard.php index bd5a5e06..347a3035 100644 --- a/tests/Unit/Plugins/Shard.php +++ b/tests/Unit/Plugins/Shard.php @@ -232,11 +232,209 @@ describe('handleArguments', function () { $reflection = new ReflectionClass($shard); $method = $reflection->getMethod('removeParallelArguments'); - $arguments = ['bin/pest', '--parallel', 'tests/', '-p']; + $arguments = ['bin/pest', '--parallel', '--processes=4', 'tests/', '-p']; $result = $method->invoke($shard, $arguments); - expect($result)->toBe([0 => 'bin/pest', 2 => 'tests/']); + expect($result)->toBe(['bin/pest', 'tests/']); + }); +}); + +describe('parseListTestsOutput', function () { + it('parses Tests\\ namespaced classes from --list-tests output', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + $listOutput = <<<'OUT' + INFO Available tests: + + - P\Tests\Features\After::__pest_evaluable_it_runs + - P\Tests\Features\After::__pest_evaluable_it_runs_twice + - P\Tests\Unit\Foo::test_bar +OUT; + + expect($method->invoke($shard, $listOutput))->toBe([ + 'Tests\\Features\\After', + 'Tests\\Unit\\Foo', + ]); + }); + + it('deduplicates repeated class names from multiple test methods', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + $listOutput = <<<'OUT' + - P\Tests\Same::method_a + - P\Tests\Same::method_b + - P\Tests\Same::method_c +OUT; + + expect($method->invoke($shard, $listOutput))->toBe(['Tests\\Same']); + }); + + it('returns an empty list for output with no matching lines', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + expect($method->invoke($shard, ''))->toBe([]) + ->and($method->invoke($shard, 'some random text'))->toBe([]); + }); + + it('parses non-Tests namespaced classes', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + $listOutput = <<<'OUT' + - P\Acme\Sharding\OneTest::test_foo + - P\Acme\Sharding\TwoTest::test_bar + - App\Suite\BazTest::test_qux +OUT; + + expect($method->invoke($shard, $listOutput))->toBe([ + 'Acme\\Sharding\\OneTest', + 'Acme\\Sharding\\TwoTest', + 'App\\Suite\\BazTest', + ]); + }); + + it('parses unnamespaced top-level classes', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + expect($method->invoke($shard, ' - P\FooTest::test_bar'))->toBe(['FooTest']); + }); + + it('strips the P\\ Pest prefix but keeps the rest of the FQCN', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + $listOutput = <<<'OUT' + - P\Acme\OneTest::a + - Acme\TwoTest::b +OUT; + + expect($method->invoke($shard, $listOutput))->toBe([ + 'Acme\\OneTest', + 'Acme\\TwoTest', + ]); + }); + + it('ignores junk lines that lack the " - …::" framing', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('parseListTestsOutput'); + + $listOutput = <<<'OUT' + INFO Available tests: + +There were errors: +garbage ::: not a test + - P\Acme\RealTest::method +OUT; + + expect($method->invoke($shard, $listOutput))->toBe(['Acme\\RealTest']); + }); +}); + +describe('buildListTestsCommand', function () { + it('builds the list-tests command with the forwarded --test-directory', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('buildListTestsCommand'); + + $command = $method->invoke($shard, ['bin/pest', '--update-shards'], 'custom/suite'); + + expect($command)->toBe([ + 'php', + 'bin/pest', + '--update-shards', + '--test-directory=custom/suite', + '--list-tests', + ]); + }); + + it('strips --parallel and -p when building the list-tests command', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('buildListTestsCommand'); + + $command = $method->invoke($shard, ['bin/pest', '--parallel', '--update-shards', '-p'], 'tests'); + + expect($command)->toBe([ + 'php', + 'bin/pest', + '--update-shards', + '--test-directory=tests', + '--list-tests', + ]); + }); + + it('forwards --test-directory even when input arguments include one', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('buildListTestsCommand'); + + $command = $method->invoke($shard, ['bin/pest'], 'suites'); + + expect($command)->toContain('--test-directory=suites'); + }); + + it('strips --processes=N when building the list-tests command', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('buildListTestsCommand'); + + $command = $method->invoke($shard, ['bin/pest', '--parallel', '--processes=4', '--update-shards'], 'tests'); + + expect($command)->toBe([ + 'php', + 'bin/pest', + '--update-shards', + '--test-directory=tests', + '--list-tests', + ]); + }); + + it('strips --processes N (space-separated) when building the list-tests command', function () { + $output = new BufferedOutput; + $shard = new Shard($output); + + $reflection = new ReflectionClass($shard); + $method = $reflection->getMethod('buildListTestsCommand'); + + $command = $method->invoke($shard, ['bin/pest', '--parallel', '--processes', '4', '--update-shards'], 'tests'); + + expect($command)->not->toContain('--processes') + ->and($command)->toContain('--update-shards') + ->and($command)->toContain('--test-directory=tests'); }); });