Add output formatter to create junit reports #167

jschaedl · 2018-06-07T12:31:20Z

fixes #89

jschaedl · 2018-06-07T12:32:15Z

phpunit.xml

+            <directory>./src</directory>
+        </whitelist>
+    </filter>
+</phpunit>


Ah, this file shouldn't be here. Will remove it.

xabbuh · 2018-06-07T12:33:41Z

phpunit.xml

+        <whitelist>
+            <exclude>
+                <directory>./src/Command</directory>
+                <directory>./src/CompilerPass</directory>


this directory does not seem to exist

xabbuh · 2018-06-07T12:35:20Z

src/ClassNameLayerResolver.php

@@ -59,4 +60,11 @@ public function getLayersByClassName(string $className): array

        return array_keys($layers);
    }
+
+    public function getLayers(): array


getLayerNames()? Is it actually required that we do the filtering here instead of letting the consumer decide which information of the layers they are interested in?

Not sure if I understand your question, but couldn't you define the layers you want to see in the yaml config file? So if I want to see different layers, I would create different config files.

Should be renamed to getLayerNames as we return names of the layers. But instead just returning names I think we should return layer-objects.

What about the getLayersByClassName(...)? It is returning names as well. I would rename it too, ok?

Layers are represented as a string. I am fine keeping these names. But we should rename this class to e.g. LayerResolver.

xabbuh · 2018-06-07T12:35:58Z

src/OutputFormatter/JUnitOutputFormatter.php

+
+class JUnitOutputFormatter implements OutputFormatterInterface
+{
+    protected $eventDispatcher;


this property does not seem to be used

Absolutely, will be removed.

xabbuh · 2018-06-07T12:36:38Z

src/OutputFormatter/JUnitOutputFormatter.php

+use SensioLabs\Deptrac\RulesetEngine\RulesetViolation;
+use Symfony\Component\Console\Output\OutputInterface;
+
+class JUnitOutputFormatter implements OutputFormatterInterface


the class is missing an @author tag

and the class could be made final

Hm, none of the classes in this project have an @author tag. But someone needs to start using it, right ;-)

Will make it final

xabbuh · 2018-06-07T12:39:21Z

tests/OutputFormatter/JUnitOutputFormatterTest.php

