From c6fecf0f1d0233760c5a3d18fee46c746fff05fb Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 18:30:02 +0200 Subject: [PATCH 1/8] feat(ci): exit non-zero when ci subcommand fails Module/Ci.php throws Components\Exception with an exit code instead of returning false-for-failure. Components.php catch arm exits with the exception code (0 normalized to 1). --- src/Components.php | 7 ++++- src/Module/Ci.php | 65 +++++++++++++++++++++++++--------------------- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/src/Components.php b/src/Components.php index 4229b136..5093eb20 100644 --- a/src/Components.php +++ b/src/Components.php @@ -224,7 +224,12 @@ public function __construct(Injector $injector, $parameters) } } catch (Exception $e) { $injector->getInstance(Output::class)->fail($e); - return; + // Module::handle() bool reports "did I match" only. Failure is + // reported by throwing an exception; its code drives the process + // exit code (0 is normalized to 1 since success-via-throw is a + // contradiction). + $code = $e->getCode(); + exit(is_int($code) && $code !== 0 ? $code : 1); } if (!$ran) { diff --git a/src/Module/Ci.php b/src/Module/Ci.php index d5b79f04..a3acf6f0 100644 --- a/src/Module/Ci.php +++ b/src/Module/Ci.php @@ -342,7 +342,10 @@ public function getHelp($action): string * @param array $arguments CLI arguments * @param Component|null $component The selected component (if any) * - * @return bool True if the module performed some action. + * @return bool True if the module recognized this invocation. + * This is *only* a "did I match" signal. Failures are + * reported by throwing Horde\Components\Exception; its + * getCode() drives the process exit code (0 → 1). */ public function handle(array $options, array $arguments, ?Component $component = null): bool { @@ -360,28 +363,23 @@ public function handle(array $options, array $arguments, ?Component $component = // Get output handler $output = $this->dependencies->get(Output::class); - try { - switch ($subcommand) { - case 'init': - return $this->handleInit($options, $output); + switch ($subcommand) { + case 'init': + return $this->handleInit($options, $output); - case 'check': - return $this->handleCheck($options, $output); + case 'check': + return $this->handleCheck($options, $output); - case 'setup': - return $this->handleSetup($options, $output); + case 'setup': + return $this->handleSetup($options, $output); - case 'run': - return $this->handleRun($options, $output); + case 'run': + return $this->handleRun($options, $output); - default: - $output->error("Unknown subcommand: {$subcommand}"); - $output->info('Valid subcommands: init, check, setup, run'); - return false; - } - } catch (Exception $e) { - $output->error('CI command failed: ' . $e->getMessage()); - return false; + default: + $output->error("Unknown subcommand: {$subcommand}"); + $output->info('Valid subcommands: init, check, setup, run'); + throw new Exception("Unknown ci subcommand: {$subcommand}", 2); } } @@ -419,7 +417,10 @@ private function handleInit(array $options, Output $output): bool $dryRun = isset($options['dry-run']) && $options['dry-run']; $initCommand = new InitCommand($output); - return $initCommand->execute($componentPath, $mode, $force, $dryRun); + if (!$initCommand->execute($componentPath, $mode, $force, $dryRun)) { + throw new Exception('ci init failed', 2); + } + return true; } /** @@ -434,7 +435,10 @@ private function handleCheck(array $options, Output $output): bool $componentPath = $options['local-path'] ?? getcwd(); $initCommand = new InitCommand($output); - return $initCommand->check($componentPath); + if (!$initCommand->check($componentPath)) { + throw new Exception('ci check: files are outdated', 1); + } + return true; } /** @@ -477,7 +481,10 @@ private function handleSetup(array $options, Output $output): bool new LaneScriptGenerator($output) ); - return $setupCommand->execute($config); + if (!$setupCommand->execute($config)) { + throw new Exception('ci setup reported failed lanes', 1); + } + return true; } /** @@ -495,7 +502,7 @@ private function handleRun(array $options, Output $output): bool if (!is_dir($workDir)) { $output->fail("Work directory does not exist: {$workDir}"); $output->info("Run 'horde-components ci setup' first to prepare test lanes."); - return false; + throw new Exception("Work directory does not exist: {$workDir}", 2); } // Determine horde-components path @@ -508,7 +515,7 @@ private function handleRun(array $options, Output $output): bool $componentsPath = realpath(__DIR__ . '/../../bin/horde-components'); if ($componentsPath === false) { $output->fail("Could not locate horde-components binary"); - return false; + throw new Exception('Could not locate horde-components binary', 2); } } @@ -526,12 +533,10 @@ private function handleRun(array $options, Output $output): bool $collector = new ResultCollector($output); $runCommand = new RunCommand($output, $collector, $componentsPath, $workDir, $apiClient); - try { - $exitCode = $runCommand->execute($workDir); - return $exitCode === 0; - } catch (Exception $e) { - $output->fail("CI run failed: " . $e->getMessage()); - return false; + $exitCode = $runCommand->execute($workDir); + if ($exitCode !== 0) { + throw new Exception("ci run reported test failures", $exitCode); } + return true; } } From 167540da7c9aa416cf98f293af6fc21434728f27 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 19:33:40 +0200 Subject: [PATCH 2/8] feat(ci): treat missing result file as lane failure --- src/Ci/Run/CheckReporter.php | 8 +- src/Ci/Run/PrCommentReporter.php | 9 +- src/Ci/Run/ResultCollector.php | 35 ++++---- src/Ci/Run/RunCommand.php | 37 ++++----- test/Unit/Ci/Run/ResultCollectorTest.php | 100 +++++++++++++++++++++++ test/Unit/Ci/Run/RunCommandTest.php | 4 +- 6 files changed, 145 insertions(+), 48 deletions(-) create mode 100644 test/Unit/Ci/Run/ResultCollectorTest.php diff --git a/src/Ci/Run/CheckReporter.php b/src/Ci/Run/CheckReporter.php index 1a9bd495..c6492682 100644 --- a/src/Ci/Run/CheckReporter.php +++ b/src/Ci/Run/CheckReporter.php @@ -66,11 +66,9 @@ public function reportLaneResults( foreach ($results as $laneName => $tools) { foreach ($tools as $toolName => $result) { - // Skip skipped tools - if (isset($result['skipped']) && $result['skipped']) { - continue; - } - + // Missing-result and real failures are both reported. + // (W4 will introduce deliberate skips; those will be filtered + // here when their carrier flag lands.) $this->createCheckRun( owner: $owner, repo: $repo, diff --git a/src/Ci/Run/PrCommentReporter.php b/src/Ci/Run/PrCommentReporter.php index 647ffbe8..53f316e9 100644 --- a/src/Ci/Run/PrCommentReporter.php +++ b/src/Ci/Run/PrCommentReporter.php @@ -164,7 +164,7 @@ private function generateCommentMarkdown( $tools = $results[$laneName] ?? []; foreach ($tools as $toolName => $result) { - if (!($result['success'] ?? false) && !isset($result['skipped'])) { + if (!($result['success'] ?? false)) { $md .= "- " . $this->formatToolFailure($toolName, $result) . "\n"; } } @@ -289,10 +289,6 @@ private function generateQualityMetrics(array $results): string private function isLanePassed(array $tools): bool { foreach ($tools as $result) { - if (isset($result['skipped']) && $result['skipped']) { - continue; - } - if (!($result['success'] ?? false)) { return false; } @@ -362,7 +358,8 @@ private function aggregateToolStats(array $results, string $tool): array $result = $tools[$tool]; - if (isset($result['skipped']) || isset($result['error'])) { + // Skip lanes with no usable statistics (missing JSON or load error) + if (isset($result['missing']) || isset($result['error'])) { continue; } diff --git a/src/Ci/Run/ResultCollector.php b/src/Ci/Run/ResultCollector.php index 77698e75..a8881c8a 100644 --- a/src/Ci/Run/ResultCollector.php +++ b/src/Ci/Run/ResultCollector.php @@ -93,18 +93,27 @@ public function addResultFromFile(string $laneName, string $tool, string $jsonFi } /** - * Add result for missing/skipped test. + * Record a missing result for a tool. + * + * Used when a result JSON was expected but never produced (lane script + * absent, lane script crashed before writing JSON, etc.). Counted as a + * lane failure: a missing JSON cannot be distinguished from a tool that + * succeeded silently and is therefore treated as the worst case. + * + * Deliberate, value-judged skips (e.g. PHPUnit constraint not satisfiable + * on the lane's PHP version) belong in a future addSkipped() with + * `success: true`; they are not modeled here. * * @param string $laneName Lane name * @param string $tool Tool name - * @param string $reason Reason for skipping + * @param string $reason Reason the result is missing */ - public function addSkipped(string $laneName, string $tool, string $reason): void + public function addMissing(string $laneName, string $tool, string $reason): void { $this->results[$laneName][$tool] = [ - 'success' => true, - 'exit_code' => 0, - 'skipped' => true, + 'success' => false, + 'exit_code' => 1, + 'missing' => true, 'reason' => $reason, ]; } @@ -174,9 +183,9 @@ private function displayToolResult(string $tool, array $result): void { $toolName = ucfirst($tool); - // Handle skipped - if (isset($result['skipped']) && $result['skipped']) { - $this->output->warn(" ⊘ {$toolName}: Skipped ({$result['reason']})"); + // Handle missing result file + if (isset($result['missing']) && $result['missing']) { + $this->output->error(" ✗ {$toolName}: Missing result ({$result['reason']})"); return; } @@ -267,12 +276,8 @@ public function getSummary(): array $lanePassed = true; foreach ($tools as $tool => $result) { - // Skipped is not a failure - if (isset($result['skipped']) && $result['skipped']) { - continue; - } - - // Check for failure + // success: false covers both real tool failures and missing + // result files (W2 — addMissing records success: false). if (!$result['success']) { $lanePassed = false; break; diff --git a/src/Ci/Run/RunCommand.php b/src/Ci/Run/RunCommand.php index fb0977d6..f3c32e3a 100644 --- a/src/Ci/Run/RunCommand.php +++ b/src/Ci/Run/RunCommand.php @@ -173,16 +173,16 @@ private function runLaneScript(array $lane): void if (!file_exists($scriptPath)) { $this->output->error("[{$lane['name']}] Script not found: {$scriptPath}"); $this->output->info(" Run 'horde-components ci setup' first"); - $this->collector->addSkipped($lane['name'], 'phpunit', 'Script not found'); - $this->collector->addSkipped($lane['name'], 'phpstan', 'Script not found'); + $this->collector->addMissing($lane['name'], 'phpunit', 'Script not found'); + $this->collector->addMissing($lane['name'], 'phpstan', 'Script not found'); return; } // Check if script is executable if (!is_executable($scriptPath)) { $this->output->error("[{$lane['name']}] Script not executable: {$scriptPath}"); - $this->collector->addSkipped($lane['name'], 'phpunit', 'Script not executable'); - $this->collector->addSkipped($lane['name'], 'phpstan', 'Script not executable'); + $this->collector->addMissing($lane['name'], 'phpunit', 'Script not executable'); + $this->collector->addMissing($lane['name'], 'phpstan', 'Script not executable'); return; } @@ -224,7 +224,7 @@ private function aggregateResults(array $lanes): void if (file_exists($phpunitFile)) { $this->collector->addResultFromFile($lane['name'], 'phpunit', $phpunitFile); } else { - $this->collector->addSkipped($lane['name'], 'phpunit', 'No result file found'); + $this->collector->addMissing($lane['name'], 'phpunit', 'No result file found'); } // Read PHPStan results @@ -232,7 +232,7 @@ private function aggregateResults(array $lanes): void if (file_exists($phpstanFile)) { $this->collector->addResultFromFile($lane['name'], 'phpstan', $phpstanFile); } else { - $this->collector->addSkipped($lane['name'], 'phpstan', 'No result file found'); + $this->collector->addMissing($lane['name'], 'phpstan', 'No result file found'); } // Read PHP CS Fixer results (only php8.4-dev) @@ -241,7 +241,7 @@ private function aggregateResults(array $lanes): void if (file_exists($csFixerFile)) { $this->collector->addResultFromFile($lane['name'], 'phpcsfixer', $csFixerFile); } else { - $this->collector->addSkipped($lane['name'], 'phpcsfixer', 'No result file found'); + $this->collector->addMissing($lane['name'], 'phpcsfixer', 'No result file found'); } } } @@ -323,11 +323,8 @@ private function generateSummaryMarkdown(): string private function isLanePassed(array $tools): bool { foreach ($tools as $result) { - // Skipped is not a failure - if (isset($result['skipped']) && $result['skipped']) { - continue; - } - + // success: false now covers both real tool failures and missing + // result files (W2 — addMissing records success: false). if (!($result['success'] ?? false)) { return false; } @@ -348,9 +345,9 @@ private function formatToolForTable(?array $result): string return '—'; } - // Skipped - if (isset($result['skipped']) && $result['skipped']) { - return '⊘ Skipped'; + // Missing result file (lane crashed before writing JSON, etc.) + if (isset($result['missing']) && $result['missing']) { + return '❌ Missing'; } // Error @@ -462,8 +459,8 @@ private function aggregateToolStats(array $results, string $tool): array $result = $tools[$tool]; - // Skip skipped/errored lanes - if (isset($result['skipped']) || isset($result['error'])) { + // Skip lanes with no usable statistics (missing JSON or load error) + if (isset($result['missing']) || isset($result['error'])) { continue; } @@ -792,9 +789,9 @@ private function formatToolForHtml(?array $result): string return '—'; } - // Skipped - if (isset($result['skipped']) && $result['skipped']) { - return '⊘ Skipped'; + // Missing result file (lane crashed before writing JSON, etc.) + if (isset($result['missing']) && $result['missing']) { + return '❌ Missing'; } // Error diff --git a/test/Unit/Ci/Run/ResultCollectorTest.php b/test/Unit/Ci/Run/ResultCollectorTest.php new file mode 100644 index 00000000..238b3f74 --- /dev/null +++ b/test/Unit/Ci/Run/ResultCollectorTest.php @@ -0,0 +1,100 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Ci\Run; + +use Horde\Components\Ci\Run\ResultCollector; +use Horde\Components\Output; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +/** + * Tests for ResultCollector — specifically the W2 missing-vs-passed contract. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(ResultCollector::class)] +class ResultCollectorTest extends TestCase +{ + public function testAddMissingMarksLaneAsFailed(): void + { + $collector = new ResultCollector($this->mockOutput()); + $collector->addMissing('php8.4-dev', 'phpunit', 'No result file found'); + + $this->assertFalse($collector->allPassed()); + $summary = $collector->getSummary(); + $this->assertSame(1, $summary['failed']); + $this->assertSame(0, $summary['passed']); + $this->assertSame(['php8.4-dev'], $summary['failed_lanes']); + } + + public function testMissingResultRecordsExpectedShape(): void + { + $collector = new ResultCollector($this->mockOutput()); + $collector->addMissing('php8.4-dev', 'phpunit', 'Script not found'); + + $results = $collector->getResults(); + $this->assertSame( + [ + 'success' => false, + 'exit_code' => 1, + 'missing' => true, + 'reason' => 'Script not found', + ], + $results['php8.4-dev']['phpunit'] + ); + } + + public function testMissingFromAddResultFromFileWhenJsonAbsent(): void + { + $collector = new ResultCollector($this->mockOutput()); + $collector->addResultFromFile('php8.4-dev', 'phpunit', '/nonexistent/path.json'); + + $this->assertFalse($collector->allPassed()); + $results = $collector->getResults(); + $this->assertFalse($results['php8.4-dev']['phpunit']['success']); + } + + public function testAllPassedWhenEveryToolSucceeded(): void + { + $collector = new ResultCollector($this->mockOutput()); + $jsonFile = $this->makeResultJson(['success' => true, 'exit_code' => 0]); + + try { + $collector->addResultFromFile('php8.4-dev', 'phpunit', $jsonFile); + $this->assertTrue($collector->allPassed()); + } finally { + @unlink($jsonFile); + } + } + + private function mockOutput(): Output + { + // Output is only used for displaySummary(); these tests hit + // the data-shape paths and do not call it. + return $this->createMock(Output::class); + } + + private function makeResultJson(array $payload): string + { + $path = tempnam(sys_get_temp_dir(), 'rct-'); + file_put_contents($path, json_encode($payload, JSON_THROW_ON_ERROR)); + return $path; + } +} diff --git a/test/Unit/Ci/Run/RunCommandTest.php b/test/Unit/Ci/Run/RunCommandTest.php index 18237e5d..3aeb9c74 100644 --- a/test/Unit/Ci/Run/RunCommandTest.php +++ b/test/Unit/Ci/Run/RunCommandTest.php @@ -368,10 +368,10 @@ public function testReportsSkippedWhenResultFileMissing(): void // No result files created - // Mock collector to expect skipped reports + // Mock collector to expect missing-result reports $this->collector ->expects($this->atLeast(2)) // PHPUnit and PHPStan - ->method('addSkipped') + ->method('addMissing') ->with( $this->stringContains('php8.4-dev'), $this->anything(), From 68e681f6542600080c65ce065f6042eb8a57bca4 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 20:25:28 +0200 Subject: [PATCH 3/8] feat(ci): pick PHPUnit version from component constraint, skip incompatible lanes --- src/Ci/Run/CheckReporter.php | 8 +- src/Ci/Run/PrCommentReporter.php | 4 +- src/Ci/Run/ResultCollector.php | 62 ++++++-- src/Ci/Run/RunCommand.php | 36 ++++- src/Ci/Setup/PhpUnitMatrix.php | 191 +++++++++++++++++++++++ src/Ci/Setup/PhpUnitSelection.php | 44 ++++++ src/Ci/Setup/SetupCommand.php | 130 ++++++++++++++- src/Ci/Setup/ToolCache.php | 78 ++++----- src/Qc/ToolFinder.php | 46 ++++-- test/Unit/Ci/Run/ResultCollectorTest.php | 31 ++++ test/Unit/Ci/Setup/PhpUnitMatrixTest.php | 167 ++++++++++++++++++++ 11 files changed, 717 insertions(+), 80 deletions(-) create mode 100644 src/Ci/Setup/PhpUnitMatrix.php create mode 100644 src/Ci/Setup/PhpUnitSelection.php create mode 100644 test/Unit/Ci/Setup/PhpUnitMatrixTest.php diff --git a/src/Ci/Run/CheckReporter.php b/src/Ci/Run/CheckReporter.php index c6492682..8620154c 100644 --- a/src/Ci/Run/CheckReporter.php +++ b/src/Ci/Run/CheckReporter.php @@ -66,9 +66,11 @@ public function reportLaneResults( foreach ($results as $laneName => $tools) { foreach ($tools as $toolName => $result) { - // Missing-result and real failures are both reported. - // (W4 will introduce deliberate skips; those will be filtered - // here when their carrier flag lands.) + // Don't post Check Runs for deliberately-skipped tools. + if (isset($result['deliberate_skip']) && $result['deliberate_skip']) { + continue; + } + $this->createCheckRun( owner: $owner, repo: $repo, diff --git a/src/Ci/Run/PrCommentReporter.php b/src/Ci/Run/PrCommentReporter.php index 53f316e9..4c2c07af 100644 --- a/src/Ci/Run/PrCommentReporter.php +++ b/src/Ci/Run/PrCommentReporter.php @@ -358,8 +358,8 @@ private function aggregateToolStats(array $results, string $tool): array $result = $tools[$tool]; - // Skip lanes with no usable statistics (missing JSON or load error) - if (isset($result['missing']) || isset($result['error'])) { + // Skip lanes with no usable statistics + if (isset($result['missing']) || isset($result['error']) || isset($result['deliberate_skip'])) { continue; } diff --git a/src/Ci/Run/ResultCollector.php b/src/Ci/Run/ResultCollector.php index a8881c8a..68ed1452 100644 --- a/src/Ci/Run/ResultCollector.php +++ b/src/Ci/Run/ResultCollector.php @@ -101,7 +101,7 @@ public function addResultFromFile(string $laneName, string $tool, string $jsonFi * succeeded silently and is therefore treated as the worst case. * * Deliberate, value-judged skips (e.g. PHPUnit constraint not satisfiable - * on the lane's PHP version) belong in a future addSkipped() with + * on the lane's PHP version) belong in {@see addSkipped()} with * `success: true`; they are not modeled here. * * @param string $laneName Lane name @@ -118,6 +118,30 @@ public function addMissing(string $laneName, string $tool, string $reason): void ]; } + /** + * Record a deliberate skip for a tool. + * + * Use when a tool was intentionally not run because no version of it + * satisfies the lane's constraints (e.g. PHPUnit ^12 on PHP 8.2). The + * lane is NOT counted as failed — there is nothing to fail. + * + * Distinct from {@see addMissing()}: that one signals an unexpected + * absence and IS a failure. + * + * @param string $laneName Lane name + * @param string $tool Tool name + * @param string $reason Why the tool was skipped + */ + public function addSkipped(string $laneName, string $tool, string $reason): void + { + $this->results[$laneName][$tool] = [ + 'success' => true, + 'exit_code' => 0, + 'deliberate_skip' => true, + 'reason' => $reason, + ]; + } + /** * Extract tool version from result data. * @@ -158,18 +182,22 @@ public function displaySummary(): void $this->output->bold("\n=== Summary ==="); if ($summary['passed'] > 0) { - $this->output->ok("✅ Passed: {$summary['passed']}/{$summary['total']} lanes"); + $this->output->ok("Passed: {$summary['passed']}/{$summary['total']} lanes"); + } + + if ($summary['skipped'] > 0) { + $this->output->skip("Skipped: {$summary['skipped']}/{$summary['total']} lanes"); } if ($summary['failed'] > 0) { - $this->output->error("❌ Failed: {$summary['failed']}/{$summary['total']} lanes"); + $this->output->error("Failed: {$summary['failed']}/{$summary['total']} lanes"); foreach ($summary['failed_lanes'] as $laneName) { $this->output->plain(" - {$laneName}"); } } if ($summary['failed'] === 0) { - $this->output->ok("\n✨ All lanes passed!"); + $this->output->ok("\nAll lanes passed!"); } } @@ -183,15 +211,21 @@ private function displayToolResult(string $tool, array $result): void { $toolName = ucfirst($tool); + // Handle deliberate skip (lane is incompatible with this tool) + if (isset($result['deliberate_skip']) && $result['deliberate_skip']) { + $this->output->skip(" {$toolName}: Skipped ({$result['reason']})"); + return; + } + // Handle missing result file if (isset($result['missing']) && $result['missing']) { - $this->output->error(" ✗ {$toolName}: Missing result ({$result['reason']})"); + $this->output->error(" {$toolName}: Missing result ({$result['reason']})"); return; } // Handle errors if (isset($result['error'])) { - $this->output->error(" ✗ {$toolName}: {$result['error']}"); + $this->output->error(" {$toolName}: {$result['error']}"); return; } @@ -200,9 +234,9 @@ private function displayToolResult(string $tool, array $result): void $statsStr = $stats ? " ({$stats})" : ''; if ($result['success']) { - $this->output->ok(" ✓ {$toolName}: Passed{$statsStr}"); + $this->output->ok(" {$toolName}: Passed{$statsStr}"); } else { - $this->output->error(" ✗ {$toolName}: FAILED{$statsStr}"); + $this->output->error(" {$toolName}: FAILED{$statsStr}"); } } @@ -264,18 +298,23 @@ private function formatStats(string $tool, array $stats): string /** * Get summary statistics. * - * @return array{passed: int, failed: int, total: int, failed_lanes: array} + * @return array{passed: int, failed: int, skipped: int, total: int, failed_lanes: array} */ public function getSummary(): array { $passed = 0; $failed = 0; + $skipped = 0; $failedLanes = []; foreach ($this->results as $laneName => $tools) { $lanePassed = true; + $laneAllSkipped = !empty($tools); foreach ($tools as $tool => $result) { + if (empty($result['deliberate_skip'])) { + $laneAllSkipped = false; + } // success: false covers both real tool failures and missing // result files (W2 — addMissing records success: false). if (!$result['success']) { @@ -284,7 +323,9 @@ public function getSummary(): array } } - if ($lanePassed) { + if ($laneAllSkipped) { + $skipped++; + } elseif ($lanePassed) { $passed++; } else { $failed++; @@ -295,6 +336,7 @@ public function getSummary(): array return [ 'passed' => $passed, 'failed' => $failed, + 'skipped' => $skipped, 'total' => count($this->results), 'failed_lanes' => $failedLanes, ]; diff --git a/src/Ci/Run/RunCommand.php b/src/Ci/Run/RunCommand.php index f3c32e3a..d4ba5a90 100644 --- a/src/Ci/Run/RunCommand.php +++ b/src/Ci/Run/RunCommand.php @@ -167,6 +167,15 @@ private function runLaneScript(array $lane): void { $scriptPath = $lane['dir'] . '/run-lane.sh'; + // Honour deliberate skips written by SetupCommand: don't try to run + // a script that was never generated, and don't print Script-not-found + // errors. Result aggregation will record the skip from skip.json. + $skipFile = $lane['component_dir'] . '/build/skip.json'; + if (file_exists($skipFile)) { + $this->output->skip("[{$lane['name']}] Deliberately skipped"); + return; + } + $this->output->info("[{$lane['name']}] Executing lane script..."); // Check if script exists @@ -219,6 +228,19 @@ private function aggregateResults(array $lanes): void foreach ($lanes as $lane) { $buildDir = $lane['component_dir'] . '/build'; + // Honour deliberate skips written by SetupCommand. + $skipFile = $buildDir . '/skip.json'; + if (file_exists($skipFile)) { + $skipData = json_decode((string) file_get_contents($skipFile), true); + if (is_array($skipData) && ($skipData['deliberate_skip'] ?? false)) { + $reason = (string) ($skipData['reason'] ?? 'Lane deliberately skipped'); + foreach ((array) ($skipData['tools'] ?? ['phpunit', 'phpstan']) as $tool) { + $this->collector->addSkipped($lane['name'], (string) $tool, $reason); + } + continue; + } + } + // Read PHPUnit results $phpunitFile = $buildDir . '/phpunit-results-summary.json'; if (file_exists($phpunitFile)) { @@ -345,6 +367,11 @@ private function formatToolForTable(?array $result): string return '—'; } + // Deliberately skipped (lane incompatible with this tool) + if (isset($result['deliberate_skip']) && $result['deliberate_skip']) { + return '⊘ Skipped'; + } + // Missing result file (lane crashed before writing JSON, etc.) if (isset($result['missing']) && $result['missing']) { return '❌ Missing'; @@ -459,8 +486,8 @@ private function aggregateToolStats(array $results, string $tool): array $result = $tools[$tool]; - // Skip lanes with no usable statistics (missing JSON or load error) - if (isset($result['missing']) || isset($result['error'])) { + // Skip lanes with no usable statistics + if (isset($result['missing']) || isset($result['error']) || isset($result['deliberate_skip'])) { continue; } @@ -789,6 +816,11 @@ private function formatToolForHtml(?array $result): string return '—'; } + // Deliberately skipped (lane incompatible with this tool) + if (isset($result['deliberate_skip']) && $result['deliberate_skip']) { + return '⊘ Skipped'; + } + // Missing result file (lane crashed before writing JSON, etc.) if (isset($result['missing']) && $result['missing']) { return '❌ Missing'; diff --git a/src/Ci/Setup/PhpUnitMatrix.php b/src/Ci/Setup/PhpUnitMatrix.php new file mode 100644 index 00000000..84dceebb --- /dev/null +++ b/src/Ci/Setup/PhpUnitMatrix.php @@ -0,0 +1,191 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Ci\Setup; + +use Horde\Components\Exception; +use Horde\Version\ConstraintParser; +use Horde\Version\InvalidVersionException; +use Horde\Version\RelaxedSemanticVersion; + +/** + * Pick a PHPUnit version that satisfies both a component's constraint and a + * lane's PHP version. + * + * Single source of truth for two questions, both of which were previously + * answered by hardcoded {php_version => phpunit_major} tables in + * ToolCache and ToolFinder: + * + * 1. Which PHPUnit PHAR should the cache download for a given lane? + * 2. Which cached PHAR should the run-lane script invoke? + * + * The supported-PHP-range table is hardcoded here; bump it when PHPUnit + * drops or adds a major. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +class PhpUnitMatrix +{ + /** + * Available PHPUnit majors with their authoritative tag and minimum PHP. + * + * Ordered newest-first so {@see pick()} can scan top-down for the + * highest satisfying entry. + * + * Keys are the major version. `tag` is the major.minor we actually + * download (the highest stable minor of that major). `minPhp` is the + * minimum PHP version per phpunit.de. + * + * @var array + */ + private const MAJORS = [ + 12 => ['tag' => '12.5', 'minPhp' => '8.3'], + 11 => ['tag' => '11.5', 'minPhp' => '8.2'], + 10 => ['tag' => '10.5', 'minPhp' => '8.1'], + 9 => ['tag' => '9.6', 'minPhp' => '7.3'], + ]; + + /** + * Pick the highest PHPUnit tag satisfying both the component's constraint + * and the lane's PHP version. + * + * @param string|null $constraint Composer-style constraint from the + * component's composer.json `require-dev`, + * or null when the component does not + * declare a PHPUnit constraint. + * @param string $phpVersion Lane PHP version, e.g. "8.2", "8.3.6". + * @return string|null Tag like "12.5", or null when no major satisfies + * both bounds. + */ + public function pick(?string $constraint, string $phpVersion): ?string + { + $parser = new ConstraintParser(); + $parsed = null; + + if ($constraint !== null && $constraint !== '') { + try { + $parsed = $parser->parse($constraint); + } catch (InvalidVersionException) { + // Unparseable constraints fall through to "highest compatible + // with PHP version" — preserves behaviour for components + // whose constraints we don't yet recognise. + $parsed = null; + } + } + + foreach (self::MAJORS as $major => $row) { + if (!$this->phpSupports($phpVersion, $row['minPhp'])) { + continue; + } + if ($parsed === null) { + return $row['tag']; + } + // Probe the major with its `tag.0` representative; that's + // enough for caret/tilde/range constraints to give a + // deterministic answer per major. + $probe = new RelaxedSemanticVersion($row['tag'] . '.0'); + if ($parsed->isSatisfiedBy($probe)) { + return $row['tag']; + } + } + + return null; + } + + /** + * Convenience: read the constraint from a component's composer.json and + * pick a tag for the given PHP version. + * + * @param string $componentComposerJsonPath Path to the component's + * composer.json file. + * @param string $phpVersion Lane PHP version. + * @return PhpUnitSelection Either a satisfying tag or a deliberate-skip + * with a human-readable reason. + * @throws Exception When composer.json is unreadable or malformed. + */ + public function pickWithSource( + string $componentComposerJsonPath, + string $phpVersion + ): PhpUnitSelection { + $constraint = $this->readConstraint($componentComposerJsonPath); + $tag = $this->pick($constraint, $phpVersion); + + if ($tag !== null) { + return new PhpUnitSelection($tag, null); + } + + $reason = $constraint === null + ? sprintf('No PHPUnit major supports PHP %s', $phpVersion) + : sprintf( + 'PHPUnit constraint "%s" not satisfiable on PHP %s', + $constraint, + $phpVersion + ); + return new PhpUnitSelection(null, $reason); + } + + /** + * Read the phpunit/phpunit constraint from a composer.json file. + * + * Looks in `require-dev` first, then `require`. Returns null when the + * file does not declare phpunit. + * + * @param string $path Path to composer.json. + * @return string|null Constraint string or null if not declared. + * @throws Exception If the file is missing or not valid JSON. + */ + public function readConstraint(string $path): ?string + { + if (!is_file($path)) { + throw new Exception("composer.json not found: {$path}"); + } + $raw = file_get_contents($path); + if ($raw === false) { + throw new Exception("Failed to read composer.json: {$path}"); + } + try { + /** @var array $data */ + $data = json_decode($raw, true, 64, JSON_THROW_ON_ERROR); + } catch (\JsonException $e) { + throw new Exception("Invalid JSON in {$path}: " . $e->getMessage(), 0, $e); + } + + foreach (['require-dev', 'require'] as $section) { + if (isset($data[$section]['phpunit/phpunit']) + && is_string($data[$section]['phpunit/phpunit'])) { + return $data[$section]['phpunit/phpunit']; + } + } + return null; + } + + /** + * Whether $phpVersion satisfies the >= $minPhp bound. + * + * Both arguments accept any number of dotted components; comparison + * uses PHP's native version_compare. + * + * @param string $phpVersion e.g. "8.2", "8.3.6" + * @param string $minPhp e.g. "8.3" + */ + private function phpSupports(string $phpVersion, string $minPhp): bool + { + return version_compare($phpVersion, $minPhp, '>='); + } +} diff --git a/src/Ci/Setup/PhpUnitSelection.php b/src/Ci/Setup/PhpUnitSelection.php new file mode 100644 index 00000000..7856a87c --- /dev/null +++ b/src/Ci/Setup/PhpUnitSelection.php @@ -0,0 +1,44 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Ci\Setup; + +/** + * Outcome of {@see PhpUnitMatrix::pickWithSource()}. + * + * Either: + * - `$tag` is a "major.minor" string and `$skipReason` is null (the lane + * should download/run that PHPUnit), OR + * - `$tag` is null and `$skipReason` carries the human-readable reason for + * skipping the lane (no PHPUnit satisfies the intersection). + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +final class PhpUnitSelection +{ + public function __construct( + public readonly ?string $tag, + public readonly ?string $skipReason + ) {} + + public function isSatisfied(): bool + { + return $this->tag !== null; + } +} diff --git a/src/Ci/Setup/SetupCommand.php b/src/Ci/Setup/SetupCommand.php index 3e3791cc..01c3925d 100644 --- a/src/Ci/Setup/SetupCommand.php +++ b/src/Ci/Setup/SetupCommand.php @@ -59,7 +59,8 @@ public function __construct( private readonly LaneCopier $laneCopier, private readonly ComposerInstaller $composerInstaller, private readonly ToolCache $toolCache, - private readonly LaneScriptGenerator $laneScriptGenerator + private readonly LaneScriptGenerator $laneScriptGenerator, + private readonly PhpUnitMatrix $phpUnitMatrix = new PhpUnitMatrix(), ) {} /** @@ -146,6 +147,15 @@ public function execute(CiConfig $config): bool $this->laneCopier->copyToLanes($config); $this->output->plain(''); + // Detect lanes deliberately incompatible with the component's + // declared PHPUnit constraint. Each such lane gets a sidecar + // build/skip.json so RunCommand can mark it as a deliberate skip + // (NOT a failure) without ever invoking composer install. + $skippedLanes = $this->markIncompatibleLanes($lanes); + if (!empty($skippedLanes)) { + $this->output->plain(''); + } + // Run composer install for each lane $this->output->bold('=== Running composer install ==='); $successful = 0; @@ -156,6 +166,12 @@ public function execute(CiConfig $config): bool $total = count($lanes); $this->output->info("[{$num}/{$total}] PHP {$lane['php']} ({$lane['stability']})"); + if (in_array($index, $skippedLanes, true)) { + $this->output->skip('Skipped (incompatible PHPUnit constraint)'); + $this->output->plain(''); + continue; + } + try { $phpBinary = $this->phpInstaller->getPhpBinary($lane['php']); @@ -196,6 +212,11 @@ public function execute(CiConfig $config): bool $this->output->info("[{$num}/{$total}] {$laneName}"); + if (in_array($index, $skippedLanes, true)) { + $this->output->skip('Skipped (no script generated)'); + continue; + } + $laneConfig = [ 'lane_name' => $laneName, 'php_version' => $lane['php'], @@ -221,6 +242,10 @@ public function execute(CiConfig $config): bool $this->output->bold('=== Setup Summary ==='); $this->output->ok("Successful lanes: {$successful}"); + if (!empty($skippedLanes)) { + $this->output->info("Skipped lanes: " . count($skippedLanes) . " (incompatible PHPUnit constraint)"); + } + if ($failed > 0) { $this->output->warn("Failed lanes: {$failed}"); } @@ -234,7 +259,9 @@ public function execute(CiConfig $config): bool // Download QC tools to cache $this->output->bold('=== Downloading QC Tools ==='); try { - $this->toolCache->ensureAllTools($testableVersions); + $this->toolCache->ensureAllTools( + $this->collectPhpUnitTags($lanes, $skippedLanes) + ); $this->output->ok('All tools downloaded'); } catch (Exception $e) { $this->output->error("Tool download failed: " . $e->getMessage()); @@ -306,4 +333,103 @@ private function readComponentInfo(string $componentPath): array 'required_extensions' => $extensions, ]; } + + /** + * Detect which lanes are deliberately incompatible with the component's + * PHPUnit constraint and write a sidecar build/skip.json so RunCommand + * marks them as deliberate skips rather than running them. + * + * @param array $lanes + * @return array Indexes of lanes that were marked as skipped + */ + private function markIncompatibleLanes(array $lanes): array + { + $skipped = []; + + foreach ($lanes as $index => $lane) { + $composerJson = $lane['dir'] . '/composer.json'; + if (!is_file($composerJson)) { + // Lane directory not yet populated, or component lacks + // composer.json. Either way we cannot decide; let + // composer install fail naturally. + continue; + } + + try { + $selection = $this->phpUnitMatrix->pickWithSource( + $composerJson, + $lane['php'] + ); + } catch (Exception $e) { + $this->output->warn( + "Could not evaluate PHPUnit compatibility for {$lane['dir']}: " + . $e->getMessage() + ); + continue; + } + + if ($selection->isSatisfied()) { + continue; + } + + // Persist the skip reason so RunCommand can pick it up. + $buildDir = $lane['dir'] . '/build'; + if (!is_dir($buildDir) && !mkdir($buildDir, 0o755, true) && !is_dir($buildDir)) { + throw new Exception("Failed to create build directory: {$buildDir}"); + } + file_put_contents( + $buildDir . '/skip.json', + json_encode( + [ + 'deliberate_skip' => true, + 'tools' => ['phpunit', 'phpstan'], + 'reason' => $selection->skipReason, + ], + JSON_PRETTY_PRINT | JSON_THROW_ON_ERROR + ) + ); + + $laneName = 'php' . $lane['php'] . '-' . $lane['stability']; + $this->output->skip("{$laneName}: " . $selection->skipReason); + $skipped[] = $index; + } + + return $skipped; + } + + /** + * Compute the set of PHPUnit tags surviving lanes will need. + * + * Iterates the lanes that were not deliberately skipped, asks the + * matrix which PHPUnit tag fits each, and returns the deduped list. + * + * @param array $lanes + * @param array $skippedLanes Indexes of lanes that were skipped + * @return array Unique PHPUnit tags (e.g. ["11.5", "12.5"]) + */ + private function collectPhpUnitTags(array $lanes, array $skippedLanes): array + { + $tags = []; + foreach ($lanes as $index => $lane) { + if (in_array($index, $skippedLanes, true)) { + continue; + } + $composerJson = $lane['dir'] . '/composer.json'; + if (!is_file($composerJson)) { + continue; + } + try { + $selection = $this->phpUnitMatrix->pickWithSource( + $composerJson, + $lane['php'] + ); + } catch (Exception) { + continue; + } + if ($selection->tag !== null) { + $tags[] = $selection->tag; + } + } + return array_values(array_unique($tags)); + } } diff --git a/src/Ci/Setup/ToolCache.php b/src/Ci/Setup/ToolCache.php index 9393f3c1..8600e2fd 100644 --- a/src/Ci/Setup/ToolCache.php +++ b/src/Ci/Setup/ToolCache.php @@ -36,19 +36,6 @@ */ class ToolCache { - /** - * PHPUnit version mapping for PHP versions. - * - * PHPUnit 11.x for PHP 8.2-8.3 - * PHPUnit 12.x for PHP 8.4+ - */ - private const PHPUNIT_VERSIONS = [ - '8.2' => '11.5', - '8.3' => '11.5', - '8.4' => '12.5', - '8.5' => '12.5', - ]; - /** * Constructor. * @@ -61,21 +48,25 @@ public function __construct( ) {} /** - * Ensure all required tools are cached for given PHP versions. + * Ensure all required tools are cached for the given PHPUnit tags. * - * @param array $phpVersions PHP versions to prepare tools for + * @param array $phpUnitTags PHPUnit major.minor tags to download + * (e.g. ["11.5", "12.5"]). Caller is + * responsible for picking these via + * {@see PhpUnitMatrix}; this method + * does not do version selection. * @throws Exception If download fails */ - public function ensureAllTools(array $phpVersions): void + public function ensureAllTools(array $phpUnitTags): void { // Create cache directory if (!is_dir($this->cacheDir)) { mkdir($this->cacheDir, 0o755, true); } - // Download PHPUnit for each PHP version - foreach ($phpVersions as $phpVersion) { - $this->ensurePhpUnit($phpVersion); + // Download each requested PHPUnit tag, deduped. + foreach (array_unique($phpUnitTags) as $tag) { + $this->ensurePhpUnit($tag); } // Download PHPStan (single version for all PHP versions) @@ -86,33 +77,28 @@ public function ensureAllTools(array $phpVersions): void } /** - * Ensure PHPUnit is cached for specific PHP version. + * Ensure PHPUnit is cached at the given tag. * - * @param string $phpVersion PHP version (e.g., "8.4") + * @param string $tag PHPUnit major.minor tag (e.g. "12.5") * @return string Path to PHPUnit PHAR - * @throws Exception If version not supported or download fails + * @throws Exception If download fails */ - public function ensurePhpUnit(string $phpVersion): string + public function ensurePhpUnit(string $tag): string { - if (!isset(self::PHPUNIT_VERSIONS[$phpVersion])) { - throw new Exception("No PHPUnit version mapped for PHP {$phpVersion}"); - } - - $phpunitVersion = self::PHPUNIT_VERSIONS[$phpVersion]; - $pharName = "phpunit-{$phpunitVersion}.phar"; + $pharName = "phpunit-{$tag}.phar"; $pharPath = $this->cacheDir . '/' . $pharName; if (file_exists($pharPath)) { - $this->output->plain(" ✓ PHPUnit {$phpunitVersion} (cached)"); + $this->output->plain(" PHPUnit {$tag} (cached)"); return $pharPath; } - $this->output->info(" ⬇ Downloading PHPUnit {$phpunitVersion}..."); + $this->output->info(" Downloading PHPUnit {$tag}..."); - $url = "https://phar.phpunit.de/phpunit-{$phpunitVersion}.phar"; + $url = "https://phar.phpunit.de/phpunit-{$tag}.phar"; $this->downloadPhar($url, $pharPath); - $this->output->ok(" ✓ PHPUnit {$phpunitVersion}"); + $this->output->ok(" PHPUnit {$tag}"); return $pharPath; } @@ -128,16 +114,16 @@ public function ensurePhpStan(): string $pharPath = $this->cacheDir . '/' . $pharName; if (file_exists($pharPath)) { - $this->output->plain(" ✓ PHPStan (cached)"); + $this->output->plain(" PHPStan (cached)"); return $pharPath; } - $this->output->info(" ⬇ Downloading PHPStan (latest)..."); + $this->output->info(" Downloading PHPStan (latest)..."); $url = 'https://github.com/phpstan/phpstan/releases/latest/download/phpstan.phar'; $this->downloadPhar($url, $pharPath); - $this->output->ok(" ✓ PHPStan"); + $this->output->ok(" PHPStan"); return $pharPath; } @@ -153,34 +139,28 @@ public function ensurePhpCsFixer(): string $pharPath = $this->cacheDir . '/' . $pharName; if (file_exists($pharPath)) { - $this->output->plain(" ✓ PHP-CS-Fixer (cached)"); + $this->output->plain(" PHP-CS-Fixer (cached)"); return $pharPath; } - $this->output->info(" ⬇ Downloading PHP-CS-Fixer (latest)..."); + $this->output->info(" Downloading PHP-CS-Fixer (latest)..."); $url = 'https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/releases/latest/download/php-cs-fixer.phar'; $this->downloadPhar($url, $pharPath); - $this->output->ok(" ✓ PHP-CS-Fixer"); + $this->output->ok(" PHP-CS-Fixer"); return $pharPath; } /** - * Get the path to PHPUnit for a specific PHP version. + * Get the path to a cached PHPUnit PHAR by tag. * - * @param string $phpVersion PHP version (e.g., "8.4") + * @param string $tag PHPUnit major.minor tag (e.g. "12.5") * @return string|null Path to PHPUnit PHAR or null if not cached */ - public function getPhpUnitPath(string $phpVersion): ?string + public function getPhpUnitPath(string $tag): ?string { - if (!isset(self::PHPUNIT_VERSIONS[$phpVersion])) { - return null; - } - - $phpunitVersion = self::PHPUNIT_VERSIONS[$phpVersion]; - $pharPath = $this->cacheDir . "/phpunit-{$phpunitVersion}.phar"; - + $pharPath = $this->cacheDir . "/phpunit-{$tag}.phar"; return file_exists($pharPath) ? $pharPath : null; } diff --git a/src/Qc/ToolFinder.php b/src/Qc/ToolFinder.php index e9dbe212..b4231518 100644 --- a/src/Qc/ToolFinder.php +++ b/src/Qc/ToolFinder.php @@ -11,6 +11,7 @@ namespace Horde\Components\Qc; +use Horde\Components\Ci\Setup\PhpUnitMatrix; use Phar; use PharException; use Throwable; @@ -83,19 +84,14 @@ public function findBinary(string $toolName, bool $checkPhar = true): ?string // 0. Tools directory (highest priority - for CI mode) if (!empty($this->toolsDir)) { - // For PHPUnit, prefer version-specific PHARs based on PHP version + // For PHPUnit, ask PhpUnitMatrix for the right tag based on the + // component's composer.json constraint and the running PHP. Falls + // back to "highest compatible" when the component does not declare + // a constraint (matrix returns the highest tag for the PHP). if ($toolName === 'phpunit' && $checkPhar) { - $phpVersion = PHP_VERSION_ID; - - // PHPUnit 11.x for PHP 8.2-8.3, PHPUnit 12.x for PHP 8.4+ - if ($phpVersion < 80400) { - // PHP 8.2-8.3: Try PHPUnit 11.5 first - $locations[] = $this->toolsDir . '/phpunit-11.5.phar'; - $locations[] = $this->toolsDir . '/phpunit-11.phar'; - } else { - // PHP 8.4+: Try PHPUnit 12.5 first - $locations[] = $this->toolsDir . '/phpunit-12.5.phar'; - $locations[] = $this->toolsDir . '/phpunit-12.phar'; + $tag = $this->resolvePhpUnitTag(); + if ($tag !== null) { + $locations[] = $this->toolsDir . '/phpunit-' . $tag . '.phar'; } } @@ -349,4 +345,30 @@ private function findAutoloaderForTool(string $toolPath): ?string return null; } + + /** + * Resolve which PHPUnit tag this lane should run via PhpUnitMatrix. + * + * Reads the component's composer.json (if present) and intersects the + * declared phpunit/phpunit constraint with the lane's running PHP. + * + * @return string|null Tag like "12.5", or null when neither the + * constraint nor the PHP version yields a hit + * (caller's lookup falls through to vendor/bin etc.). + */ + private function resolvePhpUnitTag(): ?string + { + $matrix = new PhpUnitMatrix(); + $phpVersion = PHP_MAJOR_VERSION . '.' . PHP_MINOR_VERSION; + + $composerJson = $this->getComponentPath() . '/composer.json'; + if (is_file($composerJson)) { + try { + return $matrix->pickWithSource($composerJson, $phpVersion)->tag; + } catch (Throwable) { + // Fall through to constraint-less lookup below. + } + } + return $matrix->pick(null, $phpVersion); + } } diff --git a/test/Unit/Ci/Run/ResultCollectorTest.php b/test/Unit/Ci/Run/ResultCollectorTest.php index 238b3f74..d473c51c 100644 --- a/test/Unit/Ci/Run/ResultCollectorTest.php +++ b/test/Unit/Ci/Run/ResultCollectorTest.php @@ -84,6 +84,37 @@ public function testAllPassedWhenEveryToolSucceeded(): void } } + public function testAddSkippedDoesNotFailLane(): void + { + $collector = new ResultCollector($this->mockOutput()); + $collector->addSkipped('php8.2-dev', 'phpunit', 'PHPUnit ^12 not satisfiable on PHP 8.2'); + + $this->assertTrue($collector->allPassed()); + $summary = $collector->getSummary(); + // A lane whose only result is a deliberate skip is counted as + // skipped, not passed. + $this->assertSame(0, $summary['passed']); + $this->assertSame(1, $summary['skipped']); + $this->assertSame(0, $summary['failed']); + } + + public function testDeliberateSkipShape(): void + { + $collector = new ResultCollector($this->mockOutput()); + $collector->addSkipped('php8.2-dev', 'phpunit', 'incompatible'); + + $results = $collector->getResults(); + $this->assertSame( + [ + 'success' => true, + 'exit_code' => 0, + 'deliberate_skip' => true, + 'reason' => 'incompatible', + ], + $results['php8.2-dev']['phpunit'] + ); + } + private function mockOutput(): Output { // Output is only used for displaySummary(); these tests hit diff --git a/test/Unit/Ci/Setup/PhpUnitMatrixTest.php b/test/Unit/Ci/Setup/PhpUnitMatrixTest.php new file mode 100644 index 00000000..e120e788 --- /dev/null +++ b/test/Unit/Ci/Setup/PhpUnitMatrixTest.php @@ -0,0 +1,167 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Ci\Setup; + +use Horde\Components\Ci\Setup\PhpUnitMatrix; +use Horde\Components\Exception; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +/** + * Tests for the PHPUnit version selector. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(PhpUnitMatrix::class)] +class PhpUnitMatrixTest extends TestCase +{ + public function testCaret12OnPhp82IsUnsatisfiable(): void + { + $this->assertNull((new PhpUnitMatrix())->pick('^12', '8.2')); + } + + public function testCaret12OnPhp83Picks125(): void + { + $this->assertSame('12.5', (new PhpUnitMatrix())->pick('^12', '8.3')); + } + + public function testCaret12OnPhp84Picks125(): void + { + $this->assertSame('12.5', (new PhpUnitMatrix())->pick('^12', '8.4')); + } + + public function testCaret11Or12OnPhp82Picks115(): void + { + $this->assertSame('11.5', (new PhpUnitMatrix())->pick('^11 || ^12', '8.2')); + } + + public function testCaret11Or12OnPhp84Picks125(): void + { + // Highest-satisfying wins. + $this->assertSame('12.5', (new PhpUnitMatrix())->pick('^11 || ^12', '8.4')); + } + + public function testNullConstraintFallsBackToHighestForPhpVersion(): void + { + $matrix = new PhpUnitMatrix(); + $this->assertSame('12.5', $matrix->pick(null, '8.4')); + $this->assertSame('11.5', $matrix->pick(null, '8.2')); + } + + public function testEmptyConstraintTreatedAsNull(): void + { + $this->assertSame('12.5', (new PhpUnitMatrix())->pick('', '8.4')); + } + + public function testInvalidConstraintFallsBackToHighestForPhpVersion(): void + { + // Garbage that the parser cannot understand should not crash; + // it falls through to the PHP-only branch. + $this->assertSame('11.5', (new PhpUnitMatrix())->pick('totally bogus', '8.2')); + } + + public function testPickWithSourceReadsRequireDev(): void + { + $tmp = $this->writeComposerJson([ + 'name' => 'horde/example', + 'require-dev' => ['phpunit/phpunit' => '^12'], + ]); + + try { + $sel = (new PhpUnitMatrix())->pickWithSource($tmp, '8.4'); + $this->assertTrue($sel->isSatisfied()); + $this->assertSame('12.5', $sel->tag); + $this->assertNull($sel->skipReason); + } finally { + @unlink($tmp); + } + } + + public function testPickWithSourceFallsBackToRequire(): void + { + $tmp = $this->writeComposerJson([ + 'name' => 'horde/example', + 'require' => ['phpunit/phpunit' => '^11'], + ]); + + try { + $sel = (new PhpUnitMatrix())->pickWithSource($tmp, '8.2'); + $this->assertSame('11.5', $sel->tag); + } finally { + @unlink($tmp); + } + } + + public function testPickWithSourceReturnsSkipReasonOnEmptyIntersection(): void + { + $tmp = $this->writeComposerJson([ + 'name' => 'horde/example', + 'require-dev' => ['phpunit/phpunit' => '^12'], + ]); + + try { + $sel = (new PhpUnitMatrix())->pickWithSource($tmp, '8.2'); + $this->assertFalse($sel->isSatisfied()); + $this->assertNull($sel->tag); + $this->assertNotNull($sel->skipReason); + $this->assertStringContainsString('^12', $sel->skipReason); + $this->assertStringContainsString('8.2', $sel->skipReason); + } finally { + @unlink($tmp); + } + } + + public function testReadConstraintMissingFile(): void + { + $this->expectException(Exception::class); + (new PhpUnitMatrix())->readConstraint('/nonexistent/composer.json'); + } + + public function testReadConstraintInvalidJson(): void + { + $tmp = tempnam(sys_get_temp_dir(), 'phpunitmatrix-'); + file_put_contents($tmp, '{ not valid json'); + + try { + $this->expectException(Exception::class); + (new PhpUnitMatrix())->readConstraint($tmp); + } finally { + @unlink($tmp); + } + } + + public function testReadConstraintReturnsNullWhenAbsent(): void + { + $tmp = $this->writeComposerJson(['name' => 'horde/example']); + + try { + $this->assertNull((new PhpUnitMatrix())->readConstraint($tmp)); + } finally { + @unlink($tmp); + } + } + + private function writeComposerJson(array $payload): string + { + $path = tempnam(sys_get_temp_dir(), 'phpunitmatrix-'); + file_put_contents($path, json_encode($payload, JSON_THROW_ON_ERROR)); + return $path; + } +} From b3f002cb1ef9ff05b5f10240df850180bf54b0eb Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 20:46:13 +0200 Subject: [PATCH 4/8] feat(ci): bump GitHub Actions to v5 (Node 24 native) --- data/ci/workflow.yml.template | 8 ++++---- src/Ci/Template/TemplateRenderer.php | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/data/ci/workflow.yml.template b/data/ci/workflow.yml.template index ee53fea7..72748a79 100644 --- a/data/ci/workflow.yml.template +++ b/data/ci/workflow.yml.template @@ -22,16 +22,16 @@ jobs: steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@v5 - name: Cache horde-components.phar - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: {{WORK_DIR}}/bin key: components-phar-${{ hashFiles('bin/ci-bootstrap.sh') }} - name: Cache QC tools (PHPUnit, PHPStan, PHP-CS-Fixer) - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: {{WORK_DIR}}/tools key: ci-tools-${{ hashFiles('.horde.yml') }} @@ -44,7 +44,7 @@ jobs: - name: Upload test results if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v5 with: name: ci-results-${{ github.run_number }} path: | diff --git a/src/Ci/Template/TemplateRenderer.php b/src/Ci/Template/TemplateRenderer.php index e9e5fdaa..d243ae5e 100644 --- a/src/Ci/Template/TemplateRenderer.php +++ b/src/Ci/Template/TemplateRenderer.php @@ -38,7 +38,7 @@ class TemplateRenderer * This version is embedded in generated files and used to detect * when components are using outdated templates. */ - private const TEMPLATE_VERSION = '1.1.0'; + private const TEMPLATE_VERSION = '1.2.0'; /** * Constructor. From 63b080b67243ba733606b3dde9011392dc4d7070 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 21:00:15 +0200 Subject: [PATCH 5/8] feat(ci): make PHPStan work when launched from a phar --- box.json | 1 + box.json.dist | 1 + src/Qc/PhpStanRuleExtractor.php | 148 ++++++++++++++++++++++ src/Qc/Task/Phpstan.php | 60 +++++++-- test/Unit/Qc/PhpStanRuleExtractorTest.php | 87 +++++++++++++ 5 files changed, 283 insertions(+), 14 deletions(-) create mode 100644 src/Qc/PhpStanRuleExtractor.php create mode 100644 test/Unit/Qc/PhpStanRuleExtractorTest.php diff --git a/box.json b/box.json index a2eefb30..5e9c918e 100644 --- a/box.json +++ b/box.json @@ -10,6 +10,7 @@ "files": [ "LICENSE", "bin/horde-components", + "phpstan-bootstrap.php", ".php-cs-fixer.dist.php" ], "finder": [ diff --git a/box.json.dist b/box.json.dist index a2eefb30..5e9c918e 100644 --- a/box.json.dist +++ b/box.json.dist @@ -10,6 +10,7 @@ "files": [ "LICENSE", "bin/horde-components", + "phpstan-bootstrap.php", ".php-cs-fixer.dist.php" ], "finder": [ diff --git a/src/Qc/PhpStanRuleExtractor.php b/src/Qc/PhpStanRuleExtractor.php new file mode 100644 index 00000000..bdca8b0b --- /dev/null +++ b/src/Qc/PhpStanRuleExtractor.php @@ -0,0 +1,148 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Qc; + +use Horde\Components\Exception; +use Phar; + +/** + * Materialize the PHPStan custom-rules bootstrap on disk. + * + * The Horde-specific PHPStan rules ship inside `horde-components.phar`. PHPStan + * itself runs as a separate phar process and cannot read into another phar's + * stream wrapper, so when horde-components is invoked from a phar (the CI + * mode) the bootstrap and rule files have to be extracted to a real + * filesystem directory first. + * + * Outside a phar (developer-checkout invocation) the rules are already on + * disk; the extractor short-circuits and returns the in-tree path. + * + * The bootstrap and the rule files share a `__DIR__`-relative layout: + * + * /phpstan-bootstrap.php + * /src/PhpStan/Rules/*.php + * /src/PhpStan/Rules/Draft/*.php + * + * The extractor preserves that layout so `phpstan-bootstrap.php`'s + * `require_once __DIR__ . '/src/PhpStan/Rules/...';` calls keep working. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +class PhpStanRuleExtractor +{ + /** + * Source files to extract relative to the components root, both + * inside the phar and on a developer checkout. + */ + private const FILES = [ + 'phpstan-bootstrap.php', + 'src/PhpStan/Rules/NoDirectGlobalAccessRule.php', + 'src/PhpStan/Rules/RequireImmutableUriUsageRule.php', + 'src/PhpStan/Rules/NoDeprecatedHordeUtilRule.php', + 'src/PhpStan/Rules/Draft/NoExitInLibraryCodeRule.php', + 'src/PhpStan/Rules/Draft/RequireDependencyInjectionRule.php', + 'src/PhpStan/Rules/Draft/RequireCoversClassAttributeRule.php', + ]; + + /** + * Subdirectory under $cacheDir into which the rule tree is extracted. + */ + private const CACHE_SUBDIR = 'phpstan-rules'; + + /** + * Return an on-disk path to phpstan-bootstrap.php usable by a separate + * PHPStan process via --autoload-file. + * + * - When running from a phar, extracts bootstrap + rule files into + * `$cacheDir/phpstan-rules/` (idempotent) and returns the on-disk + * bootstrap path there. + * - When running from a developer checkout, returns the in-tree + * path unchanged. The cacheDir argument is ignored. + * + * @param string|null $cacheDir Directory the extractor may write to. + * Required when running from a phar. + * @return string Absolute path to a readable phpstan-bootstrap.php. + * @throws Exception If running from a phar and the cache directory is + * missing, or extraction fails. + */ + public function extract(?string $cacheDir): string + { + $pharRoot = Phar::running(false); + + if ($pharRoot === '') { + // Developer checkout — the bootstrap is already on disk. + return $this->checkoutBootstrapPath(); + } + + if ($cacheDir === null || $cacheDir === '') { + throw new Exception( + 'PhpStanRuleExtractor: a cacheDir is required when running ' + . 'from a phar (got null/empty).' + ); + } + + $destRoot = rtrim($cacheDir, '/') . '/' . self::CACHE_SUBDIR; + $destBootstrap = $destRoot . '/phpstan-bootstrap.php'; + + if (is_file($destBootstrap)) { + // Idempotent — assume a previous extract already populated the tree. + return $destBootstrap; + } + + $sourceRoot = 'phar://' . $pharRoot; + foreach (self::FILES as $relative) { + $sourcePath = $sourceRoot . '/' . $relative; + $destPath = $destRoot . '/' . $relative; + + // Drafts are guarded by class_exists() in the bootstrap; skip + // gracefully if a draft rule was removed from the phar. + if (!file_exists($sourcePath)) { + continue; + } + + $destDir = dirname($destPath); + if (!is_dir($destDir) && !mkdir($destDir, 0o755, true) && !is_dir($destDir)) { + throw new Exception("Failed to create directory: {$destDir}"); + } + + if (!copy($sourcePath, $destPath)) { + throw new Exception("Failed to extract {$relative} to {$destPath}"); + } + } + + if (!is_file($destBootstrap)) { + throw new Exception( + "PhpStanRuleExtractor: phpstan-bootstrap.php missing from " + . "phar after extraction (looked at {$sourceRoot}/phpstan-bootstrap.php)." + ); + } + + return $destBootstrap; + } + + /** + * Path to the bootstrap in a developer checkout. + */ + private function checkoutBootstrapPath(): string + { + // src/Qc/PhpStanRuleExtractor.php → up 3 levels → repo root. + return dirname(__DIR__, 2) . '/phpstan-bootstrap.php'; + } +} diff --git a/src/Qc/Task/Phpstan.php b/src/Qc/Task/Phpstan.php index 9165a8e2..3442be82 100644 --- a/src/Qc/Task/Phpstan.php +++ b/src/Qc/Task/Phpstan.php @@ -11,6 +11,7 @@ namespace Horde\Components\Qc\Task; +use Horde\Components\Qc\PhpStanRuleExtractor; use Horde\Components\Qc\ToolFinder; use Throwable; @@ -73,7 +74,7 @@ public function getName(): string */ public function validate(array $options = []): array { - $binary = $this->findPhpStanBinary(); + $binary = $this->findPhpStanBinary($options['tools_dir'] ?? null); if ($binary === null) { return ['PHPStan is not installed!']; @@ -125,13 +126,30 @@ public function run(array &$options = []): int $watermarkResult = $this->testLevel($binary, $componentPath, $watermark, $options); if (!$watermarkResult['passed']) { - // CODE REGRESSION - fails at watermark! - $this->getOutput()->regression( - 'Code fails at watermark level ' . $watermark - . ' (' . $watermarkResult['errors'] . ' error' - . ($watermarkResult['errors'] !== 1 ? 's' : '') . ')' - ); - $this->getOutput()->warn('Watermark level MUST pass - fix these errors!'); + if ($watermarkResult['errors'] > 0) { + // CODE REGRESSION - fails at watermark with concrete errors + $this->getOutput()->regression( + 'Code fails at watermark level ' . $watermark + . ' (' . $watermarkResult['errors'] . ' error' + . ($watermarkResult['errors'] !== 1 ? 's' : '') . ')' + ); + $this->getOutput()->warn('Watermark level MUST pass - fix these errors!'); + } else { + // TOOLING FAILURE - PHPStan refused to start (config error, + // missing autoload-file, unloadable rule classes, etc.). + // Surface the raw output so the caller can diagnose. + $this->getOutput()->error( + 'PHPStan exited with code ' . $watermarkResult['exit_code'] + . ' before producing results. This is a tooling failure ' + . 'rather than a code regression.' + ); + $rawOutput = (string) ($watermarkResult['raw_output'] ?? ''); + if ($rawOutput !== '') { + foreach (array_slice(explode("\n", $rawOutput), 0, 20) as $line) { + $this->getOutput()->plain($line); + } + } + } // Parse results for output $this->nativeResults = $watermarkResult['results']; @@ -140,7 +158,7 @@ public function run(array &$options = []): int $this->writeJsonResults($componentPath, $watermarkResult['exit_code'], $watermark); $this->outputStatistics(); - return $watermarkResult['errors']; // Non-zero = failure + return max(1, $watermarkResult['errors']); // Non-zero = failure } // Watermark passed - discover highest passing level @@ -475,9 +493,17 @@ private function testLevel(string $binary, string $componentPath, int $level, ar // Generate temporary config file with auto-detected paths $tempConfig = $this->generateTempConfig($componentPath, $level); - // Get horde-components rules bootstrap - $componentsRoot = __DIR__ . '/../../..'; - $componentsBootstrap = realpath($componentsRoot . '/phpstan-bootstrap.php'); + // Materialize the horde-components custom-rules bootstrap on disk. + // When running from a phar, the bootstrap and rule files have to be + // copied out before we hand the path to a separate phpstan process. + $extractor = new PhpStanRuleExtractor(); + $extractCacheDir = $options['tools_dir'] ?? sys_get_temp_dir() . '/horde-components-phpstan'; + try { + $componentsBootstrap = $extractor->extract($extractCacheDir); + } catch (Throwable $e) { + $this->getOutput()->warn('PHPStan rule extraction failed: ' . $e->getMessage()); + $componentsBootstrap = null; + } $cmd = [ escapeshellarg($binary), @@ -486,7 +512,7 @@ private function testLevel(string $binary, string $componentPath, int $level, ar ]; // Add autoload file for custom rules - if ($componentsBootstrap && file_exists($componentsBootstrap)) { + if ($componentsBootstrap !== null && file_exists($componentsBootstrap)) { $cmd[] = '--autoload-file=' . escapeshellarg($componentsBootstrap); } @@ -525,11 +551,17 @@ private function testLevel(string $binary, string $componentPath, int $level, ar } } + // `passed` reflects only that PHPStan exited cleanly. Code-regression + // detection (errors > 0 at the watermark level) is the caller's job. + // Conflating the two — old contract was `passed = (exit==0 && errors==0)` + // — caused tooling failures (e.g. autoload-file unreachable in phar + // context) to be reported as code regressions with "0 errors". return [ - 'passed' => ($exitCode === 0 && $errors === 0), + 'passed' => ($exitCode === 0), 'errors' => $errors, 'exit_code' => $exitCode, 'results' => $results, + 'raw_output' => $fullOutput, ]; } diff --git a/test/Unit/Qc/PhpStanRuleExtractorTest.php b/test/Unit/Qc/PhpStanRuleExtractorTest.php new file mode 100644 index 00000000..12f732dd --- /dev/null +++ b/test/Unit/Qc/PhpStanRuleExtractorTest.php @@ -0,0 +1,87 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Qc; + +use Horde\Components\Qc\PhpStanRuleExtractor; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +/** + * Tests for PhpStanRuleExtractor. + * + * Phar-context extraction is structurally awkward to test in unit tests + * (would require constructing an in-memory phar with the right layout) and + * is exercised by the Victim smoke loop instead. The unit tests cover the + * developer-checkout branch and the idempotency contract. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(PhpStanRuleExtractor::class)] +class PhpStanRuleExtractorTest extends TestCase +{ + public function testCheckoutShortCircuitsAndReturnsRepoBootstrap(): void + { + $extractor = new PhpStanRuleExtractor(); + $bootstrap = $extractor->extract(null); + + $this->assertFileExists($bootstrap); + $this->assertSame('phpstan-bootstrap.php', basename($bootstrap)); + // Repo-level bootstrap sits next to a src/ directory. + $this->assertDirectoryExists(dirname($bootstrap) . '/src/PhpStan/Rules'); + } + + public function testCheckoutIgnoresCacheDirArgument(): void + { + $extractor = new PhpStanRuleExtractor(); + $cacheDir = sys_get_temp_dir() . '/phpstan-extractor-test-' . bin2hex(random_bytes(4)); + + try { + $bootstrap = $extractor->extract($cacheDir); + + // We're in a developer checkout, so the cache dir should NOT + // have been touched. + $this->assertDirectoryDoesNotExist($cacheDir); + $this->assertFileExists($bootstrap); + } finally { + if (is_dir($cacheDir)) { + $this->recursiveRm($cacheDir); + } + } + } + + private function recursiveRm(string $dir): void + { + if (!is_dir($dir)) { + return; + } + foreach (scandir($dir) as $entry) { + if ($entry === '.' || $entry === '..') { + continue; + } + $path = $dir . '/' . $entry; + if (is_dir($path) && !is_link($path)) { + $this->recursiveRm($path); + } else { + @unlink($path); + } + } + @rmdir($dir); + } +} From abed3fb66378612232e45cea485c9be6106e1e84 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 21:27:34 +0200 Subject: [PATCH 6/8] feat(ci): report scanned vs error-bearing file counts separately --- src/Ci/Run/CheckReporter.php | 11 ++-- src/Ci/Run/ResultCollector.php | 7 ++- src/Ci/Run/RunCommand.php | 17 +++-- src/Qc/Task/Phpstan.php | 109 +++++++++++++++++++++++++++------ 4 files changed, 113 insertions(+), 31 deletions(-) diff --git a/src/Ci/Run/CheckReporter.php b/src/Ci/Run/CheckReporter.php index 8620154c..3141e6ec 100644 --- a/src/Ci/Run/CheckReporter.php +++ b/src/Ci/Run/CheckReporter.php @@ -251,18 +251,21 @@ private function formatPhpUnitOutput(array $stats, bool $success): array */ private function formatPhpStanOutput(array $stats, bool $success): array { - $filesAnalyzed = $stats['files_analyzed'] ?? 0; + $filesScanned = $stats['files_scanned'] ?? 0; + $filesWithErrors = $stats['files_with_errors'] ?? 0; $errors = $stats['errors'] ?? 0; if ($success) { return [ 'title' => "✅ No errors found", - 'summary' => "**Files analyzed**: {$filesAnalyzed}\n**Errors**: 0\n\n✨ Static analysis passed!", + 'summary' => "**Files scanned**: {$filesScanned}\n**Errors**: 0\n\n✨ Static analysis passed!", ]; } - $summary = "**Files analyzed**: {$filesAnalyzed}\n**Errors**: {$errors}\n\n"; - $summary .= "⚠️ PHPStan found issues that need attention."; + $summary = "**Files scanned**: {$filesScanned}\n" + . "**Files with errors**: {$filesWithErrors}\n" + . "**Errors**: {$errors}\n\n" + . "⚠️ PHPStan found issues that need attention."; return [ 'title' => "⚠️ {$errors} errors found", diff --git a/src/Ci/Run/ResultCollector.php b/src/Ci/Run/ResultCollector.php index 68ed1452..48fde410 100644 --- a/src/Ci/Run/ResultCollector.php +++ b/src/Ci/Run/ResultCollector.php @@ -272,8 +272,11 @@ private function formatStats(string $tool, array $stats): string case 'phpstan': $parts = []; - if (isset($stats['files_analyzed'])) { - $parts[] = "{$stats['files_analyzed']} files"; + if (isset($stats['files_scanned'])) { + $parts[] = "{$stats['files_scanned']} scanned"; + } + if (isset($stats['files_with_errors']) && $stats['files_with_errors'] > 0) { + $parts[] = "{$stats['files_with_errors']} with errors"; } if (isset($stats['errors'])) { $parts[] = "{$stats['errors']} errors"; diff --git a/src/Ci/Run/RunCommand.php b/src/Ci/Run/RunCommand.php index d4ba5a90..4d500cc1 100644 --- a/src/Ci/Run/RunCommand.php +++ b/src/Ci/Run/RunCommand.php @@ -442,13 +442,13 @@ private function generateDetailedMetrics(array $results): string if (!empty($phpstanStats)) { $md .= "#### PHPStan\n\n"; $totalErrors = $phpstanStats['errors'] ?? 0; - $filesAnalyzed = $phpstanStats['files_analyzed'] ?? 0; + $filesScanned = $phpstanStats['files_scanned'] ?? 0; $lanesRun = $phpstanStats['lanes_run'] ?? 0; if ($totalErrors === 0) { - $md .= "✅ **No errors found** in {$filesAnalyzed} files ({$lanesRun} lanes)\n\n"; + $md .= "✅ **No errors found** in {$filesScanned} files ({$lanesRun} lanes)\n\n"; } else { - $md .= "⚠️ **{$totalErrors} errors found** in {$filesAnalyzed} files ({$lanesRun} lanes)\n\n"; + $md .= "⚠️ **{$totalErrors} errors found** ({$filesScanned} files scanned, {$lanesRun} lanes)\n\n"; } } @@ -923,7 +923,8 @@ private function generateHtmlMetrics(array $results): string // PHPStan section if (!empty($phpstanStats)) { $totalErrors = $phpstanStats['errors'] ?? 0; - $filesAnalyzed = $phpstanStats['files_analyzed'] ?? 0; + $filesScanned = $phpstanStats['files_scanned'] ?? 0; + $filesWithErrors = $phpstanStats['files_with_errors'] ?? 0; $lanesRun = $phpstanStats['lanes_run'] ?? 0; $statusIcon = $totalErrors === 0 ? '✅' : '⚠️'; @@ -939,8 +940,12 @@ private function generateHtmlMetrics(array $results): string {$lanesRun}
- Files analyzed: - {$filesAnalyzed} + Files scanned: + {$filesScanned} +
+
+ Files with errors: + {$filesWithErrors}
Errors: diff --git a/src/Qc/Task/Phpstan.php b/src/Qc/Task/Phpstan.php index 3442be82..efae3c75 100644 --- a/src/Qc/Task/Phpstan.php +++ b/src/Qc/Task/Phpstan.php @@ -34,7 +34,8 @@ class Phpstan extends Base * Statistics collected during execution. */ private array $stats = [ - 'files_analyzed' => 0, + 'files_scanned' => 0, + 'files_with_errors' => 0, 'errors' => 0, 'file_errors' => 0, ]; @@ -116,7 +117,8 @@ public function run(array &$options = []): int // Reset statistics $this->stats = [ - 'files_analyzed' => 0, + 'files_scanned' => 0, + 'files_with_errors' => 0, 'errors' => 0, 'file_errors' => 0, ]; @@ -602,30 +604,82 @@ private function getPathsFromConfig(string $configPath): array } /** - * Generate temporary PHPStan config with auto-detected paths. + * Resolve which directories to analyze for a given component. * - * @param string $componentPath Path to component directory. - * @param int $level PHPStan level to enforce. + * Mirrors {@see generateTempConfig()} so a separate scanned-file + * count can be computed without re-parsing the NEON. * - * @return string Path to temporary config file. + * @param string $componentPath Path to component directory. + * @return array Absolute directory paths. */ - private function generateTempConfig(string $componentPath, int $level): string + private function resolveAnalysisPaths(string $componentPath): array { - // Auto-detect paths to analyze (use absolute paths) $paths = []; - $candidates = ['src', 'migration']; - - foreach ($candidates as $candidate) { + foreach (['src', 'migration'] as $candidate) { $path = $componentPath . '/' . $candidate; if (is_dir($path)) { - $paths[] = " - " . $path; + $paths[] = $path; } } - - // If no standard paths found, analyze entire component if (empty($paths)) { - $paths[] = " - " . $componentPath; + $paths[] = $componentPath; + } + return $paths; + } + + /** + * Count `.php` files under a list of directories, excluding vendor/ + * and build/ to match the NEON excludePaths. + * + * @param array $paths Absolute directory paths. + * @return int Count of `.php` files reachable from those paths. + */ + private function countPhpFiles(array $paths): int + { + $count = 0; + foreach ($paths as $root) { + if (!is_dir($root)) { + if (is_file($root) && str_ends_with($root, '.php')) { + $count++; + } + continue; + } + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator( + $root, + \FilesystemIterator::SKIP_DOTS + ) + ); + foreach ($iterator as $file) { + $path = (string) $file; + if (!str_ends_with($path, '.php')) { + continue; + } + // Match the NEON excludePaths: anything under vendor/ or build/. + if (str_contains($path, '/vendor/') || str_contains($path, '/build/')) { + continue; + } + $count++; + } } + return $count; + } + + /** + * Generate temporary PHPStan config with auto-detected paths. + * + * @param string $componentPath Path to component directory. + * @param int $level PHPStan level to enforce. + * + * @return string Path to temporary config file. + */ + private function generateTempConfig(string $componentPath, int $level): string + { + // Auto-detect paths to analyze (use absolute paths) + $paths = array_map( + static fn (string $p): string => " - " . $p, + $this->resolveAnalysisPaths($componentPath) + ); $pathsYaml = implode("\n", $paths); @@ -764,9 +818,18 @@ private function parseResults(): void } } - // Count files analyzed + // PHPStan's JSON `files` map only lists files that produced errors, + // not every file scanned. The total scanned count is computed + // separately via filesystem traversal of the configured paths. if (isset($this->nativeResults['files'])) { - $this->stats['files_analyzed'] = count($this->nativeResults['files']); + $this->stats['files_with_errors'] = count($this->nativeResults['files']); + } + + $componentPath = $this->getPath() ?: getcwd(); + if (is_string($componentPath) && $componentPath !== '') { + $this->stats['files_scanned'] = $this->countPhpFiles( + $this->resolveAnalysisPaths($componentPath) + ); } } @@ -835,8 +898,16 @@ private function outputStatistics(): void { $parts = []; - if ($this->stats['files_analyzed'] > 0) { - $parts[] = $this->stats['files_analyzed'] . ' file' . ($this->stats['files_analyzed'] !== 1 ? 's' : '') . ' analyzed'; + if ($this->stats['files_scanned'] > 0) { + $parts[] = $this->stats['files_scanned'] + . ' file' . ($this->stats['files_scanned'] !== 1 ? 's' : '') + . ' scanned'; + } + + if ($this->stats['files_with_errors'] > 0) { + $parts[] = $this->stats['files_with_errors'] + . ' file' . ($this->stats['files_with_errors'] !== 1 ? 's' : '') + . ' with errors'; } if ($this->stats['errors'] > 0) { From b28a4c6ab6f5b29479e88c08d3b0a1450ee29a24 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Thu, 18 Jun 2026 22:05:13 +0200 Subject: [PATCH 7/8] feat(ci): honor PHP-CS-Fixer dry-run exit codes and exclude build/ --- .php-cs-fixer.dist.php | 2 +- src/Qc/Task/Phpcsfixer.php | 68 +++++++++++- test/Unit/Qc/Task/PhpcsfixerTest.php | 160 +++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 6 deletions(-) create mode 100644 test/Unit/Qc/Task/PhpcsfixerTest.php diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php index 8275523c..f0e422cd 100644 --- a/.php-cs-fixer.dist.php +++ b/.php-cs-fixer.dist.php @@ -21,7 +21,7 @@ } } -$finder->exclude(['fixtures']); +$finder->exclude(['fixtures', 'build', 'vendor']); $config = (new PhpCsFixer\Config()) ->setRules([ diff --git a/src/Qc/Task/Phpcsfixer.php b/src/Qc/Task/Phpcsfixer.php index 7a0c7f1d..b1001c4a 100644 --- a/src/Qc/Task/Phpcsfixer.php +++ b/src/Qc/Task/Phpcsfixer.php @@ -210,11 +210,19 @@ private function executePhpCsFixer(string $binary, string $componentPath, bool $ // First, get total file count using list-files $this->stats['files_checked'] = $this->getTotalFileCount($binary, $componentPath, $configPath); - // Build command + // Build command — invoke from inside the component dir so relative + // config paths resolve correctly. We deliberately do NOT pass a + // positional path argument: passing one would override the config's + // Finder includes/excludes (PHP-CS-Fixer treats positional paths as + // an alternative to the Finder, not an intersection by default), and + // would pull in transient directories such as build/ that the config + // explicitly excludes. $cmd = [ + 'cd', + escapeshellarg($componentPath), + '&&', escapeshellarg($binary), 'fix', - escapeshellarg($componentPath), ]; if ($isDryRun) { @@ -378,10 +386,21 @@ private function parseResults(): void return; } - // Count files with issues (files_checked is already set in executePhpCsFixer) + // PHP-CS-Fixer's JSON output for `--dry-run` lists files in `files` + // by name only (no `appliedFixers` key) — they are files that + // *would* be modified. The fix-mode JSON adds `appliedFixers` per + // file. Count both shapes as files_with_issues; only fix-mode + // entries are counted as files_fixed. foreach ($this->nativeResults['files'] as $file) { - if (isset($file['appliedFixers']) && is_array($file['appliedFixers']) && count($file['appliedFixers']) > 0) { + $isModified = isset($file['appliedFixers']) + && is_array($file['appliedFixers']) + && count($file['appliedFixers']) > 0; + $isDryRunHit = !isset($file['appliedFixers']) && isset($file['name']); + + if ($isModified || $isDryRunHit) { $this->stats['files_with_issues']++; + } + if ($isModified) { $this->stats['files_fixed']++; } } @@ -431,7 +450,8 @@ private function writeJsonResults(string $componentPath, int $exitCode, bool $is 'tool_source' => $binary ?: 'unknown', 'mode' => $isDryRun ? 'check' : 'fix', 'exit_code' => $exitCode, - 'success' => ($exitCode === 0), + 'success' => $this->isSuccessExitCode($exitCode, $isDryRun), + 'tooling_error' => $this->isToolingErrorExitCode($exitCode), 'statistics' => $this->stats, ]; @@ -760,4 +780,42 @@ private function recursiveRemoveDirectory(string $dir): bool return rmdir($dir); } + + /** + * Decide whether a PHP-CS-Fixer exit code means "style is clean". + * + * PHP-CS-Fixer 3.x exit codes (combinable bit flags): + * 0 OK + * 1 General error + * 4 Some files have invalid syntax + * 8 In dry-run/check mode: some files would be modified + * 16 Configuration error + * 32 Configuration of a fixer is invalid + * 64 Exception raised within the application + * + * For CI gating "is style clean": + * exit 0 → success + * exit 8 in dry-run → real style issues → failure + * anything else → failure (covers tooling errors and unknowns) + * + * Note that exit code 1 is reserved by PHP-CS-Fixer for general errors; + * we treat it as a non-success too because we cannot tell otherwise. + */ + private function isSuccessExitCode(int $exitCode, bool $isDryRun): bool + { + return $exitCode === 0; + } + + /** + * Whether an exit code indicates a tooling failure (as opposed to + * a real style finding). Used by reporters to distinguish "code has + * style issues" from "the fixer itself blew up". + * + * Tooling errors per PHP-CS-Fixer 3.x: 16 (config), 32 (fixer config), + * 64 (uncaught exception). Bits may be combined with other flags. + */ + private function isToolingErrorExitCode(int $exitCode): bool + { + return ($exitCode & (16 | 32 | 64)) !== 0; + } } diff --git a/test/Unit/Qc/Task/PhpcsfixerTest.php b/test/Unit/Qc/Task/PhpcsfixerTest.php new file mode 100644 index 00000000..7c8afe8d --- /dev/null +++ b/test/Unit/Qc/Task/PhpcsfixerTest.php @@ -0,0 +1,160 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Qc\Task; + +use Horde\Components\Output; +use Horde\Components\Qc\Task\Phpcsfixer; +use Horde\Components\Qc\Tasks as QcTasks; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; +use ReflectionClass; + +/** + * Tests for the PHP-CS-Fixer task — exit-code classifier and dry-run + * JSON parser. + * + * The task as a whole spawns php-cs-fixer as a subprocess, which is hard + * to drive in unit tests. These tests target the pure-data-shape methods + * via reflection. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(Phpcsfixer::class)] +class PhpcsfixerTest extends TestCase +{ + public function testExitCodeZeroIsSuccess(): void + { + $this->assertTrue($this->callIsSuccess(0, true)); + $this->assertTrue($this->callIsSuccess(0, false)); + } + + public function testExitCodeEightIsFailure(): void + { + // 8 = "would be modified" in dry-run; treated as failure for CI gating. + $this->assertFalse($this->callIsSuccess(8, true)); + } + + public function testToolingErrorCodes(): void + { + $this->assertTrue($this->callIsToolingError(16)); + $this->assertTrue($this->callIsToolingError(32)); + $this->assertTrue($this->callIsToolingError(64)); + $this->assertTrue($this->callIsToolingError(16 | 8)); // combined flags + } + + public function testNonToolingErrorCodes(): void + { + $this->assertFalse($this->callIsToolingError(0)); + $this->assertFalse($this->callIsToolingError(1)); + $this->assertFalse($this->callIsToolingError(4)); + $this->assertFalse($this->callIsToolingError(8)); + } + + public function testParseDryRunCountsNameOnlyEntriesAsIssues(): void + { + $task = $this->makeTask(); + $native = [ + 'files' => [ + ['name' => 'src/Foo.php'], + ['name' => 'src/Bar.php'], + ['name' => 'src/Baz.php'], + ], + ]; + + $stats = $this->callParse($task, $native); + + $this->assertSame(3, $stats['files_with_issues']); + $this->assertSame(0, $stats['files_fixed']); // no actual fixing in dry-run + } + + public function testParseFixModeCountsAppliedFixers(): void + { + $task = $this->makeTask(); + $native = [ + 'files' => [ + ['name' => 'src/Foo.php', 'appliedFixers' => ['braces']], + ['name' => 'src/Bar.php', 'appliedFixers' => ['indentation_type', 'no_trailing_whitespace']], + // entry without appliedFixers in fix mode shouldn't happen, but + // count it as an issue too (lenient parsing) + ['name' => 'src/Baz.php'], + ], + ]; + + $stats = $this->callParse($task, $native); + + $this->assertSame(3, $stats['files_with_issues']); + $this->assertSame(2, $stats['files_fixed']); // only entries with appliedFixers + } + + private function callIsSuccess(int $exitCode, bool $isDryRun): bool + { + $task = $this->makeTask(); + $reflection = new ReflectionClass($task); + $method = $reflection->getMethod('isSuccessExitCode'); + $method->setAccessible(true); + return $method->invoke($task, $exitCode, $isDryRun); + } + + private function callIsToolingError(int $exitCode): bool + { + $task = $this->makeTask(); + $reflection = new ReflectionClass($task); + $method = $reflection->getMethod('isToolingErrorExitCode'); + $method->setAccessible(true); + return $method->invoke($task, $exitCode); + } + + /** + * @param array $native + * @return array + */ + private function callParse(Phpcsfixer $task, array $native): array + { + $reflection = new ReflectionClass($task); + + $natProp = $reflection->getProperty('nativeResults'); + $natProp->setAccessible(true); + $natProp->setValue($task, $native); + + $statsProp = $reflection->getProperty('stats'); + $statsProp->setAccessible(true); + $statsProp->setValue($task, [ + 'files_checked' => 0, + 'files_with_issues' => 0, + 'files_fixed' => 0, + 'files_invalid' => 0, + 'files_skipped' => 0, + ]); + + $method = $reflection->getMethod('parseResults'); + $method->setAccessible(true); + $method->invoke($task); + + return $statsProp->getValue($task); + } + + private function makeTask(): Phpcsfixer + { + return new Phpcsfixer( + $this->createMock(QcTasks::class), + $this->createMock(Output::class) + ); + } +} From ce99d012ad129046b5a0f11b5ac512384e09d1ad Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Fri, 19 Jun 2026 07:58:08 +0200 Subject: [PATCH 8/8] feat(ci): emit GitHub Actions annotations for PHPStan and PHP-CS-Fixer findings --- data/ci/bootstrap-github.sh.template | 12 +- data/ci/workflow.yml.template | 7 +- src/Ci/Config/CiConfig.php | 14 +- src/Ci/GitHubAnnotations.php | 173 +++++++++++ src/Ci/Run/CheckReporter.php | 51 ++-- src/Ci/Run/FindingsAggregator.php | 192 ++++++++++++ src/Ci/Run/PrCommentReporter.php | 313 ++++++++++++++++---- src/Ci/Run/RunCommand.php | 155 ++++++++-- src/Ci/Setup/LaneScriptGenerator.php | 8 +- src/Ci/Template/TemplateRenderer.php | 2 +- src/Module/Ci.php | 13 +- src/Module/Qc.php | 18 +- src/Output/Presenter/Ci.php | 36 ++- src/Qc/Task/Phpcsfixer.php | 49 +++ src/Qc/Task/Phpstan.php | 78 ++++- src/Qc/Task/Unit.php | 58 +++- src/Qc/Tasks.php | 9 +- src/Runner/Qc.php | 8 +- test/Unit/Ci/GitHubAnnotationsTest.php | 196 ++++++++++++ test/Unit/Ci/Run/FindingsAggregatorTest.php | 234 +++++++++++++++ test/Unit/Output/Presenter/CiTest.php | 24 +- test/Unit/Qc/Task/UnitJunitParserTest.php | 104 +++++++ 22 files changed, 1589 insertions(+), 165 deletions(-) create mode 100644 src/Ci/GitHubAnnotations.php create mode 100644 src/Ci/Run/FindingsAggregator.php create mode 100644 test/Unit/Ci/GitHubAnnotationsTest.php create mode 100644 test/Unit/Ci/Run/FindingsAggregatorTest.php create mode 100644 test/Unit/Qc/Task/UnitJunitParserTest.php diff --git a/data/ci/bootstrap-github.sh.template b/data/ci/bootstrap-github.sh.template index a44ad140..a9090cdc 100644 --- a/data/ci/bootstrap-github.sh.template +++ b/data/ci/bootstrap-github.sh.template @@ -25,11 +25,13 @@ NC='' # Logging functions log_info() { - if [ -n "$GITHUB_ACTIONS" ]; then - echo "::notice::$*" - else - echo "[INFO] $*" - fi + # Bootstrap-stage progress lines stay as plain stdout under + # GitHub Actions. Routing them through ::notice:: would flood the + # Annotations panel with non-actionable entries. log_warn and + # log_error still emit workflow commands because bootstrap-level + # problems (missing GITHUB_TOKEN, download failure, etc.) belong + # in the panel. + echo "[INFO] $*" } log_warn() { diff --git a/data/ci/workflow.yml.template b/data/ci/workflow.yml.template index 72748a79..62df4885 100644 --- a/data/ci/workflow.yml.template +++ b/data/ci/workflow.yml.template @@ -18,7 +18,12 @@ on: jobs: ci: + name: CI runs-on: ubuntu-24.04 + permissions: + contents: read + pull-requests: write + checks: write steps: - name: Checkout code @@ -46,7 +51,7 @@ jobs: if: always() uses: actions/upload-artifact@v5 with: - name: ci-results-${{ github.run_number }} + name: ci-results-pr${{ github.event.pull_request.number || github.run_number }}-${{ github.sha }} path: | {{WORK_DIR}}/lanes/*/{{COMPONENT_NAME}}/build/*.json retention-days: 30 diff --git a/src/Ci/Config/CiConfig.php b/src/Ci/Config/CiConfig.php index 4c3fca32..586d2be5 100644 --- a/src/Ci/Config/CiConfig.php +++ b/src/Ci/Config/CiConfig.php @@ -159,18 +159,26 @@ public function getTestLanes(): array $testablePhpVersions = $this->getTestablePhpVersions(); foreach ($testablePhpVersions as $phpVersion) { - // Dev lane + // Dev lane — minimum-stability=dev so transitive deps may + // resolve to dev branches. $lanes[] = [ 'php' => $phpVersion, 'stability' => 'dev', 'dir' => $this->workDir . '/lanes/php' . $phpVersion . '-dev/' . $this->componentName, ]; - // Stable lane (or component's own stability) + // Second lane uses the component's own declared stability + // (alpha, beta, RC, stable). The directory name carries the + // same string so downstream readers (RunCommand::discoverLanes, + // log labels, artifact globs) all see one consistent label. + // Previously this was hardcoded to "-stable" while the + // stability field carried the component's value, causing the + // same lane to appear as both "php8.5-stable" and + // "php8.5-alpha" in different log lines. $lanes[] = [ 'php' => $phpVersion, 'stability' => $this->componentStability, - 'dir' => $this->workDir . '/lanes/php' . $phpVersion . '-stable/' . $this->componentName, + 'dir' => $this->workDir . '/lanes/php' . $phpVersion . '-' . $this->componentStability . '/' . $this->componentName, ]; } diff --git a/src/Ci/GitHubAnnotations.php b/src/Ci/GitHubAnnotations.php new file mode 100644 index 00000000..8103eb9c --- /dev/null +++ b/src/Ci/GitHubAnnotations.php @@ -0,0 +1,173 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Ci; + +/** + * Emit GitHub Actions workflow-command annotations on stdout. + * + * The runner picks up lines of the form + * + * ::error file=path,line=N,title=T::message text + * ::warning file=path,line=N::message text + * ::notice file=path::message text + * + * and surfaces them as line-anchored annotations on the PR diff plus + * entries in the run's "Annotations" sidebar. See + * https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions + * + * Path values must be **relative to `$GITHUB_WORKSPACE`** for the runner + * to anchor an annotation to a diff line. Absolute paths from the lane + * directory (`/tmp/horde-ci/lanes/.../Victim/src/X.php`) won't anchor; + * use {@see relativizeForAnnotation()} to strip the lane prefix. + * + * Outside GitHub Actions (no `GITHUB_ACTIONS=true` env var) every + * emit method is a silent no-op so local invocations don't print + * meaningless `::error::` lines. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +class GitHubAnnotations +{ + /** + * Emit a workflow-command line at error severity. + * + * @param string $message Plain message body. Newlines and `%` are + * escaped to GitHub's wire format. + * @param string|null $file Path relative to $GITHUB_WORKSPACE, or null + * to emit a sidebar-only annotation. + * @param int|null $line 1-based line number, or null. + * @param string|null $title Short title shown above the message. + */ + public static function error( + string $message, + ?string $file = null, + ?int $line = null, + ?string $title = null + ): void { + self::emit('error', $message, $file, $line, $title); + } + + public static function warning( + string $message, + ?string $file = null, + ?int $line = null, + ?string $title = null + ): void { + self::emit('warning', $message, $file, $line, $title); + } + + public static function notice( + string $message, + ?string $file = null, + ?int $line = null, + ?string $title = null + ): void { + self::emit('notice', $message, $file, $line, $title); + } + + /** + * Strip a lane/component prefix from an absolute path so the runner + * can anchor the annotation to the PR diff. + * + * Example: a finding for + * /tmp/horde-ci/lanes/php8.3-dev/Victim/src/ComposerJsonFile.php + * with componentDir + * /tmp/horde-ci/lanes/php8.3-dev/Victim + * returns "src/ComposerJsonFile.php". + * + * If the absolute path is not under componentDir, returns it unchanged. + */ + public static function relativizeForAnnotation( + string $absolute, + string $componentDir + ): string { + $componentDir = rtrim($componentDir, '/') . '/'; + if (str_starts_with($absolute, $componentDir)) { + return substr($absolute, strlen($componentDir)); + } + return $absolute; + } + + /** + * Whether annotations will actually be emitted in the current + * environment. Useful for callers that want to skip building the + * data when no one is listening. + */ + public static function isActive(): bool + { + return getenv('GITHUB_ACTIONS') === 'true'; + } + + private static function emit( + string $severity, + string $message, + ?string $file, + ?int $line, + ?string $title + ): void { + if (!self::isActive()) { + return; + } + + $params = []; + if ($file !== null && $file !== '') { + $params[] = 'file=' . self::escapeProperty($file); + } + if ($line !== null && $line > 0) { + $params[] = 'line=' . $line; + } + if ($title !== null && $title !== '') { + $params[] = 'title=' . self::escapeProperty($title); + } + + $paramStr = $params === [] ? '' : ' ' . implode(',', $params); + echo '::' . $severity . $paramStr . '::' . self::escapeData($message) . PHP_EOL; + } + + /** + * Escape per GitHub's workflow-command spec for the message body. + * + * Newlines have to be escaped as %0A or the runner will treat + * the next line as a new command (or, worse, swallow it). + */ + private static function escapeData(string $value): string + { + return strtr($value, [ + '%' => '%25', + "\r" => '%0D', + "\n" => '%0A', + ]); + } + + /** + * Escape for property values (file=, line=, title=). Property values + * also need `:` and `,` escaped because those are field separators. + */ + private static function escapeProperty(string $value): string + { + return strtr($value, [ + '%' => '%25', + "\r" => '%0D', + "\n" => '%0A', + ':' => '%3A', + ',' => '%2C', + ]); + } +} diff --git a/src/Ci/Run/CheckReporter.php b/src/Ci/Run/CheckReporter.php index 3141e6ec..46f06efb 100644 --- a/src/Ci/Run/CheckReporter.php +++ b/src/Ci/Run/CheckReporter.php @@ -17,7 +17,9 @@ namespace Horde\Components\Ci\Run; use Horde\Components\Output; -use Horde\GithubApiClient\GithubClient; +use Horde\GithubApiClient\CreateCheckRunParams; +use Horde\GithubApiClient\GithubApiClient; +use Horde\GithubApiClient\GithubRepository; use Exception; /** @@ -37,11 +39,11 @@ class CheckReporter * Constructor. * * @param Output $output Output handler - * @param GithubClient $apiClient GitHub API client + * @param GithubApiClient $apiClient GitHub API client */ public function __construct( private readonly Output $output, - private readonly GithubClient $apiClient + private readonly GithubApiClient $apiClient ) {} /** @@ -62,6 +64,11 @@ public function reportLaneResults( ): void { $this->output->info('Creating GitHub Check Runs...'); + // The typed API takes a GithubRepository value object. The + // description and clone-URL fields aren't used by the check-runs + // endpoint; pass empty strings. + $repository = new GithubRepository($repo, "{$owner}/{$repo}", '', ''); + $checkCount = 0; foreach ($results as $laneName => $tools) { @@ -72,8 +79,7 @@ public function reportLaneResults( } $this->createCheckRun( - owner: $owner, - repo: $repo, + repository: $repository, sha: $sha, laneName: $laneName, toolName: $toolName, @@ -88,18 +94,16 @@ public function reportLaneResults( } /** - * Create a single check run. + * Create a single check run via the GithubApiClient typed surface. * - * @param string $owner Repository owner - * @param string $repo Repository name - * @param string $sha Commit SHA - * @param string $laneName Lane name (e.g., "php8.4-dev") - * @param string $toolName Tool name (e.g., "phpunit") - * @param array $result Tool result + * @param GithubRepository $repository Target repository. + * @param string $sha Commit SHA the check anchors on. + * @param string $laneName Lane name (e.g., "php8.4-dev"). + * @param string $toolName Tool name (e.g., "phpunit"). + * @param array $result Tool result from ResultCollector. */ private function createCheckRun( - string $owner, - string $repo, + GithubRepository $repository, string $sha, string $laneName, string $toolName, @@ -109,19 +113,16 @@ private function createCheckRun( $conclusion = $this->determineConclusion($result); $output = $this->formatOutput($toolName, $result); - $data = [ - 'name' => $checkName, - 'head_sha' => $sha, - 'status' => 'completed', - 'conclusion' => $conclusion, - 'output' => $output, - ]; + $params = new CreateCheckRunParams( + name: $checkName, + headSha: $sha, + status: 'completed', + conclusion: $conclusion, + output: $output, + ); try { - $this->apiClient->post( - "/repos/{$owner}/{$repo}/check-runs", - $data - ); + $this->apiClient->createCheckRun($repository, $params); } catch (Exception $e) { $this->output->warn("Failed to create check run '{$checkName}': " . $e->getMessage()); } diff --git a/src/Ci/Run/FindingsAggregator.php b/src/Ci/Run/FindingsAggregator.php new file mode 100644 index 00000000..2a1c1517 --- /dev/null +++ b/src/Ci/Run/FindingsAggregator.php @@ -0,0 +1,192 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Ci\Run; + +/** + * Aggregate per-tool findings across lanes, deduped by tool-specific keys. + * + * Each method takes a map `[laneName => /lane/build/dir]` and returns a flat + * list of findings. Identical findings hit by multiple lanes collapse into + * one entry, with a `lanes` array listing every lane that hit it. + * + * Dedup keys per the round-3 plan: + * + * - **PHPStan**: `(file, line, identifier)`. Identifier comes from PHPStan + * 2.x's per-message `identifier` field (e.g. `isset.property`). When + * identifier is missing the message text is used as the third component. + * - **PHP-CS-Fixer**: `(file)`. Dry-run output does not carry line numbers + * or per-fixer detail in JSON. + * - **PHPUnit**: not yet implemented; PHPUnit text/JSON output does not + * carry per-failure file+line in a uniform shape. Future: switch the + * lane wrapper to `--log-junit` and key on `(test_class, test_method)`. + * + * The aggregator reads files from disk; tasks must persist their native + * shape via `qc phpstan --dump-native` (writes phpstan-native.json) or + * `qc phpcsfixer` (already writes php-cs-fixer-native.json on every run). + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +class FindingsAggregator +{ + /** + * Collect deduplicated PHPStan findings across lanes. + * + * @param array $laneBuildDirs Map of laneName => build dir + * (the dir containing + * `phpstan-native.json`). + * @return array}> + */ + public function aggregatePhpStan(array $laneBuildDirs): array + { + $byKey = []; + + foreach ($laneBuildDirs as $laneName => $buildDir) { + $native = $this->loadJson($buildDir . '/phpstan-native.json'); + if (!is_array($native) || !isset($native['files']) || !is_array($native['files'])) { + continue; + } + + foreach ($native['files'] as $absolutePath => $fileEntry) { + if (!is_string($absolutePath) || !is_array($fileEntry)) { + continue; + } + $messages = $fileEntry['messages'] ?? []; + if (!is_array($messages)) { + continue; + } + $relPath = $this->stripLanePrefix((string) $absolutePath, (string) $laneName); + + foreach ($messages as $msg) { + if (!is_array($msg)) { + continue; + } + $message = (string) ($msg['message'] ?? ''); + $line = isset($msg['line']) && is_int($msg['line']) ? $msg['line'] : null; + $identifier = isset($msg['identifier']) && is_string($msg['identifier']) + ? $msg['identifier'] + : null; + + $key = $relPath . '|' . ($line ?? '') . '|' . ($identifier ?? $message); + + if (!isset($byKey[$key])) { + $byKey[$key] = [ + 'file' => $relPath, + 'line' => $line, + 'identifier' => $identifier, + 'message' => $message, + 'lanes' => [], + ]; + } + if (!in_array($laneName, $byKey[$key]['lanes'], true)) { + $byKey[$key]['lanes'][] = $laneName; + } + } + } + } + + $findings = array_values($byKey); + usort($findings, fn (array $a, array $b): int => + ($a['file'] <=> $b['file']) + ?: (($a['line'] ?? 0) <=> ($b['line'] ?? 0)) + ?: (($a['identifier'] ?? '') <=> ($b['identifier'] ?? '')) + ); + return $findings; + } + + /** + * Collect deduplicated PHP-CS-Fixer findings across lanes. + * + * The native PCF JSON in dry-run mode lists files in `files[].name` + * (no line numbers, no per-fixer detail). + * + * @param array $laneBuildDirs Map of laneName => build dir + * @return array}> + */ + public function aggregatePhpCsFixer(array $laneBuildDirs): array + { + $byKey = []; + + foreach ($laneBuildDirs as $laneName => $buildDir) { + $native = $this->loadJson($buildDir . '/php-cs-fixer-native.json'); + if (!is_array($native) || !isset($native['files']) || !is_array($native['files'])) { + continue; + } + + foreach ($native['files'] as $file) { + if (!is_array($file) || !isset($file['name']) || !is_string($file['name'])) { + continue; + } + $name = $file['name']; + $key = $name; + if (!isset($byKey[$key])) { + $byKey[$key] = [ + 'file' => $name, + 'lanes' => [], + ]; + } + if (!in_array($laneName, $byKey[$key]['lanes'], true)) { + $byKey[$key]['lanes'][] = $laneName; + } + } + } + + $findings = array_values($byKey); + usort($findings, fn (array $a, array $b): int => $a['file'] <=> $b['file']); + return $findings; + } + + /** + * Best-effort: strip a `/...///` prefix from an + * absolute path so the rendered table shows repo-relative paths. + */ + private function stripLanePrefix(string $absolute, string $laneName): string + { + // Expect paths like /tmp/horde-ci/lanes///src/Foo.php. + // Find the lane name segment and return the part after the next + // segment (the component dir). + $needle = '/' . $laneName . '/'; + $pos = strpos($absolute, $needle); + if ($pos === false) { + return $absolute; + } + $tail = substr($absolute, $pos + strlen($needle)); + $slash = strpos($tail, '/'); + if ($slash === false) { + return $tail; + } + return substr($tail, $slash + 1); + } + + /** + * @return array|null Decoded JSON, or null on any error. + */ + private function loadJson(string $path): ?array + { + if (!is_file($path)) { + return null; + } + $raw = file_get_contents($path); + if ($raw === false) { + return null; + } + $decoded = json_decode($raw, true); + return is_array($decoded) ? $decoded : null; + } +} diff --git a/src/Ci/Run/PrCommentReporter.php b/src/Ci/Run/PrCommentReporter.php index 4c2c07af..5b6636de 100644 --- a/src/Ci/Run/PrCommentReporter.php +++ b/src/Ci/Run/PrCommentReporter.php @@ -17,7 +17,9 @@ namespace Horde\Components\Ci\Run; use Horde\Components\Output; -use Horde\GithubApiClient\GithubClient; +use Horde\GithubApiClient\GithubApiClient; +use Horde\GithubApiClient\GithubComment; +use Horde\GithubApiClient\GithubRepository; use Exception; /** @@ -41,11 +43,11 @@ class PrCommentReporter * Constructor. * * @param Output $output Output handler - * @param GithubClient $apiClient GitHub API client + * @param GithubApiClient $apiClient GitHub API client */ public function __construct( private readonly Output $output, - private readonly GithubClient $apiClient + private readonly GithubApiClient $apiClient ) {} /** @@ -57,6 +59,12 @@ public function __construct( * @param array>> $results Results by lane and tool * @param array{passed: int, failed: int, total: int, failed_lanes: array} $summary Summary statistics * @param string $runUrl URL to the GitHub Actions run + * @param array>> $findingsByTool + * Per-tool deduplicated findings list produced by + * {@see \Horde\Components\Ci\Run\FindingsAggregator}. Keys are + * tool names ("phpstan", "phpcsfixer"). When empty (e.g. + * because the aggregator wasn't run), the comment falls back to + * summed counts. */ public function postComment( string $owner, @@ -64,29 +72,26 @@ public function postComment( int $prNumber, array $results, array $summary, - string $runUrl + string $runUrl, + array $findingsByTool = [] ): void { $this->output->info("Posting CI results comment to PR #{$prNumber}..."); - $markdown = $this->generateCommentMarkdown($results, $summary, $runUrl); + $markdown = $this->generateCommentMarkdown($results, $summary, $runUrl, $findingsByTool); + // The typed API takes a GithubRepository value object. Only owner + + // name are used by the comment endpoints; description and clone-URL + // can be empty. + $repository = new GithubRepository($repo, "{$owner}/{$repo}", '', ''); try { - // Find existing comment - $existingComment = $this->findExistingComment($owner, $repo, $prNumber); + // Find existing comment (idempotency marker lives in the body). + $existingComment = $this->findExistingComment($repository, $prNumber); if ($existingComment !== null) { - // Update existing comment - $this->apiClient->patch( - "/repos/{$owner}/{$repo}/issues/comments/{$existingComment['id']}", - ['body' => $markdown] - ); + $this->apiClient->updateComment($repository, $existingComment->id, $markdown); $this->output->ok("Updated existing PR comment"); } else { - // Create new comment - $this->apiClient->post( - "/repos/{$owner}/{$repo}/issues/{$prNumber}/comments", - ['body' => $markdown] - ); + $this->apiClient->createPullRequestComment($repository, $prNumber, $markdown); $this->output->ok("Created new PR comment"); } } catch (Exception $e) { @@ -95,27 +100,31 @@ public function postComment( } /** - * Find existing bot comment on PR. + * Find an existing horde-components CI comment on the PR. * - * @param string $owner Repository owner - * @param string $repo Repository name - * @param int $prNumber PR number - * @return array|null Comment data or null if not found + * Identifies a previous comment by the HTML marker we plant at the top + * of every CI comment body. Returns null when no such comment exists or + * the lookup fails. + * + * @param GithubRepository $repository Target repository. + * @param int $prNumber PR number. + * @return GithubComment|null */ - private function findExistingComment(string $owner, string $repo, int $prNumber): ?array + private function findExistingComment(GithubRepository $repository, int $prNumber): ?GithubComment { try { - $comments = $this->apiClient->get( - "/repos/{$owner}/{$repo}/issues/{$prNumber}/comments" - ); - + $comments = $this->apiClient->listPullRequestComments($repository, $prNumber); foreach ($comments as $comment) { - if (strpos($comment['body'] ?? '', self::COMMENT_MARKER) !== false) { + if (str_contains($comment->body, self::COMMENT_MARKER)) { return $comment; } } } catch (Exception $e) { - // Ignore errors finding existing comment + // Lookup failure isn't fatal; the worst case is a duplicate + // comment on the PR rather than a swallowed run. + $this->output->warn( + 'Could not list PR comments to find existing CI comment: ' . $e->getMessage() + ); } return null; @@ -132,18 +141,24 @@ private function findExistingComment(string $owner, string $repo, int $prNumber) private function generateCommentMarkdown( array $results, array $summary, - string $runUrl + string $runUrl, + array $findingsByTool = [] ): string { $md = self::COMMENT_MARKER . "\n"; $md .= "## 🔍 CI Results\n\n"; - // Overall status + // Overall status — the count line. if ($summary['failed'] === 0) { $md .= "**Overall**: ✅ All {$summary['total']} lanes passed\n\n"; } else { $md .= "**Overall**: ❌ {$summary['failed']}/{$summary['total']} lanes failed\n\n"; } + // TL;DR — the tool breakdown. Drops the lane count (already on + // Overall) and uses deduplicated finding counts where the + // aggregator provided them. + $md .= $this->generateTldr($results, $summary, $findingsByTool) . "\n\n"; + // Summary by PHP version $md .= "### Summary by PHP Version\n\n"; $md .= $this->generateVersionTable($results); @@ -151,7 +166,7 @@ private function generateCommentMarkdown( // Quality metrics $md .= "### Quality Metrics\n\n"; - $md .= $this->generateQualityMetrics($results); + $md .= $this->generateQualityMetrics($results, $findingsByTool); $md .= "\n"; // Failed lanes detail (if any) @@ -183,6 +198,127 @@ private function generateCommentMarkdown( return $md; } + /** + * Build the TL;DR line at the top of the PR comment. + * + * Drops the lane count (already on the Overall line above) and shows + * the per-tool breakdown. + * + * Green case: + * `**TL;DR:** ✅ All clean: 54 tests, 15 files scanned, 19 files styled.` + * + * Red case (with deduped findings): + * `**TL;DR:** ❌ Quality issues: PHPStan: 1 unique error in 6 lanes; + * PHP-CS-Fixer: 4 files.` + * + * Red case (no aggregator data, falls back to summed counts): + * `**TL;DR:** ❌ Quality issues: PHPStan: 6 errors.` + * + * @param array>> $results Results by lane and tool + * @param array{passed: int, failed: int, total: int, failed_lanes: array} $summary + * @param array>> $findingsByTool Deduped findings per tool, optional + * @return string Markdown line (no trailing newline). + */ + private function generateTldr(array $results, array $summary, array $findingsByTool = []): string + { + $phpunit = $this->aggregateToolStats($results, 'phpunit'); + $phpstan = $this->aggregateToolStats($results, 'phpstan'); + $pcf = $this->aggregateToolStats($results, 'phpcsfixer'); + + if ($summary['failed'] === 0) { + $parts = []; + // Tests/files are per-lane unit counts. Use the per-lane max so + // a 6-lane matrix doesn't read as `324 tests`. Lane count from + // either tool (whichever ran) frames the matrix size. + $phpunitLanes = $phpunit['lanes_run'] ?? 0; + $tests = $phpunit['tests_max'] ?? 0; + $assertions = $phpunit['assertions_max'] ?? 0; + if ($tests > 0) { + $testsFragment = $assertions > 0 + ? "{$tests} tests ({$assertions} assertions)" + : "{$tests} tests"; + $parts[] = $phpunitLanes > 1 + ? "{$testsFragment} in {$phpunitLanes} lanes" + : $testsFragment; + } + $scanned = $phpstan['files_scanned_max'] ?? 0; + if ($scanned > 0) { + $parts[] = "{$scanned} files scanned"; + } + $checked = $pcf['files_checked_max'] ?? 0; + if ($checked > 0) { + $parts[] = "{$checked} files styled"; + } + $detail = $parts === [] ? '' : ': ' . implode(', ', $parts); + return "**TL;DR:** ✅ All clean{$detail}."; + } + + $reasons = []; + $phpunitFailures = ($phpunit['failures'] ?? 0) + ($phpunit['errors'] ?? 0); + if ($phpunitFailures > 0) { + $reasons[] = "PHPUnit: {$phpunitFailures} test" + . ($phpunitFailures === 1 ? '' : 's') + . ' failed'; + } + + // PHPStan: prefer the deduped finding count (one row per distinct + // file/line/identifier) over the lanes-summed count. Without dedup + // a single bug repeated across 6 lanes reads as "6 errors", which + // overstates the work. + $phpstanFindings = $findingsByTool['phpstan'] ?? null; + if (is_array($phpstanFindings) && $phpstanFindings !== []) { + $unique = count($phpstanFindings); + $laneCount = count($this->collectLanes($phpstanFindings)); + $reasons[] = sprintf( + 'PHPStan: %d unique error%s in %d lane%s', + $unique, + $unique === 1 ? '' : 's', + $laneCount, + $laneCount === 1 ? '' : 's' + ); + } else { + $phpstanErrors = $phpstan['errors'] ?? 0; + if ($phpstanErrors > 0) { + $reasons[] = "PHPStan: {$phpstanErrors} error" + . ($phpstanErrors === 1 ? '' : 's'); + } + } + + // PHP-CS-Fixer: prefer the deduped file count too. + $pcfFindings = $findingsByTool['phpcsfixer'] ?? null; + if (is_array($pcfFindings) && $pcfFindings !== []) { + $uniqueFiles = count($pcfFindings); + $reasons[] = "PHP-CS-Fixer: {$uniqueFiles} file" + . ($uniqueFiles === 1 ? '' : 's'); + } else { + $pcfHits = $pcf['files_with_issues_max'] ?? 0; + if ($pcfHits > 0) { + $reasons[] = "PHP-CS-Fixer: {$pcfHits} file" + . ($pcfHits === 1 ? '' : 's'); + } + } + + $detail = $reasons === [] ? '' : ': ' . implode('; ', $reasons); + return "**TL;DR:** ❌ Quality issues{$detail}."; + } + + /** + * Count the unique lanes referenced by a list of deduped findings. + * + * @param array> $findings + * @return array + */ + private function collectLanes(array $findings): array + { + $lanes = []; + foreach ($findings as $finding) { + foreach ((array) ($finding['lanes'] ?? []) as $lane) { + $lanes[(string) $lane] = true; + } + } + return array_keys($lanes); + } + /** * Generate version summary table. * @@ -218,8 +354,8 @@ private function generateVersionTable(array $results): string $md .= sprintf( "| %s | %s | %s |\n", $version, - $stabilities['dev'] ?? '—', - $stabilities['alpha'] ?? '—' + $stabilities['dev'] ?? '-', + $stabilities['alpha'] ?? '-' ); } @@ -230,9 +366,10 @@ private function generateVersionTable(array $results): string * Generate quality metrics section. * * @param array>> $results Results by lane and tool + * @param array>> $findingsByTool Deduped findings per tool, optional * @return string Markdown content */ - private function generateQualityMetrics(array $results): string + private function generateQualityMetrics(array $results, array $findingsByTool = []): string { $md = ''; @@ -243,37 +380,76 @@ private function generateQualityMetrics(array $results): string // PHPUnit if (!empty($phpunitStats)) { - $tests = $phpunitStats['tests'] ?? 0; + $tests = $phpunitStats['tests_max'] ?? 0; + $assertions = $phpunitStats['assertions_max'] ?? 0; $failures = $phpunitStats['failures'] ?? 0; $errors = $phpunitStats['errors'] ?? 0; + $lanes = $phpunitStats['lanes_run'] ?? 0; if ($failures === 0 && $errors === 0) { - $md .= "- **PHPUnit**: {$tests} tests passed ✅\n"; + $testsFragment = $assertions > 0 + ? "{$tests} tests ({$assertions} assertions)" + : "{$tests} tests"; + $md .= $lanes > 1 + ? "- **PHPUnit**: {$testsFragment} passed in {$lanes} lanes ✅\n" + : "- **PHPUnit**: {$testsFragment} passed ✅\n"; } else { $md .= "- **PHPUnit**: {$failures} failures, {$errors} errors ❌\n"; } } - // PHPStan + // PHPStan: prefer deduped finding count when the aggregator + // provided it. "1 unique error in 6 lanes" reads more honestly + // than "6 errors found" when the same bug repeats across the matrix. if (!empty($phpstanStats)) { - $errors = $phpstanStats['errors'] ?? 0; - - if ($errors === 0) { - $md .= "- **PHPStan**: No errors found ✅\n"; + $phpstanFindings = $findingsByTool['phpstan'] ?? null; + if (is_array($phpstanFindings) && $phpstanFindings !== []) { + $unique = count($phpstanFindings); + $laneCount = count($this->collectLanes($phpstanFindings)); + $md .= sprintf( + "- **PHPStan**: %d unique error%s in %d lane%s ⚠️\n", + $unique, + $unique === 1 ? '' : 's', + $laneCount, + $laneCount === 1 ? '' : 's' + ); } else { - $md .= "- **PHPStan**: {$errors} errors found ⚠️\n"; + $errors = $phpstanStats['errors'] ?? 0; + if ($errors === 0) { + $md .= "- **PHPStan**: No errors found ✅\n"; + } else { + $md .= sprintf( + "- **PHPStan**: %d error%s found ⚠️\n", + $errors, + $errors === 1 ? '' : 's' + ); + } } } // PHP-CS-Fixer if (!empty($csFixerStats)) { - $filesChecked = $csFixerStats['files_checked'] ?? 0; - $filesWithIssues = $csFixerStats['files_with_issues'] ?? 0; - - if ($filesWithIssues === 0) { - $md .= "- **PHP-CS-Fixer**: {$filesChecked} files checked, no issues ✅\n"; + $filesChecked = $csFixerStats['files_checked_max'] ?? 0; + $pcfFindings = $findingsByTool['phpcsfixer'] ?? null; + if (is_array($pcfFindings) && $pcfFindings !== []) { + $uniqueFiles = count($pcfFindings); + $md .= sprintf( + "- **PHP-CS-Fixer**: %d unique file%s with issues (of %d checked) ⚠️\n", + $uniqueFiles, + $uniqueFiles === 1 ? '' : 's', + $filesChecked + ); } else { - $md .= "- **PHP-CS-Fixer**: {$filesWithIssues} files with issues ⚠️\n"; + $filesWithIssues = $csFixerStats['files_with_issues_max'] ?? 0; + if ($filesWithIssues === 0) { + $md .= "- **PHP-CS-Fixer**: {$filesChecked} files checked, no issues ✅\n"; + } else { + $md .= sprintf( + "- **PHP-CS-Fixer**: %d file%s with issues ⚠️\n", + $filesWithIssues, + $filesWithIssues === 1 ? '' : 's' + ); + } } } @@ -321,20 +497,24 @@ private function formatToolFailure(string $toolName, array $result): string return match ($toolName) { 'phpunit' => sprintf( - "%s: %d failures, %d errors", + '%s: %d failure%s, %d error%s', $toolDisplay, $stats['failures'] ?? 0, - $stats['errors'] ?? 0 + ($stats['failures'] ?? 0) === 1 ? '' : 's', + $stats['errors'] ?? 0, + ($stats['errors'] ?? 0) === 1 ? '' : 's' ), 'phpstan' => sprintf( - "%s: %d errors found", + '%s: %d error%s found', $toolDisplay, - $stats['errors'] ?? 0 + $stats['errors'] ?? 0, + ($stats['errors'] ?? 0) === 1 ? '' : 's' ), 'phpcsfixer' => sprintf( - "%s: %d files with issues", + '%s: %d file%s with issues', $toolDisplay, - $stats['files_with_issues'] ?? 0 + $stats['files_with_issues'] ?? 0, + ($stats['files_with_issues'] ?? 0) === 1 ? '' : 's' ), default => "{$toolDisplay}: Failed", }; @@ -349,7 +529,7 @@ private function formatToolFailure(string $toolName, array $result): string */ private function aggregateToolStats(array $results, string $tool): array { - $aggregated = []; + $aggregated = ['lanes_run' => 0]; foreach ($results as $laneName => $tools) { if (!isset($tools[$tool])) { @@ -363,11 +543,28 @@ private function aggregateToolStats(array $results, string $tool): array continue; } + $aggregated['lanes_run']++; $stats = $result['statistics'] ?? []; foreach ($stats as $key => $value) { - if (is_numeric($value)) { - $aggregated[$key] = ($aggregated[$key] ?? 0) + $value; + if (!is_numeric($value)) { + continue; } + // Two views per numeric stat: + // `$key` sum across lanes — meaningful for + // finding counts (failures, errors) + // that legitimately stack. + // `${key}_max` highest value seen on any single + // lane — meaningful for unit-of-work + // counts (tests, assertions, files + // scanned/checked) which are constant + // per lane and would otherwise be + // overstated by the lane count. + $aggregated[$key] = ($aggregated[$key] ?? 0) + $value; + $maxKey = $key . '_max'; + $aggregated[$maxKey] = max( + $aggregated[$maxKey] ?? 0, + $value + ); } } diff --git a/src/Ci/Run/RunCommand.php b/src/Ci/Run/RunCommand.php index 4d500cc1..e839074f 100644 --- a/src/Ci/Run/RunCommand.php +++ b/src/Ci/Run/RunCommand.php @@ -18,7 +18,7 @@ use Horde\Components\Exception; use Horde\Components\Output; -use Horde\GithubApiClient\GithubClient; +use Horde\GithubApiClient\GithubApiClient; use Horde\Http\Uri; /** @@ -38,16 +38,25 @@ class RunCommand * @param ResultCollector $collector Result collector * @param string $componentsPath Path to horde-components binary * @param string $workDir Work directory (for tools path) - * @param GithubClient|null $apiClient GitHub API client (optional) + * @param GithubApiClient|null $apiClient GitHub API client (optional) */ public function __construct( private readonly Output $output, private readonly ResultCollector $collector, private readonly string $componentsPath, private readonly string $workDir, - private readonly ?GithubClient $apiClient = null + private readonly ?GithubApiClient $apiClient = null ) {} + /** + * Map of lane name → build directory, populated by aggregateResults. + * Used downstream by generateDetailedMetrics to read per-lane native + * tool JSON for findings deduplication. + * + * @var array + */ + private array $laneBuildDirs = []; + /** * Execute tests across all lanes. * @@ -204,7 +213,16 @@ private function runLaneScript(array $lane): void // Log output foreach ($output as $line) { - $this->output->plain("[{$lane['name']}] {$line}"); + // GitHub Actions workflow commands must start the line for the + // runner to parse them. Lines like `::error file=...::msg` from + // a lane script have to pass through verbatim — prefixing with + // `[lane-name]` turns them into ordinary log output and breaks + // annotation rendering. + if (str_starts_with($line, '::')) { + $this->output->plain($line); + } else { + $this->output->plain("[{$lane['name']}] {$line}"); + } } // Report result @@ -227,6 +245,7 @@ private function aggregateResults(array $lanes): void foreach ($lanes as $lane) { $buildDir = $lane['component_dir'] . '/build'; + $this->laneBuildDirs[$lane['name']] = $buildDir; // Honour deliberate skips written by SetupCommand. $skipFile = $buildDir . '/skip.json'; @@ -364,7 +383,7 @@ private function isLanePassed(array $tools): bool private function formatToolForTable(?array $result): string { if ($result === null) { - return '—'; + return '-'; } // Deliberately skipped (lane incompatible with this tool) @@ -417,7 +436,9 @@ private function generateDetailedMetrics(array $results): string // PHPUnit section if (!empty($phpunitStats)) { $md .= "#### PHPUnit\n\n"; - $totalTests = $phpunitStats['tests'] ?? 0; + // tests is per-lane; show the max so a 6-lane matrix reads + // "54 tests in 6 lanes" rather than the summed "324". + $totalTests = $phpunitStats['tests_max'] ?? 0; $totalFailures = $phpunitStats['failures'] ?? 0; $totalErrors = $phpunitStats['errors'] ?? 0; $lanesRun = $phpunitStats['lanes_run'] ?? 0; @@ -428,7 +449,7 @@ private function generateDetailedMetrics(array $results): string $md .= "❌ **Tests failed**\n"; } - $md .= "- Total tests: {$totalTests}\n"; + $md .= "- Tests per lane: {$totalTests}\n"; if ($totalFailures > 0) { $md .= "- Failures: {$totalFailures}\n"; } @@ -442,32 +463,98 @@ private function generateDetailedMetrics(array $results): string if (!empty($phpstanStats)) { $md .= "#### PHPStan\n\n"; $totalErrors = $phpstanStats['errors'] ?? 0; - $filesScanned = $phpstanStats['files_scanned'] ?? 0; + // files_scanned is per-lane; max == the actual codebase size. + $filesScanned = $phpstanStats['files_scanned_max'] ?? 0; $lanesRun = $phpstanStats['lanes_run'] ?? 0; if ($totalErrors === 0) { $md .= "✅ **No errors found** in {$filesScanned} files ({$lanesRun} lanes)\n\n"; } else { $md .= "⚠️ **{$totalErrors} errors found** ({$filesScanned} files scanned, {$lanesRun} lanes)\n\n"; + $md .= $this->renderPhpStanFindingsTable(); } } // PHP-CS-Fixer section if (!empty($csFixerStats)) { $md .= "#### PHP-CS-Fixer\n\n"; - $filesChecked = $csFixerStats['files_checked'] ?? 0; - $filesWithIssues = $csFixerStats['files_with_issues'] ?? 0; + // files_checked/files_with_issues are per-lane; max == unique. + $filesChecked = $csFixerStats['files_checked_max'] ?? 0; + $filesWithIssues = $csFixerStats['files_with_issues_max'] ?? 0; if ($filesWithIssues === 0) { $md .= "✅ **No style issues** in {$filesChecked} files\n\n"; } else { $md .= "⚠️ **{$filesWithIssues} files** with style issues (of {$filesChecked} checked)\n\n"; + $md .= $this->renderPhpCsFixerFindingsTable(); } } return $md; } + /** + * Render a deduplicated PHPStan findings table. + * + * @return string Markdown + */ + private function renderPhpStanFindingsTable(): string + { + $aggregator = new FindingsAggregator(); + $findings = $aggregator->aggregatePhpStan($this->laneBuildDirs); + if ($findings === []) { + return ''; + } + + $md = "| File | Line | Identifier | Message | Lanes |\n"; + $md .= "|------|------|------------|---------|-------|\n"; + foreach ($findings as $f) { + $md .= sprintf( + "| %s | %s | %s | %s | %s |\n", + $this->escapeMd($f['file']), + $f['line'] !== null ? (string) $f['line'] : '-', + $f['identifier'] !== null ? '`' . $this->escapeMd($f['identifier']) . '`' : '-', + $this->escapeMd($f['message']), + $this->escapeMd(implode(', ', $f['lanes'])) + ); + } + return $md . "\n"; + } + + /** + * Render a deduplicated PHP-CS-Fixer findings table. + * + * @return string Markdown + */ + private function renderPhpCsFixerFindingsTable(): string + { + $aggregator = new FindingsAggregator(); + $findings = $aggregator->aggregatePhpCsFixer($this->laneBuildDirs); + if ($findings === []) { + return ''; + } + + $md = "| File | Lanes |\n"; + $md .= "|------|-------|\n"; + foreach ($findings as $f) { + $md .= sprintf( + "| %s | %s |\n", + $this->escapeMd($f['file']), + $this->escapeMd(implode(', ', $f['lanes'])) + ); + } + return $md . "\n"; + } + + /** + * Lightly escape a value for use in a markdown table cell. + */ + private function escapeMd(string $value): string + { + // Pipes and newlines would break the table layout. + return strtr($value, ['|' => '\\|', "\n" => ' ', "\r" => '']); + } + /** * Aggregate statistics for a specific tool across all lanes. * @@ -493,12 +580,22 @@ private function aggregateToolStats(array $results, string $tool): array $aggregated['lanes_run']++; - // Aggregate statistics $stats = $result['statistics'] ?? []; foreach ($stats as $key => $value) { - if (is_numeric($value)) { - $aggregated[$key] = ($aggregated[$key] ?? 0) + $value; + if (!is_numeric($value)) { + continue; } + // Two views per numeric stat: `$key` sum across lanes + // (meaningful for finding counts that stack), `${key}_max` + // highest seen on any single lane (meaningful for + // unit-of-work counts like tests/files which are + // per-lane and would be overstated by the lane count). + $aggregated[$key] = ($aggregated[$key] ?? 0) + $value; + $maxKey = $key . '_max'; + $aggregated[$maxKey] = max( + $aggregated[$maxKey] ?? 0, + $value + ); } } @@ -813,7 +910,7 @@ private function generateHtmlReport(): string private function formatToolForHtml(?array $result): string { if ($result === null) { - return '—'; + return '-'; } // Deliberately skipped (lane incompatible with this tool) @@ -868,11 +965,13 @@ private function generateHtmlMetrics(array $results): string // PHPUnit section if (!empty($phpunitStats)) { - $totalTests = $phpunitStats['tests'] ?? 0; + // Tests/assertions are per-lane; show the per-lane max so the + // HTML report doesn't multiply by the matrix size. + $totalTests = $phpunitStats['tests_max'] ?? 0; $totalFailures = $phpunitStats['failures'] ?? 0; $totalErrors = $phpunitStats['errors'] ?? 0; $lanesRun = $phpunitStats['lanes_run'] ?? 0; - $assertions = $phpunitStats['assertions'] ?? 0; + $assertions = $phpunitStats['assertions_max'] ?? 0; $statusIcon = ($totalFailures === 0 && $totalErrors === 0) ? '✅' : '❌'; $statusText = ($totalFailures === 0 && $totalErrors === 0) @@ -887,11 +986,11 @@ private function generateHtmlMetrics(array $results): string {$lanesRun}
- Total tests: + Tests per lane: {$totalTests}
- Total assertions: + Assertions per lane: {$assertions}
@@ -923,8 +1022,9 @@ private function generateHtmlMetrics(array $results): string // PHPStan section if (!empty($phpstanStats)) { $totalErrors = $phpstanStats['errors'] ?? 0; - $filesScanned = $phpstanStats['files_scanned'] ?? 0; - $filesWithErrors = $phpstanStats['files_with_errors'] ?? 0; + // Files scanned/with errors are per-lane; max == codebase. + $filesScanned = $phpstanStats['files_scanned_max'] ?? 0; + $filesWithErrors = $phpstanStats['files_with_errors_max'] ?? 0; $lanesRun = $phpstanStats['lanes_run'] ?? 0; $statusIcon = $totalErrors === 0 ? '✅' : '⚠️'; @@ -958,8 +1058,8 @@ private function generateHtmlMetrics(array $results): string // PHP-CS-Fixer section if (!empty($csFixerStats)) { - $filesChecked = $csFixerStats['files_checked'] ?? 0; - $filesWithIssues = $csFixerStats['files_with_issues'] ?? 0; + $filesChecked = $csFixerStats['files_checked_max'] ?? 0; + $filesWithIssues = $csFixerStats['files_with_issues_max'] ?? 0; $statusIcon = $filesWithIssues === 0 ? '✅' : '⚠️'; $statusText = $filesWithIssues === 0 @@ -1068,6 +1168,14 @@ private function postPrComment(): void ->withPath("/{$repo}/actions/runs/{$runId}") ->__toString(); + // Build deduped findings once so the comment can show "1 unique + // error in 6 lanes" rather than the lanes-summed "6 errors". + $aggregator = new FindingsAggregator(); + $findingsByTool = [ + 'phpstan' => $aggregator->aggregatePhpStan($this->laneBuildDirs), + 'phpcsfixer' => $aggregator->aggregatePhpCsFixer($this->laneBuildDirs), + ]; + $reporter = new PrCommentReporter($this->output, $this->apiClient); $reporter->postComment( owner: $owner, @@ -1075,7 +1183,8 @@ private function postPrComment(): void prNumber: (int) $prNumber, results: $this->collector->getResults(), summary: $this->collector->getSummary(), - runUrl: $runUrl + runUrl: $runUrl, + findingsByTool: $findingsByTool ); } } diff --git a/src/Ci/Setup/LaneScriptGenerator.php b/src/Ci/Setup/LaneScriptGenerator.php index 99019ed1..dfdf8a02 100644 --- a/src/Ci/Setup/LaneScriptGenerator.php +++ b/src/Ci/Setup/LaneScriptGenerator.php @@ -215,8 +215,10 @@ private function generatePhpStanSection(): string cd "$COMPONENT_DIR" "$PHP_BINARY" "$COMPONENTS_PATH" qc phpstan \ --tools-dir="$TOOLS_DIR" \ + --dump-native \ 2>&1 || PHPSTAN_EXIT=$? # JSON written to: $BUILD_DIR/phpstan-results.json + # Native JSON written to: $BUILD_DIR/phpstan-native.json (--dump-native) BASH; @@ -259,13 +261,13 @@ private function generateSummarySection(array $tasks): string // Add exit code display for each task if (in_array('phpunit', $tasks)) { - $summary .= 'echo "PHPUnit: ${PHPUNIT_EXIT:-0}"' . "\n"; + $summary .= 'echo "PHPUnit exit: ${PHPUNIT_EXIT:-0}"' . "\n"; } if (in_array('phpstan', $tasks)) { - $summary .= 'echo "PHPStan: ${PHPSTAN_EXIT:-0}"' . "\n"; + $summary .= 'echo "PHPStan exit: ${PHPSTAN_EXIT:-0}"' . "\n"; } if (in_array('phpcsfixer', $tasks)) { - $summary .= 'echo "PHP-CS-Fixer: ${PHPCS_EXIT:-0}"' . "\n"; + $summary .= 'echo "PHP-CS-Fixer exit: ${PHPCS_EXIT:-0}"' . "\n"; } $summary .= "\n# Exit with first non-zero exit code\n"; diff --git a/src/Ci/Template/TemplateRenderer.php b/src/Ci/Template/TemplateRenderer.php index d243ae5e..5bf382c4 100644 --- a/src/Ci/Template/TemplateRenderer.php +++ b/src/Ci/Template/TemplateRenderer.php @@ -38,7 +38,7 @@ class TemplateRenderer * This version is embedded in generated files and used to detect * when components are using outdated templates. */ - private const TEMPLATE_VERSION = '1.2.0'; + private const TEMPLATE_VERSION = '1.5.0'; /** * Constructor. diff --git a/src/Module/Ci.php b/src/Module/Ci.php index a3acf6f0..4db8c2ec 100644 --- a/src/Module/Ci.php +++ b/src/Module/Ci.php @@ -25,6 +25,7 @@ use Horde\Components\Ci\Setup\ExtensionInstaller; use Horde\Components\Ci\Setup\LaneCopier; use Horde\Components\Ci\Setup\ComposerInstaller; +use Horde\GithubApiClient\GithubApiClient; use Horde\Components\Ci\Setup\ToolCache; use Horde\Components\Ci\Setup\LaneScriptGenerator; use Horde\Components\Ci\Init\InitCommand; @@ -519,13 +520,17 @@ private function handleRun(array $options, Output $output): bool } } - // Get GitHub API client (if available) + // Get GitHub API client (if available). Resolved via the injector so + // GithubApiConfig (set up in Components::__construct from the + // GITHUB_TOKEN env / github.token config key) flows through. $apiClient = null; if (getenv('GITHUB_TOKEN') !== false) { try { - $apiClient = $this->dependencies->getGithubClient(); - } catch (\Exception $e) { - // GitHub client not available, continue without it + $apiClient = $this->dependencies->get(GithubApiClient::class); + } catch (\Throwable $e) { + // Don't silently swallow: a maintainer running into a real + // misconfiguration should see why the PR comment didn't post. + $output->warn('Could not initialize GitHub API client: ' . $e->getMessage()); } } diff --git a/src/Module/Qc.php b/src/Module/Qc.php index 36c57e07..2593f4f9 100644 --- a/src/Module/Qc.php +++ b/src/Module/Qc.php @@ -18,6 +18,7 @@ use Horde\Argv\Option; use Horde\Components\Component; use Horde\Components\Component\ComponentDirectory; +use Horde\Components\Exception; use Horde\Components\RuntimeContext\CurrentWorkingDirectory; use Horde\Components\Runner\Qc as RunnerQc; use Horde\Components\Output; @@ -74,6 +75,13 @@ public function getOptionGroupOptions(): array 'help' => 'Config file preference: "tool" (horde-components), "uut" (component being tested), or path to specific config. Applies to PHP CS Fixer and PHPStan.', ] ), + new Option( + '--dump-native', + [ + 'action' => 'store_true', + 'help' => 'Persist the underlying tool\'s native JSON next to the summary (e.g. phpstan-native.json). Used by the CI lane runner so per-finding aggregation can read findings from disk; off by default for local invocations.', + ] + ), ]; } @@ -325,7 +333,15 @@ public function handle(array $options, array $arguments, ?Component $component = $output, $qcTasks ); - $runner->run(); + $numErrors = $runner->run(); + if ($numErrors > 0) { + // Per the Module::handle() contract: bool=matched, exception=failure. + // The shell exit code is the exception's getCode() (W1). + throw new Exception( + "qc reported {$numErrors} error" . ($numErrors === 1 ? '' : 's'), + 1 + ); + } return true; } return false; diff --git a/src/Output/Presenter/Ci.php b/src/Output/Presenter/Ci.php index 83ea118f..d0eae1dd 100644 --- a/src/Output/Presenter/Ci.php +++ b/src/Output/Presenter/Ci.php @@ -25,10 +25,13 @@ * where colored output doesn't render well and special annotations * are preferred. * - * GitHub Actions format: - * ::notice::Success message - * ::warning::Warning message - * ::error::Error message + * GitHub Actions: log lines are written as plain text. Per-finding + * annotations are emitted through {@see \Horde\Components\Ci\GitHubAnnotations} + * which knows how to construct workflow commands with file=/line= so + * the resulting annotations anchor to the PR diff. Routing summary + * decoration (ok/warn/info/error) through the workflow-command + * channel as well floods the Annotations panel with duplicate entries + * that have no diff anchor — see F24 in the CI strategy doc. * * Plain CI format (GitLab, Jenkins, etc.): * [OK] Success message @@ -38,7 +41,6 @@ * * This format is optimized for: * - No ANSI color codes (clean logs) - * - GitHub Actions workflow commands for annotations * - Concise brackets without padding * - Easy parsing by CI tools * @@ -86,7 +88,12 @@ public function __construct($output = null) public function ok(string $message): void { if ($this->githubActions) { - $this->writeln("::notice::{$message}"); + // Under GitHub Actions, route log decoration through plain text. + // Findings reach the Annotations panel via the explicit + // {@see \Horde\Components\Ci\GitHubAnnotations} helper which + // populates file= and line= — bare ::-markup would flood the + // panel with duplicate-summary entries that have no diff anchor. + $this->writeln($message); } else { $this->writeln("[OK] {$message}"); } @@ -103,7 +110,7 @@ public function ok(string $message): void public function warn(string $message): void { if ($this->githubActions) { - $this->writeln("::warning::{$message}"); + $this->writeln($message); } else { $this->writeln("[WARN] {$message}"); } @@ -123,7 +130,7 @@ public function warn(string $message): void public function info(string $message): void { if ($this->githubActions) { - $this->writeln("::notice::{$message}"); + $this->writeln($message); } else { $this->writeln("[INFO] {$message}"); } @@ -140,7 +147,7 @@ public function info(string $message): void public function error(string $message): void { if ($this->githubActions) { - $this->writeln("::error::{$message}"); + $this->writeln($message); } else { $this->writeln("[ERROR] {$message}"); } @@ -238,13 +245,10 @@ public function semantic(string $category, string $message): void } if ($this->githubActions) { - // Map to GitHub Actions workflow commands - $command = match ($category) { - 'regression', 'validation' => '::error::', - 'improvement', 'auto', 'created' => '::notice::', - default => '::debug::', // Most semantic categories are debug-level - }; - $this->writeln("{$command}{$message}"); + // Same rationale as ok/warn/info/error above: plain text, + // no workflow-command markup. Findings are emitted as + // annotations explicitly via GitHubAnnotations elsewhere. + $this->writeln($message); } else { // Plain CI format with category labels $label = match ($category) { diff --git a/src/Qc/Task/Phpcsfixer.php b/src/Qc/Task/Phpcsfixer.php index b1001c4a..0a4f92b3 100644 --- a/src/Qc/Task/Phpcsfixer.php +++ b/src/Qc/Task/Phpcsfixer.php @@ -11,6 +11,7 @@ namespace Horde\Components\Qc\Task; +use Horde\Components\Ci\GitHubAnnotations; use Horde\Components\Qc\ToolFinder; use Phar; @@ -132,6 +133,11 @@ public function run(array &$options = []): int if ($this->nativeResults !== null) { $this->parseResults(); $this->writeJsonResults($componentPath, $exitCode, $isDryRun); + // Annotate dry-run hits on the PR diff. In fix mode the issues + // were just fixed, so annotating them as findings would be wrong. + if ($isDryRun) { + $this->emitPhpCsFixerAnnotations($componentPath); + } } $this->outputStatistics($isDryRun); @@ -818,4 +824,47 @@ private function isToolingErrorExitCode(int $exitCode): bool { return ($exitCode & (16 | 32 | 64)) !== 0; } + + /** + * Emit one GitHub Actions `::warning` annotation per file that + * php-cs-fixer flagged in dry-run mode. + * + * The native JSON in dry-run mode has shape: + * { files: [ { name: "" }, ... ] } + * Names are relative to the component path (cwd of the spawn). + * php-cs-fixer does NOT report line numbers in JSON output, so the + * annotation anchors at the file level (sidebar entry only — no + * diff-line marker). + * + * No-op outside GitHub Actions. + * + * @param string $componentPath Component root directory; used only to + * resolve any absolute paths back to + * repo-relative form. + */ + private function emitPhpCsFixerAnnotations(string $componentPath): void + { + if (!GitHubAnnotations::isActive()) { + return; + } + if (!isset($this->nativeResults['files']) || !is_array($this->nativeResults['files'])) { + return; + } + + foreach ($this->nativeResults['files'] as $file) { + if (!is_array($file) || !isset($file['name']) || !is_string($file['name'])) { + continue; + } + $name = $file['name']; + // The path may be absolute or repo-relative depending on + // how php-cs-fixer was invoked; both shapes are handled. + $relPath = GitHubAnnotations::relativizeForAnnotation($name, $componentPath); + GitHubAnnotations::warning( + "File would be reformatted by php-cs-fixer (run 'horde-components qc phpcsfixer --fix-qc-issues' to apply).", + $relPath, + null, + 'PHP-CS-Fixer' + ); + } + } } diff --git a/src/Qc/Task/Phpstan.php b/src/Qc/Task/Phpstan.php index efae3c75..48a5bbf0 100644 --- a/src/Qc/Task/Phpstan.php +++ b/src/Qc/Task/Phpstan.php @@ -11,6 +11,7 @@ namespace Horde\Components\Qc\Task; +use Horde\Components\Ci\GitHubAnnotations; use Horde\Components\Qc\PhpStanRuleExtractor; use Horde\Components\Qc\ToolFinder; use Throwable; @@ -136,6 +137,13 @@ public function run(array &$options = []): int . ($watermarkResult['errors'] !== 1 ? 's' : '') . ')' ); $this->getOutput()->warn('Watermark level MUST pass - fix these errors!'); + + // Annotate each finding on the PR diff for GitHub Actions. + $this->emitPhpStanAnnotations( + $watermarkResult['results'] ?? null, + $componentPath, + $watermark + ); } else { // TOOLING FAILURE - PHPStan refused to start (config error, // missing autoload-file, unloadable rule classes, etc.). @@ -157,7 +165,7 @@ public function run(array &$options = []): int $this->nativeResults = $watermarkResult['results']; $this->level = $watermark; $this->parseResults(); - $this->writeJsonResults($componentPath, $watermarkResult['exit_code'], $watermark); + $this->writeJsonResults($componentPath, $watermarkResult['exit_code'], $watermark, $options); $this->outputStatistics(); return max(1, $watermarkResult['errors']); // Non-zero = failure @@ -231,7 +239,7 @@ public function run(array &$options = []): int // Parse and output results if ($this->nativeResults !== null) { $this->parseResults(); - $this->writeJsonResults($componentPath, 0, $this->level); + $this->writeJsonResults($componentPath, 0, $this->level, $options); } $this->outputStatistics(); @@ -842,7 +850,7 @@ private function parseResults(): void * * @return void */ - private function writeJsonResults(string $componentPath, int $exitCode, int $level): void + private function writeJsonResults(string $componentPath, int $exitCode, int $level, array $options = []): void { $buildDir = $componentPath . '/build'; @@ -851,9 +859,11 @@ private function writeJsonResults(string $componentPath, int $exitCode, int $lev mkdir($buildDir, 0o755, true); } - // Write native PHPStan JSON + // Write native PHPStan JSON only when the caller explicitly asked + // for it. The CI lane runner sets --dump-native; local invocations + // get only the summary so a developer's working tree stays tidy. $nativeJsonPath = $buildDir . '/phpstan-native.json'; - if ($this->nativeResults !== null) { + if (!empty($options['dump_native']) && $this->nativeResults !== null) { $json = json_encode($this->nativeResults, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); file_put_contents($nativeJsonPath, $json); } @@ -924,4 +934,62 @@ private function outputStatistics(): void } } } + + /** + * Emit one GitHub Actions `::error` annotation per PHPStan finding. + * + * The native PHPStan JSON has shape: + * { files: { "": { errors: int, messages: [ + * { message, line, identifier, ignorable }, ... ] } } } + * + * Paths are absolute on the running system (e.g. inside a CI lane: + * /tmp/horde-ci/lanes/php8.3-dev/Victim/src/Foo.php). For the runner + * to anchor an annotation to a diff line, the path must be relative + * to $GITHUB_WORKSPACE. We strip the component root prefix to get + * the repo-relative path the diff uses. + * + * No-op outside GitHub Actions. + * + * @param array|null $native The decoded PHPStan JSON. + * @param string $componentPath Component root directory. + * @param int $level Watermark level (used for the annotation title). + */ + private function emitPhpStanAnnotations(?array $native, string $componentPath, int $level): void + { + if (!GitHubAnnotations::isActive()) { + return; + } + if (!is_array($native) || !isset($native['files']) || !is_array($native['files'])) { + return; + } + + foreach ($native['files'] as $absolutePath => $fileEntry) { + if (!is_string($absolutePath) || !is_array($fileEntry)) { + continue; + } + $relPath = GitHubAnnotations::relativizeForAnnotation($absolutePath, $componentPath); + $messages = $fileEntry['messages'] ?? []; + if (!is_array($messages)) { + continue; + } + foreach ($messages as $msg) { + if (!is_array($msg)) { + continue; + } + $text = (string) ($msg['message'] ?? ''); + $line = isset($msg['line']) && is_int($msg['line']) && $msg['line'] > 0 + ? $msg['line'] + : null; + $identifier = isset($msg['identifier']) && is_string($msg['identifier']) + ? ' [' . $msg['identifier'] . ']' + : ''; + GitHubAnnotations::error( + $text . $identifier, + $relPath, + $line, + 'PHPStan level ' . $level + ); + } + } + } } diff --git a/src/Qc/Task/Unit.php b/src/Qc/Task/Unit.php index 88ecf21a..020e8405 100644 --- a/src/Qc/Task/Unit.php +++ b/src/Qc/Task/Unit.php @@ -314,8 +314,10 @@ public function run(array &$options = []): int $argv[] = $testDir; } - // Add JSON log output to build/phpunit-results.json - $jsonLogPath = $componentPath . '/build/phpunit-results.json'; + // Path for PHPUnit's JUnit XML log. The file is read back below + // to recover the assertions count, which is not surfaced through + // the event subscriber API. + $junitXmlPath = $componentPath . '/build/phpunit-results.xml'; $buildDir = $componentPath . '/build'; // Create build directory if it doesn't exist @@ -325,7 +327,7 @@ public function run(array &$options = []): int $argv[] = '--no-output'; $argv[] = '--log-junit'; - $argv[] = $jsonLogPath; + $argv[] = $junitXmlPath; // Use PHPUnit's Application class for modern in-process execution $app = new \PHPUnit\TextUI\Application(); @@ -335,6 +337,11 @@ public function run(array &$options = []): int $exitCode = $app->run($argv); ob_end_clean(); + // Recover assertions from the JUnit XML log (the event subscriber + // API does not emit per-assertion events; the count lives in the + // XML's top-level testsuites attribute). + $this->stats['assertions'] = self::parseAssertionsFromJunit($junitXmlPath); + // Write our own JSON results file with additional metadata $this->writeJsonResults($componentPath, $exitCode); @@ -414,4 +421,49 @@ private function outputStatistics(): void } } } + + /** + * Recover the assertion count from a PHPUnit JUnit XML log. + * + * PHPUnit's event-subscriber API does not emit per-assertion events, + * so the per-test counter we keep in {@see registerEventSubscribers} + * stays at the count of tests, not assertions. The JUnit XML log + * carries `assertions` as an attribute on the root `` + * element (and on each nested ``). Reading it back is the + * supported, version-stable way to surface the count. + * + * @param string $junitXmlPath Path to the JUnit XML file. + * @return int Assertion count, or 0 when the file is missing/unparseable. + */ + public static function parseAssertionsFromJunit(string $junitXmlPath): int + { + if (!is_file($junitXmlPath) || !is_readable($junitXmlPath)) { + return 0; + } + $previous = libxml_use_internal_errors(true); + try { + $xml = simplexml_load_file($junitXmlPath); + } finally { + libxml_use_internal_errors($previous); + } + if ($xml === false) { + return 0; + } + // Root usually carries the aggregated count. + $assertions = isset($xml['assertions']) ? (int) $xml['assertions'] : 0; + if ($assertions > 0) { + return $assertions; + } + // Fallback for variants that emit a single root or + // omit the aggregated attribute: sum the per-suite counts. + $suites = $xml->getName() === 'testsuites' + ? $xml->children() + : [$xml]; + foreach ($suites as $suite) { + if (isset($suite['assertions'])) { + $assertions += (int) $suite['assertions']; + } + } + return $assertions; + } } diff --git a/src/Qc/Tasks.php b/src/Qc/Tasks.php index b1d923e2..d1205b31 100644 --- a/src/Qc/Tasks.php +++ b/src/Qc/Tasks.php @@ -84,12 +84,16 @@ public function getTask($name, Component $component): TaskBase * @param array $sequence The task sequence. * @param Component $component The component to be checked. * @param array $options Additional options. + * + * @return int Sum of error counts reported by each task. Zero when + * every task ran cleanly. Used by Module\Qc to decide + * whether to throw and make the process exit non-zero. */ public function run( array $sequence, Component $component, $options = [] - ): void { + ): int { $this->_options = $options; $this->_sequence = $sequence; @@ -118,6 +122,7 @@ public function run( } } $output = $this->_dependencies->getInstance(Output::class); + $totalErrors = 0; foreach ($selected_tasks as $task) { $output->bold(str_repeat('-', 30)); $output->ok( @@ -126,6 +131,7 @@ public function run( $output->plain(''); $numErrors = $task->run($options); + $totalErrors += max(0, (int) $numErrors); $output->plain(''); if ($numErrors == 1) { @@ -135,5 +141,6 @@ public function run( } $output->bold(str_repeat('-', 30) . "\n"); } + return $totalErrors; } } diff --git a/src/Runner/Qc.php b/src/Runner/Qc.php index e3a92cb0..30a997d5 100644 --- a/src/Runner/Qc.php +++ b/src/Runner/Qc.php @@ -51,7 +51,7 @@ public function __construct( private readonly QcTasks $qc ) {} - public function run(): void + public function run(): int { $sequence = []; @@ -105,14 +105,14 @@ public function run(): void } if (!empty($sequence)) { - $this->qc->run( + return $this->qc->run( $sequence, $this->component, $this->options ); - } else { - $this->output->warn('Huh?! No tasks selected... All done!'); } + $this->output->warn('Huh?! No tasks selected... All done!'); + return 0; } /** diff --git a/test/Unit/Ci/GitHubAnnotationsTest.php b/test/Unit/Ci/GitHubAnnotationsTest.php new file mode 100644 index 00000000..01a373a7 --- /dev/null +++ b/test/Unit/Ci/GitHubAnnotationsTest.php @@ -0,0 +1,196 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Ci; + +use Horde\Components\Ci\GitHubAnnotations; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +/** + * Tests for the GitHub Actions annotation emitter. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(GitHubAnnotations::class)] +class GitHubAnnotationsTest extends TestCase +{ + private ?string $previousEnv = null; + + protected function setUp(): void + { + // Save and clear so each test sets its own state. + $this->previousEnv = getenv('GITHUB_ACTIONS') ?: null; + putenv('GITHUB_ACTIONS'); + } + + protected function tearDown(): void + { + if ($this->previousEnv === null) { + putenv('GITHUB_ACTIONS'); + } else { + putenv('GITHUB_ACTIONS=' . $this->previousEnv); + } + } + + public function testNoOpWhenNotInGitHubActions(): void + { + ob_start(); + try { + GitHubAnnotations::error('boom'); + $this->assertSame('', ob_get_contents()); + } finally { + ob_end_clean(); + } + } + + public function testNoOpWhenGitHubActionsIsAnythingOtherThanTrue(): void + { + putenv('GITHUB_ACTIONS=false'); + ob_start(); + try { + GitHubAnnotations::warning('hush'); + $this->assertSame('', ob_get_contents()); + } finally { + ob_end_clean(); + } + } + + public function testEmitErrorWithFullTriple(): void + { + putenv('GITHUB_ACTIONS=true'); + $captured = $this->captureEmit(static function (): void { + GitHubAnnotations::error( + 'something is wrong', + 'src/Foo.php', + 42, + 'PHPStan level 5' + ); + }); + $this->assertSame( + '::error file=src/Foo.php,line=42,title=PHPStan level 5::something is wrong' . PHP_EOL, + $captured + ); + } + + public function testEmitWarningWithFileOnly(): void + { + putenv('GITHUB_ACTIONS=true'); + $captured = $this->captureEmit(static function (): void { + GitHubAnnotations::warning('would be modified', 'src/Foo.php'); + }); + $this->assertSame( + '::warning file=src/Foo.php::would be modified' . PHP_EOL, + $captured + ); + } + + public function testEmitNoticeSidebarOnlyWhenAllParamsNull(): void + { + putenv('GITHUB_ACTIONS=true'); + $captured = $this->captureEmit(static function (): void { + GitHubAnnotations::notice('heads up'); + }); + $this->assertSame('::notice::heads up' . PHP_EOL, $captured); + } + + public function testEscapeMessageNewlines(): void + { + putenv('GITHUB_ACTIONS=true'); + $captured = $this->captureEmit(static function (): void { + GitHubAnnotations::error("line one\nline two"); + }); + $this->assertSame('::error::line one%0Aline two' . PHP_EOL, $captured); + } + + public function testEscapeMessagePercent(): void + { + putenv('GITHUB_ACTIONS=true'); + $captured = $this->captureEmit(static function (): void { + GitHubAnnotations::error('100% broken'); + }); + $this->assertSame('::error::100%25 broken' . PHP_EOL, $captured); + } + + public function testEscapePropertyValueColonAndComma(): void + { + putenv('GITHUB_ACTIONS=true'); + // Title legitimately containing colons and commas. + $captured = $this->captureEmit(static function (): void { + GitHubAnnotations::error( + 'msg', + null, + null, + 'Foo: bar, baz' + ); + }); + $this->assertSame( + '::error title=Foo%3A bar%2C baz::msg' . PHP_EOL, + $captured + ); + } + + public function testRelativizeForAnnotation(): void + { + $this->assertSame( + 'src/Foo.php', + GitHubAnnotations::relativizeForAnnotation( + '/tmp/horde-ci/lanes/php8.3-dev/Victim/src/Foo.php', + '/tmp/horde-ci/lanes/php8.3-dev/Victim' + ) + ); + } + + public function testRelativizeForAnnotationLeavesUnrelatedPathUnchanged(): void + { + $this->assertSame( + '/elsewhere/Foo.php', + GitHubAnnotations::relativizeForAnnotation( + '/elsewhere/Foo.php', + '/tmp/horde-ci/lanes/php8.3-dev/Victim' + ) + ); + } + + public function testIsActiveTrueOnlyWhenEnvIsExactlyTrue(): void + { + putenv('GITHUB_ACTIONS=true'); + $this->assertTrue(GitHubAnnotations::isActive()); + + putenv('GITHUB_ACTIONS=1'); // truthy in some shells but not GitHub's contract + $this->assertFalse(GitHubAnnotations::isActive()); + + putenv('GITHUB_ACTIONS'); + $this->assertFalse(GitHubAnnotations::isActive()); + } + + /** + * Capture stdout produced by the supplied callable. + */ + private function captureEmit(callable $fn): string + { + ob_start(); + try { + $fn(); + return (string) ob_get_contents(); + } finally { + ob_end_clean(); + } + } +} diff --git a/test/Unit/Ci/Run/FindingsAggregatorTest.php b/test/Unit/Ci/Run/FindingsAggregatorTest.php new file mode 100644 index 00000000..fc7e1fdb --- /dev/null +++ b/test/Unit/Ci/Run/FindingsAggregatorTest.php @@ -0,0 +1,234 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Ci\Run; + +use Horde\Components\Ci\Run\FindingsAggregator; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +/** + * Tests for the per-tool findings aggregator. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(FindingsAggregator::class)] +class FindingsAggregatorTest extends TestCase +{ + private string $tempBase; + + protected function setUp(): void + { + $this->tempBase = sys_get_temp_dir() . '/findings-aggregator-' . bin2hex(random_bytes(4)); + mkdir($this->tempBase, 0o755, true); + } + + protected function tearDown(): void + { + $this->recursiveRm($this->tempBase); + } + + public function testPhpStanIdenticalFindingAcrossLanesCollapsesToOne(): void + { + $native = [ + 'files' => [ + '/tmp/horde-ci/lanes/php8.3-dev/Victim/src/Foo.php' => [ + 'errors' => 1, + 'messages' => [ + ['message' => 'm', 'line' => 10, 'identifier' => 'isset.property'], + ], + ], + ], + ]; + $native2 = [ + 'files' => [ + '/tmp/horde-ci/lanes/php8.4-dev/Victim/src/Foo.php' => [ + 'errors' => 1, + 'messages' => [ + ['message' => 'm', 'line' => 10, 'identifier' => 'isset.property'], + ], + ], + ], + ]; + + $laneA = $this->writeBuild('php8.3-dev', 'phpstan-native.json', $native); + $laneB = $this->writeBuild('php8.4-dev', 'phpstan-native.json', $native2); + + $findings = (new FindingsAggregator())->aggregatePhpStan([ + 'php8.3-dev' => $laneA, + 'php8.4-dev' => $laneB, + ]); + + $this->assertCount(1, $findings); + $this->assertSame('src/Foo.php', $findings[0]['file']); + $this->assertSame(10, $findings[0]['line']); + $this->assertSame('isset.property', $findings[0]['identifier']); + $this->assertSame(['php8.3-dev', 'php8.4-dev'], $findings[0]['lanes']); + } + + public function testPhpStanDifferentLineSplitsToTwoFindings(): void + { + $a = [ + 'files' => [ + '/tmp/horde-ci/lanes/php8.3-dev/X/src/Foo.php' => [ + 'errors' => 2, + 'messages' => [ + ['message' => 'm', 'line' => 10, 'identifier' => 'isset.property'], + ['message' => 'm', 'line' => 20, 'identifier' => 'isset.property'], + ], + ], + ], + ]; + $laneA = $this->writeBuild('php8.3-dev', 'phpstan-native.json', $a); + + $findings = (new FindingsAggregator())->aggregatePhpStan(['php8.3-dev' => $laneA]); + $this->assertCount(2, $findings); + $this->assertSame(10, $findings[0]['line']); + $this->assertSame(20, $findings[1]['line']); + } + + public function testPhpStanFallsBackToMessageWhenIdentifierMissing(): void + { + $a = [ + 'files' => [ + '/tmp/horde-ci/lanes/php8.3-dev/X/src/Foo.php' => [ + 'errors' => 1, + 'messages' => [ + ['message' => 'msg-A', 'line' => 10], + ], + ], + ], + ]; + $b = [ + 'files' => [ + '/tmp/horde-ci/lanes/php8.4-dev/X/src/Foo.php' => [ + 'errors' => 1, + 'messages' => [ + ['message' => 'msg-B', 'line' => 10], + ], + ], + ], + ]; + $laneA = $this->writeBuild('php8.3-dev', 'phpstan-native.json', $a); + $laneB = $this->writeBuild('php8.4-dev', 'phpstan-native.json', $b); + + $findings = (new FindingsAggregator())->aggregatePhpStan([ + 'php8.3-dev' => $laneA, + 'php8.4-dev' => $laneB, + ]); + + // Different message text → distinct findings even at same file/line. + $this->assertCount(2, $findings); + } + + public function testPhpStanMissingNativeFileIsSkipped(): void + { + // Only one lane has a native file; the other has none. + $a = [ + 'files' => [ + '/x/src/Foo.php' => ['messages' => [['message' => 'm', 'line' => 1, 'identifier' => 'i']]], + ], + ]; + $laneA = $this->writeBuild('php8.3-dev', 'phpstan-native.json', $a); + $laneB = $this->tempBase . '/php8.4-dev/build'; + mkdir($laneB, 0o755, true); + + $findings = (new FindingsAggregator())->aggregatePhpStan([ + 'php8.3-dev' => $laneA, + 'php8.4-dev' => $laneB, + ]); + + $this->assertCount(1, $findings); + $this->assertSame(['php8.3-dev'], $findings[0]['lanes']); + } + + public function testPhpCsFixerSameFileAcrossLanesCollapsesToOne(): void + { + $a = ['files' => [['name' => 'src/Foo.php']]]; + $b = ['files' => [['name' => 'src/Foo.php'], ['name' => 'src/Bar.php']]]; + + $laneA = $this->writeBuild('php8.3-dev', 'php-cs-fixer-native.json', $a); + $laneB = $this->writeBuild('php8.4-dev', 'php-cs-fixer-native.json', $b); + + $findings = (new FindingsAggregator())->aggregatePhpCsFixer([ + 'php8.3-dev' => $laneA, + 'php8.4-dev' => $laneB, + ]); + + $this->assertCount(2, $findings); + $this->assertSame('src/Bar.php', $findings[0]['file']); + $this->assertSame(['php8.4-dev'], $findings[0]['lanes']); + $this->assertSame('src/Foo.php', $findings[1]['file']); + $this->assertSame(['php8.3-dev', 'php8.4-dev'], $findings[1]['lanes']); + } + + public function testEmptyInputsReturnEmpty(): void + { + $this->assertSame([], (new FindingsAggregator())->aggregatePhpStan([])); + $this->assertSame([], (new FindingsAggregator())->aggregatePhpCsFixer([])); + } + + public function testFindingsAreSortedDeterministically(): void + { + $a = [ + 'files' => [ + '/tmp/horde-ci/lanes/X/Component/src/Z.php' => ['messages' => [['message' => 'm', 'line' => 5, 'identifier' => 'b']]], + '/tmp/horde-ci/lanes/X/Component/src/A.php' => ['messages' => [['message' => 'm', 'line' => 10, 'identifier' => 'a']]], + ], + ]; + $laneA = $this->writeBuild('X', 'phpstan-native.json', $a); + + $findings = (new FindingsAggregator())->aggregatePhpStan(['X' => $laneA]); + $this->assertSame('src/A.php', $findings[0]['file']); + $this->assertSame('src/Z.php', $findings[1]['file']); + } + + /** + * @param array $payload + */ + private function writeBuild(string $laneName, string $filename, array $payload): string + { + $buildDir = $this->tempBase . '/' . $laneName . '/build'; + mkdir($buildDir, 0o755, true); + file_put_contents( + $buildDir . '/' . $filename, + json_encode($payload, JSON_THROW_ON_ERROR) + ); + return $buildDir; + } + + private function recursiveRm(string $dir): void + { + if (!is_dir($dir)) { + return; + } + foreach (scandir($dir) as $entry) { + if ($entry === '.' || $entry === '..') { + continue; + } + $path = $dir . '/' . $entry; + if (is_dir($path) && !is_link($path)) { + $this->recursiveRm($path); + } else { + @unlink($path); + } + } + @rmdir($dir); + } +} diff --git a/test/Unit/Output/Presenter/CiTest.php b/test/Unit/Output/Presenter/CiTest.php index 289cf68e..3c5190b7 100644 --- a/test/Unit/Output/Presenter/CiTest.php +++ b/test/Unit/Output/Presenter/CiTest.php @@ -79,7 +79,7 @@ public function testOkWithGitHubActions(): void $presenter = new Ci($this->output); $presenter->ok('Success message'); - $this->assertEquals("::notice::Success message\n", $this->getOutput()); + $this->assertEquals("Success message\n", $this->getOutput()); } /** @@ -103,7 +103,7 @@ public function testWarnWithGitHubActions(): void $presenter = new Ci($this->output); $presenter->warn('Warning message'); - $this->assertEquals("::warning::Warning message\n", $this->getOutput()); + $this->assertEquals("Warning message\n", $this->getOutput()); } /** @@ -127,7 +127,7 @@ public function testInfoWithGitHubActions(): void $presenter = new Ci($this->output); $presenter->info('Info message'); - $this->assertEquals("::notice::Info message\n", $this->getOutput()); + $this->assertEquals("Info message\n", $this->getOutput()); } /** @@ -151,7 +151,7 @@ public function testErrorWithGitHubActions(): void $presenter = new Ci($this->output); $presenter->error('Error message'); - $this->assertEquals("::error::Error message\n", $this->getOutput()); + $this->assertEquals("Error message\n", $this->getOutput()); } /** @@ -275,9 +275,9 @@ public function testGitHubActionsMultipleOutputs(): void $presenter->warn('Second'); $presenter->error('Third'); - $expected = "::notice::First\n" - . "::warning::Second\n" - . "::error::Third\n"; + $expected = "First\n" + . "Second\n" + . "Third\n"; $this->assertEquals($expected, $this->getOutput()); } @@ -342,7 +342,7 @@ public function testSemanticDetectedGitHub(): void $presenter = new Ci($this->output); $presenter->semantic('detected', 'Found tool'); - $this->assertEquals("::debug::Found tool\n", $this->getOutput()); + $this->assertEquals("Found tool\n", $this->getOutput()); } public function testSemanticRegressionPlain(): void @@ -360,7 +360,7 @@ public function testSemanticRegressionGitHub(): void $presenter = new Ci($this->output); $presenter->semantic('regression', 'Quality degraded'); - $this->assertEquals("::error::Quality degraded\n", $this->getOutput()); + $this->assertEquals("Quality degraded\n", $this->getOutput()); } public function testSemanticImprovementGitHub(): void @@ -369,7 +369,7 @@ public function testSemanticImprovementGitHub(): void $presenter = new Ci($this->output); $presenter->semantic('improvement', 'Quality improved'); - $this->assertEquals("::notice::Quality improved\n", $this->getOutput()); + $this->assertEquals("Quality improved\n", $this->getOutput()); } public function testSemanticRunningPlain(): void @@ -463,7 +463,7 @@ public function testSemanticAcceptsTraditionalOkWithGitHub(): void $presenter = new Ci($this->output); $presenter->semantic('ok', 'Success message'); - $this->assertEquals("::notice::Success message\n", $this->getOutput()); + $this->assertEquals("Success message\n", $this->getOutput()); } public function testSemanticAcceptsTraditionalErrorWithGitHub(): void @@ -472,6 +472,6 @@ public function testSemanticAcceptsTraditionalErrorWithGitHub(): void $presenter = new Ci($this->output); $presenter->semantic('error', 'Error message'); - $this->assertEquals("::error::Error message\n", $this->getOutput()); + $this->assertEquals("Error message\n", $this->getOutput()); } } diff --git a/test/Unit/Qc/Task/UnitJunitParserTest.php b/test/Unit/Qc/Task/UnitJunitParserTest.php new file mode 100644 index 00000000..d9fafeb5 --- /dev/null +++ b/test/Unit/Qc/Task/UnitJunitParserTest.php @@ -0,0 +1,104 @@ + + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ + +declare(strict_types=1); + +namespace Horde\Components\Test\Unit\Qc\Task; + +use Horde\Components\Qc\Task\Unit; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; + +/** + * Tests for the JUnit XML assertion-count parser on the Qc Unit task. + * + * @category Horde + * @package Components + * @author Ralf Lang + * @license http://www.horde.org/licenses/lgpl21 LGPL 2.1 + */ +#[CoversClass(Unit::class)] +class UnitJunitParserTest extends TestCase +{ + private string $tempPath; + + protected function setUp(): void + { + $this->tempPath = tempnam(sys_get_temp_dir(), 'unit-junit-'); + } + + protected function tearDown(): void + { + @unlink($this->tempPath); + } + + public function testRootAttributeWins(): void + { + file_put_contents($this->tempPath, <<<'XML' + + + + + XML); + + $this->assertSame(256, Unit::parseAssertionsFromJunit($this->tempPath)); + } + + public function testFallbackToSummingSuitesWhenRootMissing(): void + { + file_put_contents($this->tempPath, <<<'XML' + + + + + + XML); + + $this->assertSame(53, Unit::parseAssertionsFromJunit($this->tempPath)); + } + + public function testSingleTestsuiteRoot(): void + { + // Some PHPUnit configs emit a bare root instead of + // . The parser should still find the count. + file_put_contents($this->tempPath, <<<'XML' + + + XML); + + $this->assertSame(21, Unit::parseAssertionsFromJunit($this->tempPath)); + } + + public function testMissingFileReturnsZero(): void + { + $this->assertSame(0, Unit::parseAssertionsFromJunit('/nonexistent/path.xml')); + } + + public function testGarbageContentReturnsZero(): void + { + file_put_contents($this->tempPath, 'this is not XML'); + $this->assertSame(0, Unit::parseAssertionsFromJunit($this->tempPath)); + } + + public function testMissingAttributeReturnsZero(): void + { + file_put_contents($this->tempPath, <<<'XML' + + + + + XML); + $this->assertSame(0, Unit::parseAssertionsFromJunit($this->tempPath)); + } +}