Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to print Junit report in human-readable format #154

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@
],
"require": {
"php": "^7.2 || ^8.0",
"ext-dom": "*",
"ext-json": "*",
"ext-libxml": "*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep those only soft-required. I dont want to force everybody to have those. Majority uses only console output.

Just detect presence only when junit formatter is used and emit error if not.

"ext-tokenizer": "*"
},
"require-dev": {
"ext-dom": "*",
"ext-libxml": "*",
"editorconfig-checker/editorconfig-checker": "^10.3.0",
"ergebnis/composer-normalize": "^2.19",
"phpcompatibility/php-compatibility": "^9.3",
Expand Down
2 changes: 1 addition & 1 deletion src/Initializer.php
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ public function initFormatter(CliOptions $options): ResultFormatter
throw new InvalidConfigException("Cannot use 'junit' format with '--dump-usages' option.");
}

return new JunitFormatter($this->cwd, $this->stdOutPrinter);
return new JunitFormatter($this->cwd, $this->stdOutPrinter, $options->verbose);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it always pretty. Thank you.

}

if ($format === 'console') {
Expand Down
35 changes: 35 additions & 0 deletions src/Result/AbstractXmlFormatter.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php declare(strict_types = 1);

namespace ShipMonk\ComposerDependencyAnalyser\Result;

use DOMDocument;
use DOMElement;
use ShipMonk\ComposerDependencyAnalyser\Printer;

abstract class AbstractXmlFormatter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently useless.

{

/**
* @var Printer
*/
protected $printer;

/**
* @var DOMDocument
*/
protected $document;

/**
* @var DOMElement
*/
protected $rootElement;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep only other services inside formatter service. This state is not needed as property as it is needed only for format method execution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for $document.


public function __construct(Printer $printer, bool $verbose)
{
$this->printer = $printer;

$this->document = new DOMDocument('1.0', 'UTF-8');
$this->document->formatOutput = $verbose;
}

}
119 changes: 76 additions & 43 deletions src/Result/JunitFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace ShipMonk\ComposerDependencyAnalyser\Result;

use DOMException;
use ShipMonk\ComposerDependencyAnalyser\CliOptions;
use ShipMonk\ComposerDependencyAnalyser\Config\Configuration;
use ShipMonk\ComposerDependencyAnalyser\Config\Ignore\UnusedErrorIgnore;
Expand All @@ -19,9 +20,10 @@
use function substr;
use const ENT_COMPAT;
use const ENT_XML1;
use const LIBXML_NOEMPTYTAG;
use const PHP_INT_MAX;

class JunitFormatter implements ResultFormatter
final class JunitFormatter extends AbstractXmlFormatter implements ResultFormatter
{

/**
Expand All @@ -30,25 +32,29 @@ class JunitFormatter implements ResultFormatter
private $cwd;

/**
* @var Printer
* @throws DOMException
*/
private $printer;

public function __construct(string $cwd, Printer $printer)
public function __construct(string $cwd, Printer $printer, ?bool $verbose = null)
{
if ($verbose === null) {
$verbose = false;
}

parent::__construct($printer, $verbose);
$this->cwd = $cwd;
$this->printer = $printer;
$this->rootElement = $this->document->createElement('testsuites');
$this->document->appendChild($this->rootElement);
}

/**
* @throws DOMException
*/
public function format(
AnalysisResult $result,
CliOptions $options,
Configuration $configuration
): int
{
$xml = '<?xml version="1.0" encoding="UTF-8"?>';
$xml .= '<testsuites>';

$hasError = false;
$unusedIgnores = $result->getUnusedIgnores();

Expand All @@ -63,7 +69,7 @@ public function format(

if (count($unknownClassErrors) > 0) {
$hasError = true;
$xml .= $this->createSymbolBasedTestSuite(
$this->createSymbolBasedTestSuite(
'unknown classes',
$unknownClassErrors,
$maxShownUsages
Expand All @@ -72,7 +78,7 @@ public function format(

if (count($unknownFunctionErrors) > 0) {
$hasError = true;
$xml .= $this->createSymbolBasedTestSuite(
$this->createSymbolBasedTestSuite(
'unknown functions',
$unknownFunctionErrors,
$maxShownUsages
Expand All @@ -81,7 +87,7 @@ public function format(

if (count($shadowDependencyErrors) > 0) {
$hasError = true;
$xml .= $this->createPackageBasedTestSuite(
$this->createPackageBasedTestSuite(
'shadow dependencies',
$shadowDependencyErrors,
$maxShownUsages
Expand All @@ -90,7 +96,7 @@ public function format(

if (count($devDependencyInProductionErrors) > 0) {
$hasError = true;
$xml .= $this->createPackageBasedTestSuite(
$this->createPackageBasedTestSuite(
'dev dependencies in production code',
$devDependencyInProductionErrors,
$maxShownUsages
Expand All @@ -99,7 +105,7 @@ public function format(

if (count($prodDependencyOnlyInDevErrors) > 0) {
$hasError = true;
$xml .= $this->createPackageBasedTestSuite(
$this->createPackageBasedTestSuite(
'prod dependencies used only in dev paths',
array_fill_keys($prodDependencyOnlyInDevErrors, []),
$maxShownUsages
Expand All @@ -108,7 +114,7 @@ public function format(

if (count($unusedDependencyErrors) > 0) {
$hasError = true;
$xml .= $this->createPackageBasedTestSuite(
$this->createPackageBasedTestSuite(
'unused dependencies',
array_fill_keys($unusedDependencyErrors, []),
$maxShownUsages
Expand All @@ -117,12 +123,16 @@ public function format(

if ($unusedIgnores !== [] && $configuration->shouldReportUnmatchedIgnoredErrors()) {
$hasError = true;
$xml .= $this->createUnusedIgnoresTestSuite($unusedIgnores);
$this->createUnusedIgnoresTestSuite($unusedIgnores);
}

$xml .= '</testsuites>';
$xmlString = $this->document->saveXML(null, LIBXML_NOEMPTYTAG);

$this->printer->print($xml);
if ($xmlString === false) {
$xmlString = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isnt this hiding some failure? I think this should be some exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you need something like this to get proper exception message.

Not sure tho...

}

$this->printer->print($xmlString);

if ($hasError) {
return 255;
Expand All @@ -146,13 +156,20 @@ private function getMaxUsagesShownForErrors(CliOptions $options): int

/**
* @param array<string, list<SymbolUsage>> $errors
* @throws DOMException
*/
private function createSymbolBasedTestSuite(string $title, array $errors, int $maxShownUsages): string
private function createSymbolBasedTestSuite(string $title, array $errors, int $maxShownUsages): void
{
$xml = sprintf('<testsuite name="%s" failures="%u">', $this->escape($title), count($errors));
$testsuite = $this->document->createElement('testsuite');
$testsuite->setAttribute('name', $this->escape($title));
$testsuite->setAttribute('failures', sprintf('%u', count($errors)));

$this->rootElement->appendChild($testsuite);

foreach ($errors as $symbol => $usages) {
$xml .= sprintf('<testcase name="%s">', $this->escape($symbol));
$testcase = $this->document->createElement('testcase');
$testcase->setAttribute('name', $this->escape($symbol));
$testsuite->appendChild($testcase);

if ($maxShownUsages > 1) {
$failureUsage = [];
Expand All @@ -170,38 +187,43 @@ private function createSymbolBasedTestSuite(string $title, array $errors, int $m
}
}

$xml .= sprintf('<failure>%s</failure>', $this->escape(implode('\n', $failureUsage)));
$failureMessage = $this->escape(implode('\n', $failureUsage));
} else {
$firstUsage = $usages[0];
$restUsagesCount = count($usages) - 1;
$rest = $restUsagesCount > 0 ? " (+ {$restUsagesCount} more)" : '';
$xml .= sprintf('<failure>in %s%s</failure>', $this->escape($this->relativizeUsage($firstUsage)), $rest);

$failureMessage = sprintf('in %s%s', $this->escape($this->relativizeUsage($firstUsage)), $rest);
}

$xml .= '</testcase>';
$failure = $this->document->createElement('failure', $failureMessage);
$testcase->appendChild($failure);
}

$xml .= '</testsuite>';

return $xml;
}

/**
* @param array<string, array<string, list<SymbolUsage>>> $errors
* @throws DOMException
*/
private function createPackageBasedTestSuite(string $title, array $errors, int $maxShownUsages): string
private function createPackageBasedTestSuite(string $title, array $errors, int $maxShownUsages): void
{
$xml = sprintf('<testsuite name="%s" failures="%u">', $this->escape($title), count($errors));
$testsuite = $this->document->createElement('testsuite');
$testsuite->setAttribute('name', $this->escape($title));
$testsuite->setAttribute('failures', sprintf('%u', count($errors)));

foreach ($errors as $packageName => $usagesPerClassname) {
$xml .= sprintf('<testcase name="%s">', $this->escape($packageName));
$xml .= sprintf('<failure>%s</failure>', $this->escape(implode('\n', $this->createUsages($usagesPerClassname, $maxShownUsages))));
$xml .= '</testcase>';
}
$this->rootElement->appendChild($testsuite);

$xml .= '</testsuite>';
foreach ($errors as $packageName => $usagesPerClassname) {
$testcase = $this->document->createElement('testcase');
$testcase->setAttribute('name', $this->escape($packageName));
$testsuite->appendChild($testcase);

return $xml;
$failure = $this->document->createElement(
'failure',
$this->escape(implode('\n', $this->createUsages($usagesPerClassname, $maxShownUsages)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be "\n", otherwise it outputs the backslash.

Yes, it was buggy even before your change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same above

);
$testcase->appendChild($failure);
}
}

/**
Expand Down Expand Up @@ -265,17 +287,23 @@ static function (int $carry, array $usages): int {

/**
* @param list<UnusedSymbolIgnore|UnusedErrorIgnore> $unusedIgnores
* @throws DOMException
*/
private function createUnusedIgnoresTestSuite(array $unusedIgnores): string
private function createUnusedIgnoresTestSuite(array $unusedIgnores): void
{
$xml = sprintf('<testsuite name="unused-ignore" failures="%u">', count($unusedIgnores));
$testsuite = $this->document->createElement('testsuite');
$testsuite->setAttribute('name', 'unused-ignore');
$testsuite->setAttribute('failures', sprintf('%u', count($unusedIgnores)));

$this->rootElement->appendChild($testsuite);

foreach ($unusedIgnores as $unusedIgnore) {
if ($unusedIgnore instanceof UnusedSymbolIgnore) {
$kind = $unusedIgnore->getSymbolKind() === SymbolKind::CLASSLIKE ? 'class' : 'function';
$regex = $unusedIgnore->isRegex() ? ' regex' : '';
$message = "Unknown {$kind}{$regex} '{$unusedIgnore->getUnknownSymbol()}' was ignored, but it was never applied.";
$xml .= sprintf('<testcase name="%s"><failure>%s</failure></testcase>', $this->escape($unusedIgnore->getUnknownSymbol()), $this->escape($message));

$testcaseName = $this->escape($unusedIgnore->getUnknownSymbol());
} else {
$package = $unusedIgnore->getPackage();
$path = $unusedIgnore->getPath();
Expand All @@ -297,11 +325,16 @@ private function createUnusedIgnoresTestSuite(array $unusedIgnores): string
$message = "'{$unusedIgnore->getErrorType()}' was ignored for package '{$package}' and path '{$this->relativizePath($path)}', but it was never applied.";
}

$xml .= sprintf('<testcase name="%s"><failure>%s</failure></testcase>', $this->escape($unusedIgnore->getErrorType()), $this->escape($message));
$testcaseName = $this->escape($unusedIgnore->getErrorType());
}
}

return $xml . '</testsuite>';
$testcase = $this->document->createElement('testcase');
$testcase->setAttribute('name', $testcaseName);
$testsuite->appendChild($testcase);

$failure = $this->document->createElement('failure', $this->escape($message));
$testcase->appendChild($failure);
}
}

private function relativizeUsage(SymbolUsage $usage): string
Expand Down
2 changes: 1 addition & 1 deletion tests/BinTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public function test(): void

$usingConfig = 'Using config';

$junitOutput = '<?xml version="1.0" encoding="UTF-8"?><testsuites></testsuites>';
$junitOutput = '<?xml version="1.0" encoding="UTF-8"?>' . "\n" . '<testsuites></testsuites>';

$this->runCommand('php bin/composer-dependency-analyser', $rootDir, 0, $okOutput, $usingConfig);
$this->runCommand('php bin/composer-dependency-analyser --verbose', $rootDir, 0, $okOutput, $usingConfig);
Expand Down
9 changes: 7 additions & 2 deletions tests/JunitFormatterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace ShipMonk\ComposerDependencyAnalyser;

use DOMDocument;
use DOMException;
use ShipMonk\ComposerDependencyAnalyser\Config\Configuration;
use ShipMonk\ComposerDependencyAnalyser\Config\ErrorType;
use ShipMonk\ComposerDependencyAnalyser\Config\Ignore\UnusedErrorIgnore;
Expand Down Expand Up @@ -168,7 +169,7 @@ private function prettyPrintXml(string $inputXml): string
{
$dom = new DOMDocument();
$dom->preserveWhiteSpace = false;
$dom->formatOutput = true;
$dom->formatOutput = true; // always in human-readable format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can now compare it 1:1 without altering the real output. It should just match the passed expected string.

$dom->loadXML($inputXml);

$outputXml = $dom->saveXML(null, LIBXML_NOEMPTYTAG);
Expand All @@ -177,9 +178,13 @@ private function prettyPrintXml(string $inputXml): string
return trim($outputXml);
}

/**
* @throws DOMException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exceptions from other namespaces should never bubble up. Catch it at the lowest possible level and wrap to some our exception. That one can be propagated.

Doing that, you benefit from PHPStan exception analysis, see checkedExceptionClasses

*/
protected function createFormatter(Printer $printer): ResultFormatter
{
return new JunitFormatter('/app', $printer);
// human-readable format with 'verbose' option set to true
return new JunitFormatter('/app', $printer, true);
}

}
Loading