+{
+    public function testGetName()
+    {
+        $this->assertEquals('junit', (new JUnitOutputFormatter())->getName());


assertSame()

xabbuh · 2018-06-07T12:39:43Z

tests/OutputFormatter/JUnitOutputFormatterTest.php

+    /**
+     * @param array $layers
+     * @param array $violations
+     * @param       $expectedOutput


the @param tags are not really necessary

Will remove them.

smoench · 2018-06-07T12:46:34Z

tests/OutputFormatter/JUnitOutputFormatterTest.php

+
+    private function normalize($str)
+    {
+        return str_replace(["\t", "\n", ' '], '', $str);


Seams like we could use trim instead

Not really, because line breaks and tabs should be removed within the string as well. trim removes the defined characters only at the beginning and the end of a string.

smoench · 2018-06-07T14:14:55Z

src/OutputFormatter/JUnitOutputFormatter.php

+            file_put_contents($dumpXmlPath, $xml);
+            $output->writeln(PHP_EOL . '<info>JUnit Report dumped to '.realpath($dumpXmlPath).'</info>');
+        } else {
+            $output->writeln(PHP_EOL . '<info>JUnit Report dump:</info>');


We should not print any string/line before, because it would be more difficult to pipe the XML-Result into an other programm.

Ok, will change this.

smoench · 2018-06-07T14:16:51Z

tests/OutputFormatter/JUnitOutputFormatterTest.php

+        );
+
+        $o = $output->fetch();
+        $this->assertEquals(


We could use $this->assertXmlStringEqualsXmlString(...) instead if the pre output has been removed

I think I will change this completely and will use $this->assertXmlFileEqualsXmlFile(...). It is easier to read and easier to add new test data later.

jschaedl · 2018-06-08T14:33:01Z

All fixes done! Please merge or comment :-)

timglabisch · 2018-06-09T22:00:16Z

src/OutputFormatter/JUnitOutputFormatter.php

+
+    private function createXml(DependencyContext $dependencyContext)
+    {
+        if (!class_exists("DomDocument")) {


\DomDocument::class

timglabisch · 2018-06-09T22:00:39Z

src/OutputFormatter/JUnitOutputFormatter.php

+
+        if ($dumpXmlPath = $outputFormatterInput->getOption(static::$argument_dump_xml) ?: './junit-report.xml') {
+            file_put_contents($dumpXmlPath, $xml);
+            $output->writeln('<info>JUnit Report dumped to '.realpath($dumpXmlPath).'</info>');


early retun instead of else

timglabisch · 2018-06-09T22:01:18Z

src/OutputFormatter/JUnitOutputFormatter.php

+
+        if ($dumpXmlPath = $outputFormatterInput->getOption(static::$argument_dump_xml) ?: './junit-report.xml') {
+            file_put_contents($dumpXmlPath, $xml);
+            $output->writeln('<info>JUnit Report dumped to '.realpath($dumpXmlPath).'</info>');


coding style (space)

timglabisch · 2018-06-09T22:05:58Z

tests/OutputFormatter/JUnitOutputFormatterTest.php

+use Symfony\Component\Console\Output\BufferedOutput;
+
+/**
+ * @author Jan Schädlich <janschaedlich@sensiolabs.de>


useless "code"

So an author tag is generally not needed? @xabbuh wrote it should have one: https://github.com/sensiolabs-de/deptrac/pull/167/files/98ce07cd86eaa99238303828812d5b6e972c631e#diff-571a84413ead5d19e843d892a9f55ebf

What is the general guideline for this project? No comments at all?

there is no guideline, but also no technical reason why to embed a @author annotation. feel free to discuss this with the sensio team. from my point of view, huge 👎 for any kind of "noise".
@author statements also harm refactoring, because nobody likes to remove them.

timglabisch · 2018-06-09T22:06:32Z

tests/OutputFormatter/JUnitOutputFormatterTest.php

+ */
+class JUnitOutputFormatterTest extends TestCase
+{
+    const ACTUAL_JUNIT_REPORT_FILE = 'actual-junit-report.xml';


should be private

timglabisch · 2018-06-09T22:08:25Z

src/OutputFormatter/JUnitOutputFormatter.php

+        $layerIndex = 0;
+        foreach ($layers as $layer) {
+            $violationsByLayer = $dependencyContext->getViolationsByLayerName($layer);
+            if (count($violationsByLayer) === 0) {


empty is to weak. empty could be "", 0, "0", null, false or array().
I avoid empty, because you never know exactly what everything is checked.

👍 for count

all right :)

timglabisch · 2018-06-11T21:52:38Z

feel free to merge the code if it's ready.

DavidBadura · 2018-06-15T08:04:43Z

src/OutputFormatter/JUnitOutputFormatter.php

+     */
+    private function createXml(DependencyContext $dependencyContext)
+    {
+        if (!class_exists("\DomDocument")) {


I think you have misunderstood the comment. The line should look like this:

if (!class_exists(\DomDocument::class)) {

This does not throw any errors if the class does not exist. The class name is resolved at compile time. ;-)

compile time? ^^

PHP files are run in two stages compile time & run time:

PHP source code -(compile time)-> zend opcode -(run time)-> result

In the compiler step, the ::class expression is converted to a string. See:

https://3v4l.org/fHY7h/vld#output

And here's an example with a nonexistent class:

https://3v4l.org/f0hVj/vld#output

smoench · 2018-06-15T11:55:33Z

src/ClassNameLayerResolverCacheDecorator.php

+
+    public function getLayers(): array
+    {
+        return $this->classNameLayerResolver->getLayers();


This could be cached, too

jschaedl commented Jun 7, 2018

View reviewed changes

Add output formatter to create junit reports

98ce07c

jschaedl force-pushed the junit-formatter branch from a02c5a4 to 98ce07c Compare June 7, 2018 12:35

jschaedl requested review from smoench and DavidBadura June 7, 2018 12:38

xabbuh reviewed Jun 7, 2018

View reviewed changes

smoench reviewed Jun 7, 2018

View reviewed changes

Code review fixes

0a91f20

jschaedl force-pushed the junit-formatter branch from 8d10d44 to 0a91f20 Compare June 8, 2018 07:32

jschaedl removed the request for review from DavidBadura June 8, 2018 08:04

timglabisch reviewed Jun 9, 2018

View reviewed changes

Code review fixes

38e923e

jschaedl force-pushed the junit-formatter branch from 5e13eec to 38e923e Compare June 11, 2018 07:07

DavidBadura reviewed Jun 15, 2018

View reviewed changes

smoench approved these changes Jun 15, 2018

View reviewed changes

jschaedl added 2 commits June 15, 2018 13:44

Code review fixes

2b26d40

Remove author tag

b6c0710

smoench reviewed Jun 15, 2018

View reviewed changes

DavidBadura approved these changes Jun 15, 2018

View reviewed changes

add cache for layer names

ffe9751

smoench merged commit f5c32b6 into master Jun 22, 2018

smoench deleted the junit-formatter branch June 22, 2018 11:20

Add output formatter to create junit reports #167

Add output formatter to create junit reports #167

Conversation

jschaedl commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschaedl Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoench Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschaedl commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timglabisch commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidBadura Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschaedl Jun 7, 2018 •

edited

Loading

smoench Jun 7, 2018 •

edited

Loading

DavidBadura Jun 15, 2018 •

edited

Loading