PhpStorm/Teamcity + PhpUnit 7.4.1 - Error in setUpBeforeClass

[MaximilianKresse]

Q	A
PHPUnit version	>=7.4.0
PHP version	7.1.8
Installation Method	Composer

Sample:

{
    "require": {
        "php": "^7.1"
    },

    "require-dev": {
        "phpunit/phpunit": "^7.4"
    }
}

<?php
declare(strict_types=1);

use PHPUnit\Framework\TestCase;

require_once __DIR__ . '/../vendor/autoload.php';

class JustATest extends TestCase
{
    public static function setUpBeforeClass(): void
    {
        throw new \RuntimeException('Something\'s not quite right!');
    }

    public function testSomething(): void
    {
        $this->fail('This cannot work!');
    }
}

When I run this test in phpstorm i will just get "Empty test suite." with the success icon without any failed test. Full output:

PHPUnit 7.4.3 by Sebastian Bergmann and contributors.

Empty test suite.

Time: 64 ms, Memory: 2.00MB

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

Process finished with exit code 2

There seems to be an critical error in the teamcity logger (src/Util/Log/TeamCity.php). You added some "if (!$test instanceof TestCase) {return;}" in the last version and this also seem to trigger this error (or rather don't trigger an error). When I change the addError method to the following everything works like expected:

public function addError(Test $test, \Throwable $t, float $time): void
    {
        if (!$test instanceof TestCase && !$test instanceof TestSuite) {
            return;
        }

I'm not sure whether this is everything to fully fix this bug. Propably the other added if-statements also have to be changed like this.

[sebastianbergmann]

ping @SvetlanaZem

[SvetlanaZem]

@sebastianbergmann Thank you! @wbars Would you please take a look?

[wbars]

@sebastianbergmann Why !$test instanceof TestCase check was introduced in the first place? As I can tell, here is flow of the problem:

• We run test suite
• setUpBeforeClass got invoked, throws an exception
• error is nor registered in our engine since addError will silently return
• testSomething is not invoked

After that we got unexpected inconsistent state when we run 0 tests, but no one register at least one skipped/error/assertion and after that unexpected behaviour about the icons come in.

If !$test instanceof TestCase is not crucial I suggest to simply remove them. Otherwise please tell and we will try to figure something out.

[MaximilianKresse]

Because of no further reaction I have created a pull request with a fix for this.

As far as I can see the !$test instanceof TestCase was introduced to clear some errors from Phan (without enough thinking what the change really does). If I inspected the code for ResultPrinters correctly there can be TestCases and TestSuites - both have a getName method but the interface noted in the method signature is only a Test which doesn't have a getName method.

One way to fix this could be the change the Test interface and add a getName() method but because of my lack of knowledge of the phpunit code base I choose to just "improve" !$test instanceof TestCase to !$test instanceof TestCase && !$test instanceof TestSuite where it makes sense.

[epdenouden]

Checking for instanceof TestCase is not enough, @dataprovider driven tests are instances of DataProviderTestSuite which would be covered by instanceof TestSuite.

This has caused quite a few bugs and oversights for the test execution ordering. Some background in #3396.

[MaximilianKresse]

@epdenouden Just to confirm that I'm reading your comment correctly: You're confirming that the actual code is bugged and it's not enough and that checking against instanceof TestCase AND instanceof TestSuite would fixing it, correct? Because there seems to be some kind of confusion...

[epdenouden]

@SvetlanaZem @wbars I have time to look into this in depth and have created a branch with some first tests to isolate the issues. It seems there is more than one thing going on. Different scenarios give very different result outputs.

Branch: https://github.com/epdenouden/phpunit/tree/issue-3364-teamcity-missing-error

Default and TestDox result printer do show setUpBeforeClass error

🔬 Default printer example:

./phpunit \
  tests/end-to-end/regression/GitHub/3364/Issue3364SetupBeforeClassTest.php

✂️
There were 2 errors:

1) Issue3364SetupBeforeClassTest
RuntimeException: throw exception in setUpBeforeClass in /Users/ewout/proj/phpunit-new-order/tests/end-to-end/regression/GitHub/3364/Issue3364SetupBeforeClassTest.php:18
Stack trace:
#0 /Users/ewout/proj/phpunit-new-order/src/Framework/TestSuite.php(703): Issue3364SetupBeforeClassTest::setUpBeforeClass()
#1 /Users/ewout/proj/phpunit-new-order/src/TextUI/TestRunner.php(622): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
✂️

🔬TestDox example:

./phpunit \
  --testdox \
  tests/end-to-end/regression/GitHub/3364/Issue3364SetupBeforeClassTest.php

✂️ 
Issue3364SetupBeforeClassTest
 ✘ Error bootstapping suite (most likely in Issue3364SetupBeforeClassTest::setUpBeforeClass) [0.00 ms]
   │
   │ RuntimeException: throw exception in setUpBeforeClass in /Users/ewout/proj/phpunit-new-order/tests/end-to-end/regression/GitHub/3364/Issue3364SetupBeforeClassTest.php:18
   │ Stack trace:
   │ #0 /Users/ewout/proj/phpunit-new-order/src/Framework/TestSuite.php(703): Issue3364SetupBeforeClassTest::setUpBeforeClass()
✂️ 

🔬TeamCity however doesn't show anything useful...

./phpunit \
  --teamcity \
  tests/end-to-end/regression/GitHub/3364/Issue3364SetupBeforeClassTest.php

✂️ 
##teamcity[testCount count='2' flowId='85156']

##teamcity[testSuiteStarted name='Issue3364SetupBeforeClassTest' locationHint='php_qn:///Users/ewout/proj/phpunit-new-order/tests/end-to-end/regression/GitHub/3364/Issue3364SetupBeforeClassTest.php::\Issue3364SetupBeforeClassTest' flowId='85156']

##teamcity[testSuiteFinished name='Issue3364SetupBeforeClassTest' flowId='85156']
✂️ 

[wbars]

@sebastianbergmann @epdenouden Please refer my previous comment again about the source of the bug.

Since still nobody shared the details behind introduced check and commit message does't do it either it's unclear for me why it should be kept and I still suggest simply remove the check or at least patch it with && !$test instanceof TestSuite as OP suggests (so from this point of view this PR looks OK to me.

\PHPUnit\Framework\TestResult::addError is invoked only with TestCase and TestSuite as first argument so I don't see how this check can help TeamCity (e.g. PhpStorm) logger. If it's crucial for PHPUnit ecosystem, please share insights.

[epdenouden]

@wbars Apologies I didn't make it clear in my reply, your investigation helped pinpointing the problem. I am not sure why the extra check for just TestCase ended up there. I will check the history and see if it related to other bugs. Seems to be just a case of codestyle fixes? The commit message just says "Fix issues identified by Phan".

In any case there needs to be an error emitted somehow about the failing fixtures. I'll look into this.

[epdenouden]

I think I have found a nice fix.

[epdenouden]

@MaximilianKresse and @wbars thanks to both of you for your patience providing ongoing feedback and insight. Maximilian, eventhough your PR didn't get merged, it did provide a quick starting point for further investigation and helped me find the root cause. Thank you.

I have proposed a fix for the underlying problem in #3428. Summary: when setup failed, the TestSuite would report itself to the listener for each and every underlying test. Patching up the TeamCity printer would have hidden the problem again.

The TeamCity and other listeners and printers are going to get a nice rewrite for PHPUnit 8. Let me know if you have any other concerns. (preferably in a seperate ticket :)

[epdenouden]

@SvetlanaZem @wbars I am working on a POC for an update to the event+logger system of PHPUnit and want to use the TeamCity format as one of the main test cases, as it is async, notices changes in TestSuite and supports parallel testing.

Can you help me get in touch with the proper contact at JetBrains?

[wbars]

@epdenouden Please feel free to write an email for me: kirill.smelov at jetbrains dot com.