Problem/Motivation

In #3264764-8: PhpUnitCliTest::testFunctionalTestDebugHtmlOutput fails if BROWSERTEST_OUTPUT_DIRECTORY is an empty string @alexpott said:

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.

On top of that:

In \Drupal\Tests\Core\Test\PhpUnitCliTest::testFunctionalTestDebugHtmlOutput we run \Drupal\Tests\image\Functional\ImageDimensionsTest twice to test Functional test either creates debug HTML output or warns if it can't.

I don't think its a good pattern to use an actual Functional test to test our tests. For instance if the test moves or is somehow altered to not produce HTML output we have a problem.

\Drupal\Tests\image\Functional\ImageDimensionsTest makes 10 HTTP requests, so 10 HTML pages, where we basically only need one.

Steps to reproduce

Proposed resolution

- Turn \Drupal\Tests\Core\Test\PhpUnitCliTest::testFunctionalTestDebugHtmlOutput into a Functional test.

and

- Introduce a new Functional test with one HTTP request, so 1 HTML page and use this in \Drupal\Tests\Core\Test\PhpUnitCliTest::testFunctionalTestDebugHtmlOutput.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3415321

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:

Comments

Spokje created an issue. See original summary.

spokje’s picture

Status: Active » Needs review
mondrake’s picture

Thanks @Spokje! Actually there is also #3265459: Move testFunctionalTestDebugHtmlOutput to a functional test that is itself a follow-up of #3264764-8: PhpUnitCliTest::testFunctionalTestDebugHtmlOutput fails if BROWSERTEST_OUTPUT_DIRECTORY is an empty string. It'd probably make sense to enlarge the scope here and move "the test that tests the other test" itself to a functional one as suggested by @alexpott.

spokje’s picture

Assigned: Unassigned » spokje

Crap, should have searched before going mental in here, thanks @mondrake.

mondrake’s picture

Or I may have been less lazy earlier... :)

spokje’s picture

Heh, so much for our new years resolutions this year...

mondrake’s picture

Gave up a couple of weeks ago already :)

spokje’s picture

Title: Stop using actual Functional test to test DebugHtmlOutput in PhpUnitCliTest::testFunctionalTestDebugHtmlOutput » Refactor \Drupal\Tests\Core\Test\PhpUnitCliTest::testFunctionalTestDebugHtmlOutput
Issue summary: View changes
spokje’s picture

Closed #3265459: Move testFunctionalTestDebugHtmlOutput to a functional test as a duplicate of this one, although that one is older.

This issue expands the scope and already has an MR for a part of the extended scope.

spokje’s picture

Assigned: spokje » Unassigned
Status: Needs work » Needs review

Heh, so much for our new years resolutions this year...

spokje’s picture

Issue summary: View changes
mondrake’s picture

One comment inline, other than that I think that’s all folks!

spokje’s picture

Answered (or rather "questioned") in MR thread.

mondrake’s picture

IMHO the failure you get is absolutely legit, because if you set the env variable to nil, this https://git.drupalcode.org/project/drupal/-/blob/11.x/core/tests/Drupal/... kicks in and skips any output. Try passing a valid directory to the env variable and the test should pass.

I think @alexpott original comments stems from the consideration that functional tests usually run in an env context where all the required variables are set (think CI environments), otherwise they would fail massively.

spokje’s picture

Ah, I see, you're obsoletely right.

Changed in the MR, thanks for explaining.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

LGTM now

longwave’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

OK, this makes total sense, and will confuse me less next time when this fails somehow as I won't get confused as to why ImageDimensionsTest is in the output.

Backported to 10.2.x to keep things in sync.

Committed and pushed 9926ebb005 to 11.x and f33f0a179c to 10.2.x. Thanks!

  • longwave committed f33f0a17 on 10.2.x
    Issue #3415321 by Spokje, mondrake: Refactor \Drupal\Tests\Core\Test\...

  • longwave committed 9926ebb0 on 11.x
    Issue #3415321 by Spokje, mondrake: Refactor \Drupal\Tests\Core\Test\...

Status: Fixed » Closed (fixed)

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