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

junit report is broken #48

Closed
maks-rafalko opened this issue May 30, 2020 · 16 comments
Closed

junit report is broken #48

maks-rafalko opened this issue May 30, 2020 · 16 comments
Assignees
Labels

Comments

@maks-rafalko
Copy link

maks-rafalko commented May 30, 2020

How to reproduce:

Source class:

# src/SourceFile.php

namespace PestExample;

final class SourceFile
{
    public function add(int $a, int $b): int
    {
        return $a + $b;
    }
}

test file:

# tests/SourceFileTest.php

use PestExample\SourceFile;

it('can add two integers', function (): void {
    $source = new SourceFile();

    $result = $source->add(3, 2);

    $this->assertGreaterThan(0, $result);
});

Command line:

vendor/bin/pest --coverage-xml=pest-coverage-xml --log-junit="pest-coverage-xml/junit.xml"

generated files:

tree pest-coverage-xml

pest-coverage-xml
├── SourceFile.php.xml
├── index.xml
└── junit.xml

And the incorrect junit.xml report looks like:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.006614">
    <testsuite name="P\Tests\SourceFileTest" file="/pest-example/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(185) : eval()'d code" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.006614">
      <testcase name="it can add two integers" assertions="1" time="0.006614"/>
    </testsuite>
  </testsuite>
</testsuites>

What's wrong here? look at the following line:

<testsuite name="P\Tests\SourceFileTest" file="pest-example/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(185) : eval()'d code" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.006614">

and in particular look at the file attribute:

file="/pest-example/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(185) : eval()'d code"

It's not a valid test file.


❗❗

Expected: /pest-example/tests/SourceFileTest.php
Actual: /pest-example/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(185) : eval()'d code

If I run the same test but written in PHPUnit style, I get the correct junit report:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.007011">
    <testsuite name="Test Suite" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.007011">
      <testsuite name="PestExample\Test\PhpUnitSourceFileTest" file="/pest-example/tests/PhpUnitSourceFileTest.php" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.007011">
        <testcase name="test_it_can_add_two_integers" class="PestExample\Test\PhpUnitSourceFileTest" classname="PestExample.Test.PhpUnitSourceFileTest" file="/pest-example/tests/PhpUnitSourceFileTest.php" line="14" assertions="1" time="0.007011"/>
      </testsuite>
    </testsuite>
  </testsuite>
</testsuites>

Related to and blocks #4

@nunomaduro
Copy link
Member

That sounds normal to me.

Behind the scenes pest creates real classes to make the integration seamless with PHPUnit.

Not sure if we plan to fix this.

@nunomaduro nunomaduro added the bug label May 30, 2020
@maks-rafalko
Copy link
Author

Behind the scenes pest creates real classes to make the integration seamless with PHPUnit.

That's perfectly fine, but let me disagree with you that the current junit report "sounds normal".

So the purpose of junit report is to

  1. give the information what class is used
  2. give the information what file is used
  3. give the information what test cases are used (methods in plain PHPUnit, or test() / it() blocks in Pest)
  4. give the information about metrics, the most important ones are the time that test took to execute and the line of covered code

However, we see that the file attribute has invalid values:

/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(185) : eval()'d code

Two issues here

  1. it's not a real file of executed test (SourceFileTest.php in our example)
  2. it's not a valid path to file (note the (185) : eval()'d code postfix)

And now, let me describe the importance of the junit report for Infection.

Imagine, you have 1000 tests, and only 2 of them covers line 33 that is currently being mutated by Infection.

Infection does its best to run only those tests that covers the mutated line (line 33 in our example). All other 998 tests are not executed.

And it does that thanks to the information fetched from junit report. Without it, all 1000 tests will be executed for each mutation, which leads to completely unusable mutation testing technic for the real projects.

Do you have any ideas how we can improve that?

@nunomaduro
Copy link
Member

Got it.

Without junit report, no infection adapter right?

@maks-rafalko
Copy link
Author

Well, we can do it, but it will be barely useful. The worst thing is the time.

