Exceptions in data providers should be errors, not warnings

[lstrojny]

Q	A
PHPUnit version	9.4.1
PHP version	7.4.8
Installation Method	Composer

Summary

When a data provider throws an exception, PHPUnit reports the error as a warning. This can lead to false positive test suite results where important tests are skipped but still the test suite succeeds when run without failOnWarning=true (which is the default).

Current behavior

When a data providers throws a Throwable, it’s converted into a warning (from PHPUnit\Framework\TestBuilder)

            } catch (Throwable $t) {
                $message = sprintf(
                    "The data provider specified for %s::%s is invalid.\n%s",
                    $className,
                    $methodName,
                    $this->throwableToString($t)
                );

                $data = new WarningTestCase($message);

How to reproduce

<?php
class DataProviderTest extends \PHPUnit\Framework\TestCase
{
    public static function provide(): iterable
    {
        throw new Exception();
    }

    /** @dataProvider provide */
    public function test()
    {
    }
}

Expected behavior

PHPUnit should issue an error instead.

[epdenouden]

Agreed. The 'unit' being tested is the code plus data. This is also how PHPUnit internally thinks as you might have noticed while digging around in the TestBuilder. The provided data is retrieved and turned into individual TestCases.

@sebastianbergmann is not-failing on brokken/incompatible data providers a way of working that is a leftover from earlier days, or is there a deeper reason behind it?

For the lazy loading data providers I need to refactor the loading mechanism anyway. I don't see any big risks for converting the warning to a failure for either the current or future data loaders.

[lstrojny]

Just discovered that the same behavior is true for mock objects, where e.g. when a class is final and can't be mocked the test will succeed with a warning but it didn't even run. That also doesn't seem sensible to me. New issue?

[sebastianbergmann]

It does not "succeed with a warning". A test is only successful when it is not marked as errored, failed, incomplete, skipped, risky -- and when there is no warning.

By default, the PHPUnit test runner does not exit with an error code because of risky tests or tests that trigger warnings. However, the best practices configuration you get from phpunit --generate-configuration gives you:

<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.4/phpunit.xsd"
         bootstrap="vendor/autoload.php"
         cacheResultFile=".phpunit.cache/test-results"
         executionOrder="depends,defects"
         forceCoversAnnotation="true"
         beStrictAboutCoversAnnotation="true"
         beStrictAboutOutputDuringTests="true"
         beStrictAboutTodoAnnotatedTests="true"
         failOnRisky="true"
         failOnWarning="true"
         verbose="true">
    <testsuites>
        <testsuite name="default">
            <directory suffix="Test.php">tests</directory>
        </testsuite>
    </testsuites>

    <coverage cacheDirectory=".phpunit.cache/code-coverage"
              processUncoveredFiles="true">
        <include>
            <directory suffix=".php">src</directory>
        </include>
    </coverage>
</phpunit>

The failOnRisky="true" and failOnWarning="true" settings configure the PHPUnit test runner to exit with an error code for risky tests and tests that trigger warnings.

[lstrojny]

I understand. What I think is conceptually problematic here is that two quite different things are classified as warnings:

• Something that make the test unexecutable (mocking errors, data provider errors)
• Something that will make the test unexecutable in the future but it still works (deprecation warnings)

Now a user has the choice: either fix all deprecation warnings immediately or risk false positives. Conceptually I think the better categorization of errors would be:

• Something that makes the test unexecutable: error
• Something that is incorrect but the test will still be useful (coverage annotations, deprecations): warning

[sebastianbergmann]

I agree that this should be changed, thank you for making a convincing argument.

[sebastianbergmann]

Split into #4490, #4491, and #4492.

[lstrojny]

Thank you @sebastianbergmann