[PHPUnit 10+ only] Make dump() output in tests no longer use standard streams

Hi @mondrake
Any reason for postpone of issue,

In this issue for PHPUnit 10+, see if we can adopt a different technique instead of using standard streams.

Looking for different technique, will be good if the status is Active. Any Thoughts on this.

The parent was committed but not sure there is anything worth fixing here, I think stderr is a fine place to put debugging information during tests, unless PHPUnit 10 is even more restrictive in some way.

Personally I find it more valuable to get immediate debug feedback as the test runs rather than wait until the end - and if somehow you crash the test or the test runner, is there a chance the captured output is lost? I guess this needs to still be postponed until we are on PHPUnit 10, in any case.

Sorry if I missed something but can the issue summary be updated then reviewed?

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.

First time I'm looking at this, and wow, totally new approach! I guess this is required because of changes in PHPUnit 10?

This needs a CR to explain that phpunit.xml files need to be updated.

I like the printing of the file and line number, though in some instances that might get noisy.

For example, if I'm developing a migration and I want to see the value that's computed on each processing of a row. I'm going to add a single dump() statement and what I want to see is a monotonic list of values:

foo
foo
foo
somethingbad 
foo

I want 'somethingbad' to stick out in the list, and the list being simple is important to being able to do this. Filenames and line numbers will hamper this. (Generally, debugging something on a foreach loop is a scenario where I find dump()ing so much easier than using the debugger, as I can see all the values at a glance rather than keep advancing from a break point for each loop iteration.)

> Personally I find it more valuable to get immediate debug feedback as the test runs rather than wait until the end -

Same! Though I appreciate there may not be a way to do that with this approach.

The testVarDump() test outputs the dump statements it is testing. Is there a way of capturing the dump() output from testVarDump(), like it used to be?

I suspect some test runners will spot dump() output and not like it. It's also going to be noise in the test results from d.org CI.

Also, if the DebugDump extension it not registered, the testVarDump() test fails.

> public static function setDebugDumpHandler

The docs here could do with a @see to a fully-qualified DebugDump

> + * Drupal's extension for printing dump() output results.

Say it's a PHPUnit extension.

> self::$stagingFilePath = tempnam(sys_get_temp_dir(), 'dpd');

Could we use the VFS here? Would that be quicker?

> and if somehow you crash the test or the test runner, is there a chance the captured output is lost?

I think it's more likely that the file won't get cleaned up from the tmp directory. Though if we switch to using the VFS then that is solved.

Ahh right. That's a shame!

Parameter in the extension config -- perfect! :)

> There were already changes in PHPUnit 9 that forced to change the current solution to print to STDERR instead of STDOUT. The real problem is that, to an extent, test code and SUT code share the same standard streams

So is output to STERR still possible, even if not always desirable?

If so, could we add an option for it?

If we want 'live' output, would it work to `tail -f` the file in which the output is getting sent to? If so, it would be useful to document that solution.

> Well that would mean writing to STDOUT, so back to square one. This issue is, fundamentally, avoiding to use standard streams - just capture the output of dump in a temp file,

No, you don't need STDOUT. If the temp file is readable by a second terminal window, then `tail -f` outputs the end of the file *LIVE* as it is being written to. `tail -f` is particularly useful for watching log files, but would work here too, as long as the file is accessible and readable.

> If I understand your idea, that would mean running two separate terminals, one running PHPUnit CLI, and the other the tail command?

Yup.

I wouldn't want that to be the main intended mode of operation, but it would be useful in some circumstances -- such as something that crashes PHPUnit so badly the output is not printed.

Can the CR be written?

CR looks good to me and helps me understand more what's going on.

May be one of those things easier to see once merged so will give it a +1 but will leave in review for others more advanced in phpunit her.e

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

False positive

@mondrake: I am testing the MR on my local machine and there the tests are failing. I am using a DDEV environment.

ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml ./core/tests/Drupal/Tests/UnitTestCaseTest.php" results in:

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml

^ {#571
  +"Aldebaran": "Betelgeuse"
}
^ "Alpheratz"
FF                                                                  2 / 2 (100%)

Time: 00:00.147, Memory: 4.00 MB

There were 2 failures:

1) Drupal\Tests\UnitTestCaseTest::testVarDumpSameProcess
Failed asserting that '[]' [ASCII](length: 2) contains "Aldebaran" [ASCII](length: 9).

/var/www/html/core/tests/Drupal/Tests/UnitTestCaseTest.php:29

2) Drupal\Tests\UnitTestCaseTest::testVarDumpSeparateProcess
Failed asserting that '[]' [ASCII](length: 2) contains "Denebola" [ASCII](length: 8).

/var/www/html/core/tests/Drupal/Tests/UnitTestCaseTest.php:49

ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml --filter testVarDump ./core/tests/Drupal/KernelTests/KernelTestBaseTest.php" results in:

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml

F                                                                   1 / 1 (100%)

Time: 00:00.479, Memory: 4.00 MB

There was 1 failure:

1) Drupal\KernelTests\KernelTestBaseTest::testVarDump
Failed asserting that '[]' [ASCII](length: 2) contains "KernelTestBaseTest::testVarDump" [ASCII](length: 31).

/var/www/html/core/tests/Drupal/KernelTests/KernelTestBaseTest.php:316

ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml --filter testVarDump ./core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php" results in:

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml

F                                                                   1 / 1 (100%)

Time: 00:09.816, Memory: 4.00 MB

There was 1 failure:

1) Drupal\FunctionalTests\BrowserTestBaseTest::testVarDump
Failed asserting that '[]' [ASCII](length: 2) contains "BrowserTestBaseTest::testVarDump" [ASCII](length: 32).

/var/www/html/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:612

Maybe I am doing something wrong. Can you help me?

Edit: Found the solution. I forgot to update my phpunit.xml. I will review the MR.

All the code changes look good to me.
I have run the tests for the new version of dump() on my local machine and it work.
The CR is in order and I should have read it earlier.
My suggestion is to put this requirement at the top of the release notes!!!
When you do not update your local phpunit.xml file, the function just does not do anything. It will take some time to find out that you need to update your local phpunit.xml file. I have updated the IS for this.
For me it is RTBC.

I corrected spelling in the CR and other minor tweaks. Updating credit.

I read the IS and the comments. I didn't find any unanswered questions or other work to do. Tweaked the change record some more.

I keep looking at this issue and then closing the tab eventually, because it feels like a lot of work and disruption to fix the issue. But also, I don't have any alternative solution and it is annoying when a test fails only because of a dump() call - makes it harder to spot when you've fixed it.

So I think we should go ahead here - will do so in a couple of days if no-one turns up with a way to avoid the phpunit.xml change in the meantime.

Let's go ahead here. I linked to this issue and the CR in #core-development on slack for additional visibility.

Committed/pushed to 11.x, thanks!