From 3f5a62af1ec60f0c807bbca6c25b99d9a9699a32 Mon Sep 17 00:00:00 2001 From: Filippo Tessarotto Date: Thu, 9 Feb 2023 12:07:13 +0100 Subject: [PATCH] Fix JUnit merge issues (#729) --- src/JUnit/LogMerger.php | 6 +- src/JUnit/MessageType.php | 3 +- src/JUnit/TestCase.php | 25 ------ src/JUnit/TestCaseWithMessage.php | 1 - src/JUnit/TestSuite.php | 72 +++++++++------- src/JUnit/Writer.php | 2 +- test/Unit/WrapperRunner/WrapperRunnerTest.php | 86 +++++++++---------- test/fixtures/common_results/combined.xml | 2 +- .../common_results/junit/WarningTest.xml | 2 +- 9 files changed, 88 insertions(+), 111 deletions(-) diff --git a/src/JUnit/LogMerger.php b/src/JUnit/LogMerger.php index b425fd7a..ddf2f4f1 100644 --- a/src/JUnit/LogMerger.php +++ b/src/JUnit/LogMerger.php @@ -8,6 +8,7 @@ use function array_merge; use function assert; +use function ksort; /** * @internal @@ -39,7 +40,6 @@ public function merge(array $junitFiles): TestSuite $mainSuite->assertions, $mainSuite->failures, $mainSuite->errors, - $mainSuite->risky, $mainSuite->skipped, $mainSuite->time, '', @@ -55,7 +55,6 @@ public function merge(array $junitFiles): TestSuite $otherSuite->assertions, $otherSuite->failures, $otherSuite->errors, - $otherSuite->risky, $otherSuite->skipped, $otherSuite->time, '', @@ -90,13 +89,14 @@ private function mergeSuites(TestSuite $suite1, TestSuite $suite2): TestSuite ); } + ksort($suites); + return new TestSuite( $suite1->name, $suite1->tests + $suite2->tests, $suite1->assertions + $suite2->assertions, $suite1->failures + $suite2->failures, $suite1->errors + $suite2->errors, - $suite1->risky + $suite2->risky, $suite1->skipped + $suite2->skipped, $suite1->time + $suite2->time, $suite1->file, diff --git a/src/JUnit/MessageType.php b/src/JUnit/MessageType.php index 9c6fa6f6..22572046 100644 --- a/src/JUnit/MessageType.php +++ b/src/JUnit/MessageType.php @@ -9,13 +9,12 @@ enum MessageType { case error; case failure; - case risky; case skipped; public function toString(): string { return match ($this) { - self::error, self::risky => 'error', + self::error => 'error', self::failure => 'failure', self::skipped => 'skipped', }; diff --git a/src/JUnit/TestCase.php b/src/JUnit/TestCase.php index ffc46a01..86433b4d 100644 --- a/src/JUnit/TestCase.php +++ b/src/JUnit/TestCase.php @@ -31,13 +31,6 @@ public function __construct( final public static function caseFromNode(SimpleXMLElement $node): self { - $systemOutput = null; - $systemOutputs = $node->xpath('system-out'); - if ($systemOutputs !== null && $systemOutputs !== []) { - assert(count($systemOutputs) === 1); - $systemOutput = (string) current($systemOutputs); - } - $getFirstNode = static function (array $nodes): SimpleXMLElement { assert(count($nodes) === 1); $node = current($nodes); @@ -59,21 +52,6 @@ final public static function caseFromNode(SimpleXMLElement $node): self $type = $getType($error); $text = (string) $error; - if ($type === 'PHPUnit\\Framework\\RiskyTest') { - return new TestCaseWithMessage( - (string) $node['name'], - (string) $node['class'], - (string) $node['file'], - (int) $node['line'], - (int) $node['assertions'], - (float) $node['time'], - $type, - $text, - $systemOutput, - MessageType::risky, - ); - } - return new TestCaseWithMessage( (string) $node['name'], (string) $node['class'], @@ -83,7 +61,6 @@ final public static function caseFromNode(SimpleXMLElement $node): self (float) $node['time'], $type, $text, - $systemOutput, MessageType::error, ); } @@ -102,7 +79,6 @@ final public static function caseFromNode(SimpleXMLElement $node): self (float) $node['time'], $type, $text, - $systemOutput, MessageType::failure, ); } @@ -128,7 +104,6 @@ final public static function caseFromNode(SimpleXMLElement $node): self (float) $node['time'], null, $text, - $systemOutput, MessageType::skipped, ); } diff --git a/src/JUnit/TestCaseWithMessage.php b/src/JUnit/TestCaseWithMessage.php index e64460f2..2aff7c1f 100644 --- a/src/JUnit/TestCaseWithMessage.php +++ b/src/JUnit/TestCaseWithMessage.php @@ -20,7 +20,6 @@ public function __construct( float $time, public readonly ?string $type, public readonly string $text, - public readonly ?string $systemOutput, public readonly MessageType $xmlTagName ) { parent::__construct($name, $class, $file, $line, $assertions, $time); diff --git a/src/JUnit/TestSuite.php b/src/JUnit/TestSuite.php index 5275e34e..9e860523 100644 --- a/src/JUnit/TestSuite.php +++ b/src/JUnit/TestSuite.php @@ -7,9 +7,8 @@ use SimpleXMLElement; use SplFileInfo; -use function array_map; -use function array_sum; use function assert; +use function count; use function file_get_contents; /** @@ -19,11 +18,6 @@ */ final class TestSuite { - /** @var array */ - public readonly array $suites; - /** @var TestCase */ - public readonly array $cases; - /** * @param array $suites * @param list $cases @@ -34,15 +28,12 @@ public function __construct( public readonly int $assertions, public readonly int $failures, public readonly int $errors, - public readonly int $risky, public readonly int $skipped, public readonly float $time, public readonly string $file, - array $suites, - array $cases + public readonly array $suites, + public readonly array $cases ) { - $this->suites = $suites; - $this->cases = $cases; } public static function fromFile(SplFileInfo $logFile): self @@ -61,15 +52,41 @@ public static function fromFile(SplFileInfo $logFile): self private static function parseTestSuite(SimpleXMLElement $node, bool $isRootSuite): self { if ($isRootSuite) { - foreach ($node->testsuite as $singleTestSuiteXml) { - return self::parseTestSuite($singleTestSuiteXml, false); - } + $tests = 0; + $assertions = 0; + $failures = 0; + $errors = 0; + $skipped = 0; + $time = 0; + } else { + $tests = (int) $node['tests']; + $assertions = (int) $node['assertions']; + $failures = (int) $node['failures']; + $errors = (int) $node['errors']; + $skipped = (int) $node['skipped']; + $time = (float) $node['time']; } + $count = count($node->testsuite); $suites = []; foreach ($node->testsuite as $singleTestSuiteXml) { - $testSuite = self::parseTestSuite($singleTestSuiteXml, false); + $testSuite = self::parseTestSuite($singleTestSuiteXml, false); + if ($isRootSuite && $count === 1) { + return $testSuite; + } + $suites[$testSuite->name] = $testSuite; + + if (! $isRootSuite) { + continue; + } + + $tests += $testSuite->tests; + $assertions += $testSuite->assertions; + $failures += $testSuite->failures; + $errors += $testSuite->errors; + $skipped += $testSuite->skipped; + $time += $testSuite->time; } $cases = []; @@ -77,25 +94,14 @@ private static function parseTestSuite(SimpleXMLElement $node, bool $isRootSuite $cases[] = TestCase::caseFromNode($singleTestCase); } - $risky = array_sum(array_map(static function (TestCase $testCase): int { - return (int) ( - $testCase instanceof TestCaseWithMessage - && $testCase->xmlTagName === MessageType::risky - ); - }, $cases)); - $risky += array_sum(array_map(static function (TestSuite $testSuite): int { - return $testSuite->risky; - }, $suites)); - return new self( (string) $node['name'], - (int) $node['tests'], - (int) $node['assertions'], - (int) $node['failures'], - (int) $node['errors'] - $risky, - $risky, - (int) $node['skipped'], - (float) $node['time'], + $tests, + $assertions, + $failures, + $errors, + $skipped, + $time, (string) $node['file'], $suites, $cases, diff --git a/src/JUnit/Writer.php b/src/JUnit/Writer.php index 382f25b9..8ffe5515 100644 --- a/src/JUnit/Writer.php +++ b/src/JUnit/Writer.php @@ -65,7 +65,7 @@ private function createSuiteNode(TestSuite $parentSuite): DOMElement $suiteNode->setAttribute('tests', (string) $parentSuite->tests); $suiteNode->setAttribute('assertions', (string) $parentSuite->assertions); - $suiteNode->setAttribute('errors', (string) ($parentSuite->errors + $parentSuite->risky)); + $suiteNode->setAttribute('errors', (string) $parentSuite->errors); $suiteNode->setAttribute('failures', (string) $parentSuite->failures); $suiteNode->setAttribute('skipped', (string) $parentSuite->skipped); $suiteNode->setAttribute('time', (string) $parentSuite->time); diff --git a/test/Unit/WrapperRunner/WrapperRunnerTest.php b/test/Unit/WrapperRunner/WrapperRunnerTest.php index 409979be..7384b9b4 100644 --- a/test/Unit/WrapperRunner/WrapperRunnerTest.php +++ b/test/Unit/WrapperRunner/WrapperRunnerTest.php @@ -7,7 +7,6 @@ use ParaTest\RunnerInterface; use ParaTest\Tests\TestBase; use ParaTest\WrapperRunner\WorkerCrashedException; -use PHPUnit\Framework\Assert; use SebastianBergmann\CodeCoverage\CodeCoverage; use Symfony\Component\Process\Process; @@ -16,11 +15,12 @@ use function array_unique; use function defined; use function file_get_contents; +use function file_put_contents; use function min; use function posix_mkfifo; use function preg_match_all; +use function preg_replace; use function scandir; -use function simplexml_load_file; use function sprintf; use function str_replace; use function uniqid; @@ -36,6 +36,7 @@ * @covers \ParaTest\WrapperRunner\WorkerCrashedException * @covers \ParaTest\WrapperRunner\ResultPrinter * @covers \ParaTest\Coverage\CoverageMerger + * @covers \ParaTest\JUnit\TestSuite */ final class WrapperRunnerTest extends TestBase { @@ -124,7 +125,7 @@ public function testRunnerSortTestEqualBySeed(): void $firstOutput = $this->prepareOutputForTestOrderCheck($runnerResultFirst->output); $secondOutput = $this->prepareOutputForTestOrderCheck($runnerResultSecond->output); - Assert::assertSame($firstOutput, $secondOutput); + self::assertSame($firstOutput, $secondOutput); $this->bareOptions['--random-order-seed'] = '321'; @@ -132,7 +133,7 @@ public function testRunnerSortTestEqualBySeed(): void $thirdOutput = $this->prepareOutputForTestOrderCheck($runnerResultThird->output); - Assert::assertNotSame($thirdOutput, $firstOutput); + self::assertNotSame($thirdOutput, $firstOutput); } /** @return string[] */ @@ -140,7 +141,7 @@ private function prepareOutputForTestOrderCheck(string $output): array { $matchesCount = preg_match_all('/executing: (?\S+\.php)/', $output, $matches); - Assert::assertGreaterThan(0, $matchesCount); + self::assertGreaterThan(0, $matchesCount); return $matches['filename']; } @@ -156,7 +157,7 @@ public function testRunnerSortNoSeedProvided(): void $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('Random Seed:', $runnerResult->output); + self::assertStringContainsString('Random Seed:', $runnerResult->output); } /** @@ -170,11 +171,11 @@ public function testErrorsInDataProviderAreHandled(): void $this->bareOptions['--configuration'] = $this->fixture('github' . DS . 'GH565' . DS . 'phpunit.xml'); $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('The data provider specified for ParaTest\Tests\fixtures\github\GH565\IssueTest::testIncompleteByDataProvider is invalid', $runnerResult->output); - Assert::assertStringContainsString('The data provider specified for ParaTest\Tests\fixtures\github\GH565\IssueTest::testSkippedByDataProvider is invalid', $runnerResult->output); - Assert::assertStringContainsString('The data provider specified for ParaTest\Tests\fixtures\github\GH565\IssueTest::testErrorByDataProvider is invalid', $runnerResult->output); - Assert::assertStringContainsString('Warnings: 1', $runnerResult->output); - Assert::assertEquals(RunnerInterface::EXCEPTION_EXIT, $runnerResult->exitCode); + self::assertStringContainsString('The data provider specified for ParaTest\Tests\fixtures\github\GH565\IssueTest::testIncompleteByDataProvider is invalid', $runnerResult->output); + self::assertStringContainsString('The data provider specified for ParaTest\Tests\fixtures\github\GH565\IssueTest::testSkippedByDataProvider is invalid', $runnerResult->output); + self::assertStringContainsString('The data provider specified for ParaTest\Tests\fixtures\github\GH565\IssueTest::testErrorByDataProvider is invalid', $runnerResult->output); + self::assertStringContainsString('Warnings: 1', $runnerResult->output); + self::assertEquals(RunnerInterface::EXCEPTION_EXIT, $runnerResult->exitCode); } public function testParatestEnvironmentVariableWithWrapperRunnerWithoutTestTokens(): void @@ -184,8 +185,8 @@ public function testParatestEnvironmentVariableWithWrapperRunnerWithoutTestToken $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('Failures: 1', $runnerResult->output); - Assert::assertEquals(RunnerInterface::FAILURE_EXIT, $runnerResult->exitCode); + self::assertStringContainsString('Failures: 1', $runnerResult->output); + self::assertEquals(RunnerInterface::FAILURE_EXIT, $runnerResult->exitCode); } public function testParatestEnvironmentVariable(): void @@ -200,7 +201,7 @@ public function testPassthrus(): void $this->bareOptions['path'] = $this->fixture('passthru_tests' . DS . 'PassthruTest.php'); $runnerResult = $this->runRunner(); - Assert::assertSame(RunnerInterface::FAILURE_EXIT, $runnerResult->exitCode); + self::assertSame(RunnerInterface::FAILURE_EXIT, $runnerResult->exitCode); $this->bareOptions['--passthru-php'] = sprintf("'-d' 'highlight.comment=%s'", self::PASSTHRU_PHP_CUSTOM); if (defined('PHP_WINDOWS_VERSION_BUILD')) { @@ -231,7 +232,7 @@ public function testTeamcityOutput(): void $result = $this->runRunner(); - Assert::assertSame(35, preg_match_all('/^##teamcity/m', $result->output)); + self::assertSame(35, preg_match_all('/^##teamcity/m', $result->output)); } public function testExitCodesPathWithoutTests(): void @@ -239,7 +240,7 @@ public function testExitCodesPathWithoutTests(): void $this->bareOptions['path'] = $this->fixture('no_tests'); $runnerResult = $this->runRunner(); - Assert::assertEquals(RunnerInterface::SUCCESS_EXIT, $runnerResult->exitCode); + self::assertEquals(RunnerInterface::SUCCESS_EXIT, $runnerResult->exitCode); } /** @requires OSFAMILY Linux */ @@ -256,8 +257,8 @@ public function testTeamcityLogHandlesFifoFiles(): void $this->runRunner(); - Assert::assertSame(0, $fifoReader->wait()); - Assert::assertSame(5, preg_match_all('/^##teamcity/m', $fifoReader->getOutput())); + self::assertSame(0, $fifoReader->wait()); + self::assertSame(5, preg_match_all('/^##teamcity/m', $fifoReader->getOutput())); } public function testStopOnFailureEndsRunBeforeWholeTestSuite(): void @@ -334,50 +335,47 @@ public function testExitCodes(): void $this->bareOptions['path'] = $this->fixture('common_results' . DS . 'ErrorTest.php'); $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('Errors: 1', $runnerResult->output); - Assert::assertEquals(RunnerInterface::EXCEPTION_EXIT, $runnerResult->exitCode); + self::assertStringContainsString('Errors: 1', $runnerResult->output); + self::assertEquals(RunnerInterface::EXCEPTION_EXIT, $runnerResult->exitCode); $this->bareOptions['path'] = $this->fixture('common_results' . DS . 'FailureTest.php'); $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('Failures: 1', $runnerResult->output); - Assert::assertEquals(RunnerInterface::FAILURE_EXIT, $runnerResult->exitCode); + self::assertStringContainsString('Failures: 1', $runnerResult->output); + self::assertEquals(RunnerInterface::FAILURE_EXIT, $runnerResult->exitCode); $this->bareOptions['path'] = $this->fixture('common_results' . DS . 'SuccessTest.php'); $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('OK', $runnerResult->output); - Assert::assertEquals(RunnerInterface::SUCCESS_EXIT, $runnerResult->exitCode); + self::assertStringContainsString('OK', $runnerResult->output); + self::assertEquals(RunnerInterface::SUCCESS_EXIT, $runnerResult->exitCode); $this->bareOptions['path'] = $this->fixture('common_results'); $runnerResult = $this->runRunner(); - Assert::assertStringContainsString('Failures: 1', $runnerResult->output); - Assert::assertStringContainsString('Errors: 1', $runnerResult->output); - Assert::assertEquals(RunnerInterface::EXCEPTION_EXIT, $runnerResult->exitCode); + self::assertStringContainsString('Failures: 1', $runnerResult->output); + self::assertStringContainsString('Errors: 1', $runnerResult->output); + self::assertEquals(RunnerInterface::EXCEPTION_EXIT, $runnerResult->exitCode); } public function testWritesLogWithEmptyNameWhenPathIsNotProvided(): void { - $outputPath = $this->tmpDir . DS . 'test-output.xml'; + $outputFile = $this->tmpDir . DS . 'test-output.xml'; $this->bareOptions = [ '--configuration' => $this->fixture('phpunit-common_results.xml'), - '--log-junit' => $outputPath, + '--log-junit' => $outputFile, ]; $this->runRunner(); - Assert::assertFileExists($outputPath); - $doc = simplexml_load_file($outputPath); - Assert::assertNotFalse($doc); - $suites = (array) $doc->children(); - Assert::assertArrayHasKey('testsuite', $suites); - $attribues = (array) $suites['testsuite']->attributes(); - Assert::assertArrayHasKey('@attributes', $attribues); - Assert::assertIsArray($attribues['@attributes']); - Assert::assertArrayHasKey('name', $attribues['@attributes']); - Assert::assertSame('', $attribues['@attributes']['name']); + self::assertFileExists($outputFile); + $xml = file_get_contents($outputFile); + $xml = str_replace(FIXTURES, './test/fixtures', $xml); + $xml = preg_replace('/time="[^"]+"/', 'time="1.234567"', $xml); + file_put_contents($outputFile, $xml); + + self::assertXmlFileEqualsXmlFile(FIXTURES . '/common_results/combined.xml', $outputFile); } public function testRunnerReversed(): void @@ -397,7 +395,7 @@ public function testRunnerReversed(): void $reverseOrderReversed = array_reverse($reverseOrder); - Assert::assertSame($defaultOrder, $reverseOrderReversed); + self::assertSame($defaultOrder, $reverseOrderReversed); } /** @@ -440,11 +438,11 @@ public function testTeamcityLog(): void $this->runRunner(); - Assert::assertFileExists($outputPath); + self::assertFileExists($outputPath); $content = file_get_contents($outputPath); - Assert::assertNotFalse($content); + self::assertNotFalse($content); - Assert::assertSame(35, preg_match_all('/^##teamcity/m', $content)); + self::assertSame(35, preg_match_all('/^##teamcity/m', $content)); } public function testRunningFewerTestsThanTheWorkersIsPossible(): void @@ -467,7 +465,7 @@ public function testResultsAreCorrect(): void static::assertEquals(0, $runnerResult->exitCode); $coveragePhp = include $this->bareOptions['--coverage-php']; - Assert::assertInstanceOf(CodeCoverage::class, $coveragePhp); + self::assertInstanceOf(CodeCoverage::class, $coveragePhp); } public function testHandleCollisionWithSymfonyOutput(): void diff --git a/test/fixtures/common_results/combined.xml b/test/fixtures/common_results/combined.xml index c39927fb..8c82e07d 100644 --- a/test/fixtures/common_results/combined.xml +++ b/test/fixtures/common_results/combined.xml @@ -34,7 +34,7 @@ Failed asserting that false is true. - + diff --git a/test/fixtures/common_results/junit/WarningTest.xml b/test/fixtures/common_results/junit/WarningTest.xml index 2fb9dccb..a6bfeefd 100644 --- a/test/fixtures/common_results/junit/WarningTest.xml +++ b/test/fixtures/common_results/junit/WarningTest.xml @@ -1,6 +1,6 @@ - +