Problem/Motivation

When removing the HtmlOutputPrinter listener in PHPUnit 10, we replaced it with the HtmlOutputLogger extension, and we initialize it from the bootstrap.php file. However, the canonical way to bootstrap PHPUnit extensions is by including it in the phpunit.xml, https://docs.phpunit.de/en/11.0/configuration.html#the-extensions-element

See https://docs.phpunit.de/en/11.0/extending-phpunit.html#implementing-an-e...

Also see https://drupal.slack.com/archives/C1BMUQ9U6/p1717659911747249

Proposed resolution

  • Change the extension bootstrap to phpunit.xml
  • Return the default to print out the list of all the links generated (verbose=true)

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 3453341-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3453341

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

mondrake created an issue. See original summary.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Status: Active » Needs review
longwave’s picture

Issue tags: +Needs change record

We will need a change record as anyone with a custom phpunit.xml will need to add this.

I wonder if we should configure it with the extension configuration instead of using environment variables, or perhaps support both methods?

Let's also let @alexpott confirm that this solves the problem he found with paratest.

mondrake’s picture

+1 on adding config parameters in the xml, and deprecating the two environment variables.

longwave’s picture

Re: the change record and custom phpunit.xml, I wonder if we can detect from bootstrap.php or elsewhere that the extension is not loaded, and print some kind of notice. Or we could just detect the environment variables and print a notice or deprecation, I guess.

mondrake’s picture

I've tried #7 for deprecating BROWSERTEST_OUTPUT_VERBOSE, although one may argue this is not really a deprecation since there is no point release using it yet.

Deprecating BROWSERTEST_OUTPUT_DIRECTORY is somehow more complicated, since its value is set dynamically in code by PhpUnitTestRunner - for testing purposes and to determine realpath. I'd rather try do that in a follow-up.

alexpott’s picture

++ this works great and fixes running Drupal tests using paratest. We should definitely do this. Thanks @mondrake.

alexpott’s picture

mondrake’s picture

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

Let's see if I can please everyone with the following:
1. Let's have xml parameters for output directory and verbose, with defaults. Verbose = true
2. Let's keep the two env variables as overrides of the xml parameters, if they exist (i.e. no more deprecation)

This way we combine both worlds, and on CI we do not have to tamper with the xml file.

On that.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
mondrake’s picture

Issue tags: -Needs change record

Added a draft CR.

alexpott’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

This looks fantastic - nice work @mondrake

Raising this to major as it fixes a regression in Drupal 11. The regression is the ability to use paratest - which works great on D10. I think with this change and the move of deprecation reporting to PHPUnit it might be finally possible to consider removing run-tests.sh in favour of paratest which would be quite amazing.

mondrake’s picture

Once this is in, the main CR for PHPUnit 10 https://www.drupal.org/node/3365413 will need adjustment.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community

Rebased, only context changes

  • longwave committed 72601e5a on 11.0.x
    Issue #3453341 by mondrake, alexpott, longwave: Bootstrap...

  • longwave committed 0bfc9722 on 11.x
    Issue #3453341 by mondrake, alexpott, longwave: Bootstrap...
longwave’s picture

Version: 11.x-dev » 11.0.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +DevDaysBurgas2024

Committed and pushed 0bfc972263 to 11.x and 72601e5aef to 11.0.x. Thanks!

mondrake’s picture

Updated the main CR https://www.drupal.org/node/3365413 to reflect this.

Status: Fixed » Closed (fixed)

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