Problem/Motivation

Discovered in #3211131: Call to an undefined static method PHPUnit\Util\ErrorHandler::handleError() in DrupalStandardsListenerTrait, PhpUnitCliTest::testFunctionalTestDebugHtmlOutput() fails if BROWSERTEST_OUTPUT_DIRECTORY is an empty string:

1) Drupal\Tests\Core\Test\PhpUnitCliTest::testFunctionalTestDebugHtmlOutput
Failed asserting that 'PHPUnit 9.5.11 by Sebastian Bergmann and contributors.\n
\n
Runtime:       PHP 8.0.13\n
Configuration: /var/www/html/drupal/core/phpunit.xml.dist\n
\n
Testing Drupal\Tests\image\Functional\ImageDimensionsTest\n
.                                                                   1 / 1 (100%)\n
\n
Time: 00:05.966, Memory: 4.00 MB\n
\n
OK (1 test, 60 assertions)\n
' contains "HTML output was generated".

/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/drupal/core/tests/Drupal/Tests/Core/Test/PhpUnitCliTest.php:52
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:726
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:96

The test attempts to skip if the variable is not set, but this is too strict:

    if (getenv('BROWSERTEST_OUTPUT_DIRECTORY') === FALSE) {
      $this->markTestSkipped('This test requires the environment variable BROWSERTEST_OUTPUT_DIRECTORY to be set.');
    }

In reality HtmlOutputPrinterTrait checks for it being truthy only, so an empty string produces no output:

    if ($html_output_directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY')) {

Steps to reproduce

BROWSERTEST_OUTPUT_DIRECTORY= vendor/bin/phpunit -c core core/tests/Drupal/Tests/Core/Test/PhpUnitCliTest.php

Proposed resolution

Make the check less strict.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 3264764-3.patch877 byteslongwave

Issue fork drupal-3264764

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thanks for opening the followup @longwave

Since we have this in the HtmlOutputPrinterTrait,

    if ($html_output_directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY')) {
      // Initialize html output debugging.
      $html_output_directory = rtrim($html_output_directory, '/');

      // Check if directory exists.
      if (!is_dir($html_output_directory) || !is_writable($html_output_directory)) {
        $this->writeWithColor('bg-red, fg-black', "HTML output directory $html_output_directory is not a writable directory.");
      }

I think we should to the same to check if the test need to be skipped (i.e. ensuring the dir exists and is writable and if not, skip)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3264764-3.patch877 bytes

To me just doing this is enough and fixes the problem at hand. I don't think we should replicate the exact logic in the test - the variable being set, but incorrectly, is an error to me that the test should catch.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Uhm OK but then we should IMO be a bit more explicit what's failing, so using assertDirectoryExists and assertDirectoryIsWritable?

mondrake opened merge request !1824

mondrake’s picture

mondrake
Location 🇮🇹
commented

Done something different, does it make sense to enhance the test like this?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Additional test coverage is good, that works for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Imo we should move testFunctionalTestDebugHtmlOutput to a functional test because it runs functional tests and so has even more requirements than just BROWSERTEST_OUTPUT_DIRECTORY - it'll fail with Exception: You must provide a SIMPLETEST_BASE_URL environment variable to run some PHPUnit based functional tests. too.

Committed 90f9d14 and pushed to 10.0.x. Thanks!

  • alexpott committed 90f9d14 on 10.0.x 
    Issue #3264764 by mondrake, longwave: PhpUnitCliTest::...

mondrake closed merge request !1824

mondrake’s picture

mondrake
Location 🇮🇹
commented

Filed #3265459: Move testFunctionalTestDebugHtmlOutput to a functional test for follow up.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.