Allow to print Junit report in human-readable format #154
Conversation
|
Introducing the new class |
|
I don't think we need such option. We should just always pretty-print the output. |
I don't agree with you : my opinion is that
|
| "ext-json": "*", | ||
| "ext-libxml": "*", |
There was a problem hiding this comment.
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.
| use DOMElement; | ||
| use ShipMonk\ComposerDependencyAnalyser\Printer; | ||
|
|
||
| abstract class AbstractXmlFormatter |
| @@ -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); | |||
There was a problem hiding this comment.
Please make it always pretty. Thank you.
| return $xml; | ||
| $failure = $this->document->createElement( | ||
| 'failure', | ||
| $this->escape(implode('\n', $this->createUsages($usagesPerClassname, $maxShownUsages))) |
There was a problem hiding this comment.
I believe this should be "\n", otherwise it outputs the backslash.
Yes, it was buggy even before your change.
| /** | ||
| * @var DOMElement | ||
| */ | ||
| protected $rootElement; |
There was a problem hiding this comment.
Please keep only other services inside formatter service. This state is not needed as property as it is needed only for format method execution.
| @@ -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 | |||
There was a problem hiding this comment.
I think we can now compare it 1:1 without altering the real output. It should just match the passed expected string.
| @@ -177,9 +178,13 @@ private function prettyPrintXml(string $inputXml): string | |||
| return trim($outputXml); | |||
| } | |||
|
|
|||
| /** | |||
| * @throws DOMException | |||
There was a problem hiding this comment.
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
|
|
||
| $this->printer->print($xml); | ||
| if ($xmlString === false) { | ||
| $xmlString = ''; |
There was a problem hiding this comment.
Isnt this hiding some failure? I think this should be some exception.
There was a problem hiding this comment.
Maybe you need something like this to get proper exception message.
Not sure tho...
|
Closing this PR because we do not have the same vision of reporting format and usage. BTW, there are other enhancement that should be fixed first, to allow re-usability of your reports, otherwise no integration with third-party tools may be thinking . |
|
Ok
Anything else beside #156 and things you already reported? |
|
Yes and #157 (comment) |
Fixes #153