Another example:

  • without junit: if we have 500 mutations and 1000 tests, it will be 500*1000=500,000 executed tests in the worst scenario
  • with junit: if we have 500 mutations and 1000 tests, and each line is covered by 2-3 tests, we will have 500*3=1500 executed tests in the worst scenario, so it's 333 times better :)

Let me know if I can help with anything here.

@nunomaduro
Copy link
Member

I will take a better look at this tomorrow.

@nunomaduro
Copy link
Member

Just pushed a very native solution for this: https://github.com/pestphp/pest/tree/feat/poc-junit.

Can you try it out and tell me if something is missing?

@maks-rafalko
Copy link
Author

maks-rafalko commented Jun 1, 2020

It's much better! But still not enough.

Currently, it generates such junit report:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
    <testsuite name="P\Tests\SourceFileTest" file="/pest/src/Factories/TestCaseFactory.php(187) : eval()'d code" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
      <testcase name="it can add two integers" class="Tests\SourceFileTest" classname="Tests.SourceFileTest" file="/pest-example/tests/SourceFileTest.php" assertions="1" time="0.022643"/>
    </testsuite>
  </testsuite>
</testsuites>

where <testcase node contains the correct file=/pest-example/tests/SourceFileTest.php, but with incorrect FQCN Tests\SourceFileTest

but node <testsuite contains correct name="P\Tests\SourceFileTest" but incorrect file file="/pest/src/Factories/TestCaseFactory.php(187) : eval()'d code"

Can we somehow have this:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
-    <testsuite name="P\Tests\SourceFileTest" file="/pest/src/Factories/TestCaseFactory.php(187) : eval()'d code" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
+    <testsuite name="P\Tests\SourceFileTest" file="/pest-example/tests/SourceFileTest.php" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
      <testcase name="it can add two integers" class="Tests\SourceFileTest" classname="Tests.SourceFileTest" file="/pest-example/tests/SourceFileTest.php" assertions="1" time="0.022643"/>
    </testsuite>
  </testsuite>
</testsuites>

or at least this

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
    <testsuite name="P\Tests\SourceFileTest" file="/pest/src/Factories/TestCaseFactory.php(187) : eval()'d code" tests="1" assertions="1" errors="0" warnings="0" failures="0" skipped="0" time="0.022643">
-      <testcase name="it can add two integers" class="Tests\SourceFileTest" classname="Tests.SourceFileTest" file="/pest-example/tests/SourceFileTest.php" assertions="1" time="0.022643"/>
+      <testcase name="it can add two integers" class="P\Tests\SourceFileTest" classname="Tests.SourceFileTest" file="/pest-example/tests/SourceFileTest.php" assertions="1" time="0.022643"/>

    </testsuite>
  </testsuite>
</testsuites>

In other words, we need to have a node where we both have P\Test\SourceFileTest as FQCL and /pest-example/tests/SourceFileTest.php as a file

Explanation:

  • P\Test\SourceFileTest is used in coverage-xml report as a FQCN, but not Test\SourceFileTest (not the P\ prefix, I guess this is what Pest adds)

@maks-rafalko
Copy link
Author

@nunomaduro with the following 2 changes in your branch, I was able to generate a correct junit

diff --git a/native/phpunit/phpunit/src/Util/Log/JUnit.php b/native/phpunit/phpunit/src/Util/Log/JUnit.php
index 3a21ead..87afb77 100644
--- a/native/phpunit/phpunit/src/Util/Log/JUnit.php
+++ b/native/phpunit/phpunit/src/Util/Log/JUnit.php
@@ -198,8 +198,7 @@ final class JUnit extends Printer implements TestListener
                 if ($suite instanceof \Pest\Contracts\Test) {
                     $testSuite->setAttribute('file', $suite->getFilename());
                 } else {
-                    $class = new \ReflectionClass($suite->getName());
-                    $testSuite->setAttribute('file', $class->getFileName());
+                    $testSuite->setAttribute('file', $suite->tests()[0]->getFilename());
                 }
             } catch (\ReflectionException $e) {
             }
@@ -303,7 +302,7 @@ final class JUnit extends Printer implements TestListener
         // @codeCoverageIgnoreEnd

         if ($test instanceof \Pest\Contracts\Test) {
-            $testCase->setAttribute('class', $test->getPrintableTestCaseName());
+            $testCase->setAttribute('class', $class->getName());
             $testCase->setAttribute('classname', \str_replace('\\', '.', $test->getPrintableTestCaseName()));
             $testCase->setAttribute('file', $test->getFilename());
         } else {

I've created 2 pest files (tests) with 2 cases (test() functions) inside just to quickly check it.

Resulting junit report is:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="" tests="4" assertions="4" errors="0" warnings="0" failures="0" skipped="0" time="0.028040">
    <testsuite name="P\Tests\FirstSourceFileTest" file="/pest-example/tests/FirstSourceFileTest.php" tests="2" assertions="2" errors="0" warnings="0" failures="0" skipped="0" time="0.021014">
      <testcase name="it First file - first case" class="P\Tests\FirstSourceFileTest" classname="Tests.FirstSourceFileTest" file="/pest-example/tests/FirstSourceFileTest.php" assertions="1" time="0.019044"/>
      <testcase name="it First file - second case" class="P\Tests\FirstSourceFileTest" classname="Tests.FirstSourceFileTest" file="/pest-example/tests/FirstSourceFileTest.php" assertions="1" time="0.001970"/>
    </testsuite>
    <testsuite name="P\Tests\SecondSourceFileTest" file="/pest-example/tests/SecondSourceFileTest.php" tests="2" assertions="2" errors="0" warnings="0" failures="0" skipped="0" time="0.007026">
      <testcase name="it Second file - first case" class="P\Tests\SecondSourceFileTest" classname="Tests.SecondSourceFileTest" file="/pest-example/tests/SecondSourceFileTest.php" assertions="1" time="0.003728"/>
      <testcase name="it Second file - second case" class="P\Tests\SecondSourceFileTest" classname="Tests.SecondSourceFileTest" file="/pest-example/tests/SecondSourceFileTest.php" assertions="1" time="0.003297"/>
    </testsuite>
  </testsuite>
</testsuites>

For sure, this is not a ready solution, but probably will speed up your investigation on this topic. What do you think we need to do else to move it forward?

@nunomaduro
Copy link
Member

To be honest I am without time for this at the moment. But it's cool that we already know how to fix this problem in pest.

@olivernybroe
Copy link
Member

This issue is actually related to the issues we had with TeamCity printer.

@nunomaduro
Copy link
Member

I’m closing this issue because it has been inactive for a few months.

@michael-rubel
Copy link

@nunomaduro @maks-rafalko @olivernybroe Is it okay that Infection shows the same in its report when using Pest v2? It seems the issue appeared again:
/var/www/html/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(196) : eval()'d code

Screenshot_1

@olivernybroe
Copy link
Member

@michael-rubel Pest v2 does not have junit support.
PR's are welcome for adding the support again 👍

@michael-rubel
Copy link

michael-rubel commented Jun 27, 2023

@olivernybroe 😄 For some time after upgrading to v2 I was sure my code is "ideal" because Infection was reporting 100% when I was at 100% coverage. Incredible fail.

@arondeparon
Copy link

arondeparon commented Aug 7, 2023

It's not entirely clear to me whether this is still an open issue. @olivernybroe mentioned that Pest v2 does not have junit support; is this still the case? I see that --log-junit is documented, but assume that this simply proxies to PHPUnit.

I'm running into this today because this causes Gitlab coverage reports to break because the correct filename can't be determined.

Example:
CleanShot 2023-08-07 at 16 39 55

If this is still an issue, are there any workarounds?

@olivernybroe
Copy link
Member

@arondeparon issue was resolved back in pest V1, hence why it was closed.

@nunomaduro chose to not implement it in pest V2, but I am pretty sure that he is okay with someone adding support.

The best workaround would be to use team city output as that is supported. There might exist converters between those formats. Better workaround is to submit a PR 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants