Use Symfony's VarDumper for easier test debugging with dump()

Doesn't this count as a regression, since it makes it harder to debug tests?

You can use debug() in a BrowserTestBase, but you can only use it once because the test stops there.

I've used Symfony's dump() (from symfony/var-dumper) in PHPUnit tests I've written for non-Drupal code (Dorgflow and Drupal Code Builder), and dump() works *great* in PHPUnit, and looks good on the command-line too. Would that be a viable alternative?

Poking around at this a bit...

It looks like we need TestHttpClientMiddleware to dump() any debug()ed output it finds in an X-Drupal-Assertion HTTP header, rather than throw an exception.

However, there's something about our tests that doesn't like dump(): this is the result of doing dump() in the actual test class code:

1) Drupal\Tests\flag\Functional\DebugTest::testDebug
PHPUnit_Framework_Exception: "dump in the test method!"
a:4:{s:10:"testResult";N;s:13:"numAssertions";i:1;s:6:"result";O:28:"PHPUnit_Framework_TestResult":26:{s:9:"*passed";a:1:{s:49:"Drupal\Tests\flag\Functional\DebugTest::testDebug";a:2:{s:6:"result";N;s:4:"size";i:-1;}}s:9:"*errors";a:0:{}s:11:"*failures";a:0:{}s:17:"*notImplemented";a:0:{}s:8:"*risky";a:0:{}s:10:"*skipped";a:0:{}s:12:"*listeners";a:0:{}s:11:"*runTests";i:1;s:7:"*time";d:50.3378918170928955078125;s:15:"*topTestSuite";N;s:15:"*codeCoverage";N;s:28:"*convertErrorsToExceptions";b:1;s:7:"*stop";b:0;s:14:"*stopOnError";b:0;s:16:"*stopOnFailure";b:0;s:42:"*beStrictAboutTestsThatDoNotTestAnything";b:1;s:33:"*beStrictAboutOutputDuringTests";b:1;s:24:"*beStrictAboutTestSize";b:0;s:34:"*beStrictAboutTodoAnnotatedTests";b:0;s:14:"*stopOnRisky";b:0;s:19:"*stopOnIncomplete";b:0;s:16:"*stopOnSkipped";b:0;s:17:"*lastTestFailed";b:0;s:23:"*timeoutForSmallTests";i:1;s:24:"*timeoutForMediumTests";i:10;s:23:"*timeoutForLargeTests";i:60;}s:6:"output";s:0:"";}

Caused by
ErrorException: unserialize(): Error at offset 0 of 1047 bytes in /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/Util/PHP.php:114
Stack trace:
#0 [internal function]: PHPUnit_Util_PHP->{closure}(8, 'unserialize(): ...', '/Users/joachim/...', 114, Array)
#1 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/Util/PHP.php(114): unserialize('"dump in the te...')
#2 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/Util/PHP.php(51): PHPUnit_Util_PHP->processChildResult(Object(Drupal\Tests\flag\Functional\DebugTest), Object(PHPUnit_Framework_TestResult), '"dump in the te...', '')
#3 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/Framework/TestCase.php(722): PHPUnit_Util_PHP->runTestJob('<?php\nif (!defi...', Object(Drupal\Tests\flag\Functional\DebugTest), Object(PHPUnit_Framework_TestResult))
#4 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php(722): PHPUnit_Framework_TestCase->run(Object(PHPUnit_Framework_TestResult))
#5 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(440): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
#6 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/TextUI/Command.php(149): PHPUnit_TextUI_TestRunner->doRun(Object(PHPUnit_Framework_TestSuite), Array)
#7 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/src/TextUI/Command.php(100): PHPUnit_TextUI_Command->run(Array, true)
#8 /Users/joachim/Sites/8-drupal/vendor/phpunit/phpunit/phpunit(52): PHPUnit_TextUI_Command::main()
#9 {main}

This is not at all what happens when I use dump() in my other projects! Is this just because Drupal is using a much older version of PHPUnit, or is Drupal doing something weird here?

> I think the problem is that Drupal is using the process isolation feature of phpunit.

I think that's where the problem is then -- any kind of output during the test causes a crash.

The process for getting data from a call to debug() in module code to the test system via HTTP headers and then the TestHttpClientMiddleware class is already there. It could definitely use some improvement, and I think using Symfony VarDumper would be good, but that should probably be a separate issue.

Hmm yeah, using dump() in one of Flag's unit tests works fine. So it's definitely a problem with BrowserTestBase...

I think the trick is going to be to use a custom dumper with Symfony's var-dumper (see https://symfony.com/doc/current/components/var_dumper/advanced.html).

We can have that write successive dump lines somewhere save (like the Drupal state? or the session?) and then retrieve them at the end of the test.

Hmm, stuffing dump lines into the state works, but at what point do we leave the PHPUnit child process so we can get them out?

Here's a quick and dirty patch which uses Symfony's dumper and sends the output out to a file, which you can 'tail -f' in another terminal window.

You sadly don't get the coloured output that the dumper would normally produce, but it's already a big improvement, as the whole test runs rather than crashes at the first dump() statement!

It's possible to do some debugging using fwrite:

fwrite(STDOUT, "variable is {$variable}");
fwrite(STDERR, "problem is {$problem}");

however this isn't an ideal, permanent solution. I've not been able to print full objects/arrays/etc - would be good to see this combined with the work Joachim has already done somehow as a lot of posts on t'interwebs seem to point to debug() being the best way forward.

@Sophie.SK's fwrite(STDOUT trick works great here -- it somehow is bypassing PHPUnit's monitoring, so PHPUnit doesn't complain that the test is failing because of the output.

It's even possible to get coloured output!

The indentation for object properties seems to be missing, but still, a huge improvement.

I've figured out the indents!

(No thanks to Symfony's total lack of code documentation...)

Ah.... symfony/var-dumper isn't installed with Drupal!

Not sure how to handle this.

Quick and dirty is to stick a class_exists() around the whole thing?

Big +1 for me.

Just tried the same trick in BrowserTestBase and it works too!

When I have some free time, I'll refactor the patch into something like a common helper class rather than anonymous functions.

New patch:

- added this to functional tests
- moved all the code to a helper class
- added class_exists() so this only runs when symfony var dumper is present.

@joachim: Your patch looks good. I am missing the functional tests.

We are adding some new functionality, therefore we should add a change record. I am not completely sure how to use the new debug method.

Do we need change records for new functionality? I thought it was mostly when an existing API is changed.

> I am not completely sure how to use the new debug method.

Install Symfony Var-Dumper with `composer require symfony/var-dumper`. Then use the dump() statement anywhere -- it can be in both test code and normal code that is run during the test.

Do we need change records for new functionality? I thought it was mostly when an existing API is changed.

OK, removing the needs change record tag.

Install Symfony Var-Dumper with `composer require symfony/var-dumper`.

Should this library not be added to Drupal Core?

> Should this library not be added to Drupal Core?

No, it would only be used for development.

I suppose we could add it to require-dev, but that's a call for maintainers.

I'm going to set this back to needs review to get the attention of maintainers.

Seems like it doesn't really work consistently.
Seem to be quietly failing to output anything for very large variables, and I don't really know why.
Potentially due to some limitations in the test framework or inside var-dumper, which I'm not even gonna attempt to debug.

It's definitely better than what we have now: nothing.
This is a lifesaver.
+1 RTBC

> Seem to be quietly failing to output anything for very large variables, and I don't really know why.

How large? I've dumped entities, which are pretty large what with all their internal bits. Also, I've dumped things that are too long for my terminal's scrollback, such as objects that had the extension manager injected, with a HUUUUUGE list of ALL the module info objects.

Maybe i'm not using it correctly then.
I'm calling dump() on some very simple entities and there's no output.
Also seems like only the first call to

dump()

actually gets written to stdout, and testbot counts it as an error when that happens.

Here's the command I'm using to run tests:
sudo -u _www php core/scripts/run-tests.sh --color --keep-results --concurrency "31" --url "https://d8.dev" --sqlite "/tmp/foo.sqlite" --dburl "mysql://root@127.0.0.1/d8" --verbose --class 'Drupal\Tests\example\TestClass'

Ah, I don't use core/scripts/run-tests.sh to run tests -- I've never managed to figure it out.

I use vendor/bin/phpunit, doing something like:

vendor/bin/phpunit web/core/modules/example/test/src/Kernel/ExampleTest.php

With that, multiple dump() statements work and they don't get counted as an error.

It sounds like this patch has no effect on run-tests.sh, as the use of dump() being counted as a test error is the problem it fixes.

Wow, thanks that's a much cleaner way to run the tests.
When i call phpunit directly, dump() works as described.

Re-rolling for latest 8.9.

I can't, for the life of me, get this interdiffed. I don't know what my computer's doing, but the only small changes are line numbers in the use statements on KernelTestBase.

Apparently that one wasn't good for 8.8.1, so re-rolling. This patch has been re-rolled against Drupal 8.8-dev. Ah, line number changes.

This is looking good! I think a quick test within KernelTestBaseTest (and perhaps BrowserTestBaseTest too) would be good to ensure expected behavior.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
--- /dev/null
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php.orig

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php.orig
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php.orig
@@ -0,0 +1,758 @@

It looks like this file slipped in by mistake.

I have re-rolled patch #39.

Yeah, #41 works really good when testing. For manual tests, I've done:

1. composer require symfony/var-dumper as suggested at #28.
2. And tested it using vendor/bin/phpunit web/core/modules/user/tests/src/Kernel/UserRoleEntityTest.php.

Result is something like this:

Drupal\user\Entity\Role^ {#2750
  #id: "test_role"
  #label: null
  #weight: null
  #permissions: []
  #is_admin: null
  #originalId: "test_role"
  #status: true
  #uuid: "52181c31-400c-4751-86dc-2a16babec77e"
  -isUninstalling: false
  #langcode: "en"
  #third_party_settings: []
  #_core: []
  #trustedData: false
  #entityTypeId: "user_role"
  #enforceIsNew: true
  #typedData: null
  #cacheContexts: []
  #cacheTags: []
  #cacheMaxAge: -1
  #_serviceIds: []
  #_entityStorages: []
  #dependencies: []
  #isSyncing: false
}

Please ignore this comment. Mixed up the naming of my patch files.

Added the Symfony component var-dump as require-dev and added testing.

Somehow the file core/tests/Drupal/Tests/TestVarDumper.php did not get added to the patch file.

Ready for a review.

Parenting - this may help get rid of the verbose legacy test method.

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

I'd suggest opening a new issue to add symfony/var-dumper as a dev dependency in core first.

Filed #3164349: Add symfony/var-dumper as a top-level dev dependency

$ composer why symfony/var-dumper
symfony/error-handler  v4.4.10  requires  symfony/var-dumper (^4.4|^5.0)  
$ composer why symfony/error-handler
drupal/drupal        9.1.x-dev  requires (for development)  symfony/error-handler (^4.4)  
symfony/http-kernel  v4.4.10    requires                    symfony/error-handler (^4.4)  

Well, symfony/var-dumper is in core already, should we add it as a top-level dev dependency? Rescoped #3164349: Add symfony/var-dumper as a top-level dev dependency.

Rerolled from #49

Fixing the failed test.

Looks very good. Some points:

1) We should take the opportunity and deprecate Drupal\KernelTests\AssertLegacyTrait::verbose and replace with this.
2)

1. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
   @@ -895,4 +895,39 @@ public function testDeprecationHeaders() {
   +  /**
   +   * Tests the dump() method provided by the Symfony component var-dump.
   +   */
   +  public function testVarDump() {
   +    // Visit a Drupal page with call to the dump() method.
   +    $body = $this->drupalGet('test-page-var-dump');
   +    $this->assertSession()->statusCodeEquals(200);
   +
   +    $this->assertStringContainsString('<span class=sf-dump-note>Drupal\user\Entity\Role</span>', $body);
   +    $this->assertStringContainsString('  #<span class=sf-dump-protected title="Protected property">id</span>: "<span class=sf-dump-str title="9 characters">test_role</span>"', $body);
   +    $this->assertStringContainsString('  #<span class=sf-dump-protected title="Protected property">label</span>: <span class=sf-dump-const>null</span>', $body);
   

   this is nice as it ensures that calls to dump within runtime code get rendered in the HTML. However, the test seems too strict to me, since if anything changes in SF rendering of the dump and/or in the Drupal\user\Entity\Role, the test will start failing. Maybe we can just check if <span class=sf-dump-note> is present in the page.

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -20,6 +20,7 @@
   +use Drupal\Tests\TestVarDumper;
   

   I think we have a better namespace for this now, Drupal\TestTools

3. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -4,9 +4,11 @@
    
    use Drupal\Component\FileCache\FileCacheFactory;
    use Drupal\Core\Database\Database;
   +use Drupal\user\Entity\Role;
    use org\bovigo\vfs\vfsStream;
    use org\bovigo\vfs\visitor\vfsStreamStructureVisitor;
    use PHPUnit\Framework\SkippedTestError;
   +use Symfony\Component\VarDumper\Test\VarDumperTestTrait;
    
    /**
     * @coversDefaultClass \Drupal\KernelTests\KernelTestBase
   @@ -17,6 +19,8 @@
   
   @@ -17,6 +19,8 @@
     */
    class KernelTestBaseTest extends KernelTestBase {
    
   +  use VarDumperTestTrait;
   +
      /**
       * @covers ::setUpBeforeClass
       */
   @@ -340,4 +344,43 @@ public function testKernelTestBaseInstallSchema() {
   
   @@ -340,4 +344,43 @@ public function testKernelTestBaseInstallSchema() {
        $this->assertFalse(Database::getConnection()->schema()->tableExists('key_value'));
      }
    
   +  /**
   +   * Tests the dump() method provided by the Symfony component var-dump.
   +   */
   +  public function testVarDump() {
   +    $this->enableModules(['system', 'user']);
   +    $role = Role::create(['id' => 'test_role']);
   +    $uuid = $role->uuid();
   +    $expectedDump = <<<EOTXT
   +Drupal\user\Entity\Role {
   +  #id: "test_role"
   +  #label: null
   +  #weight: null
   +  #permissions: []
   +  #is_admin: null
   +  #originalId: "test_role"
   +  #status: true
   +  #uuid: "$uuid"
   +  -isUninstalling: false
   +  #langcode: "en"
   +  #third_party_settings: []
   +  #_core: []
   +  #trustedData: false
   +  #entityTypeId: "user_role"
   +  #enforceIsNew: true
   +  #typedData: null
   +  #cacheContexts: []
   +  #cacheTags: []
   +  #cacheMaxAge: -1
   +  #_serviceIds: []
   +  #_entityStorages: []
   +  #dependencies: []
   +  #isSyncing: false
   +}
   +EOTXT;
   +
   +    $this->assertDumpEquals($expectedDump, $role);
   +
   +  }
   +
    }
   

   This entire test seems irrelevant to me - it will test the content of the dump structure without printing anything on the CLI, which does not mean a lot, plus suffers from the same problem above

3. So we do not have automated test coverage for the fact that the usage of dump() in test code actually prints anything on CLI. Doing a manual testing adding the below and running LoggingTest

diff --git a/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php b/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
index 4716ce10e0..53e09775c0 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
@@ -49,7 +49,8 @@ public function testEnableMultiLogging() {
 
     $queries1 = Database::getLog('testing1');
     $queries2 = Database::getLog('testing2');
-
+    dump($queries1);
+    dump($queries2);
     $this->assertCount(2, $queries1, 'Correct number of queries recorded for log 1.');
     $this->assertCount(1, $queries2, 'Correct number of queries recorded for log 2.');
   }

yields

$ ../vendor/bin/phpunit tests/Drupal/KernelTests/Core/Database/LoggingTest
PHPUnit 8.5.8 by Sebastian Bergmann and contributors.
Warning:       Invocation with class name is deprecated
Testing Drupal\KernelTests\Core\Database\LoggingTest
..
array:2 [
  0 => array:5 [
    "query" => "SELECT "name" FROM "test40640408test" WHERE "age" > :age"
    "args" => array:1 [
      ":age" => 25
    ]
    "target" => "default"
    "caller" => array:6 [
      "file" => "/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php"
      "line" => 44
      "function" => "testEnableMultiLogging"
      "class" => "Drupal\KernelTests\Core\Database\LoggingTest"
      "type" => "->"
      "args" => []
    ]
    "time" => 0.0002131462097168
  ]
  1 => array:5 [
    "query" => "SELECT "age" FROM "test40640408test" WHERE "name" = :name"
    "args" => array:1 [
      ":name" => "Ringo"
    ]
    "target" => "default"
    "caller" => array:6 [
      "file" => "/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php"
      "line" => 48
      "function" => "testEnableMultiLogging"
      "class" => "Drupal\KernelTests\Core\Database\LoggingTest"
      "type" => "->"
      "args" => []
    ]
    "time" => 0.00018000602722168
  ]
]
array:1 [
  0 => array:5 [
    "query" => "SELECT "age" FROM "test40640408test" WHERE "name" = :name"
    "args" => array:1 [
      ":name8m" => "Ringo"
    ]
    "target" => "default"
    "caller" => array:6 [
      "file" => "/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php"
      "line" => 48
      "function" => "testEnableMultiLogging"
      "class" => "Drupal\KernelTests\Core\Database\LoggingTest"
      "type" => "->"
      "args" => []
    ]
    "time" => 0.00018000602722168
  ]
]
......                                                            8 / 8 (100%)
Time: 4.55 seconds, Memory: 6.00 MB
OK (8 tests, 19 assertions)

which is what we expect, I guess.

Phpunit can test CLI output, see https://phpunit.readthedocs.io/en/9.2/writing-tests-for-phpunit.html#writing-tests-for-phpunit-output-examples-outputtest-php.

So maybe we can add an automated test for #60.3

BTW we may also want to deprecate debug() in common.inc, just using dump() seems much better. Also, will help remove in D10 from runtime code a function that is only used for development (hopefully...)

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -222,6 +224,10 @@ abstract class KernelTestBase extends TestCase implements ServiceProviderInterfa
+    if (class_exists(VarDumper::class)) {
+      VarDumper::setHandler(TestVarDumper::class . '::varDumperHandler');

If the component is an explicit dependency then class_exists is not needed.

If is is not an explicit dependency, then we can't be sure of the version being used and therefore can't be certain there is a setHandler method or what it's signature is; in which case maybe this should be wrapped in a try/catch.

> If the component is an explicit dependency then class_exists is not needed.

It'll be a dev dependency once #3164349: Add symfony/var-dumper as a top-level dev dependency is in, so we can remove the class_exists().

Our tests now assume the component is there, since there are tests that cover using dump().

Really great to see this moving forward! Thanks everyone!

Thanks @mondrake, @jonathanshaw and @joachim for reviewing!

1. 1) We should take the opportunity and deprecate Drupal\KernelTests\AssertLegacyTrait::verbose and replace with this.

   BTW we may also want to deprecate debug() in common.inc, just using dump() seems much better. Also, will help remove in D10 from runtime code a function that is only used for development (hopefully...)

   Re #60.1, not sure if we can do this, simply calling dump() in tests, PHPUnit reports errors. For instance:

     function testDumpDemo() {
       $this->assertTrue(TRUE);
       dump("demo");
     }
   

   
   Testing Drupal\KernelTests\KernelTestBaseTest
   E                                                                   1 / 1 (100%)
   "demo"
   
   
   Time: 3.1 seconds, Memory: 8.00 MB
   
   There was 1 error:
   
   1) Drupal\KernelTests\KernelTestBaseTest::testDumpDemo
   This test printed output: "demo"
   
   /Users/jungle/www/9.1.x/vendor/phpunit/phpunit/src/Framework/TestResult.php:915
   

   And if we are going to do this, I'd suggest doing it in a seperate issue.

2. #60.2 and 3 addressed, but not exactly same. See the code.

   It's hard to assert colored output, so used expectOutputRegex(), instead of expectOutputString()

   • And the example in #60.3 is a dynamic output varied by time. Can not use expectOutputString() neither.
   • Renamed Drupal\Test\TestVarDumper to Drupal\TestTools\DrupalVarDumperHandler, divided the old handler into two handlers: cliHandler() for KernelTestBase and htmlHandler() for BrowserTestBase
3. #63 addressed.

+++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
@@ -340,4 +341,42 @@ public function testKernelTestBaseInstallSchema() {
+    $this->expectOutputRegex('|#.+:.+\\\n|');

Adding two screenshots after removing/commenting all expectOutputRegex() calls in Drupal\KernelTests\KernelTestBaseTest::testVarDump and running the test.

Nice!

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -340,4 +341,42 @@ public function testKernelTestBaseInstallSchema() {
   +    $this->expectOutputRegex('|Drupal\\\\user\\\\Entity\\\\Role|');
   +    // Assert that `#foo: bar\n` like pattern presents.
   +    $this->expectOutputRegex('|#.+:.+\\\n|');
   +    $this->expectOutputRegex('|id.+test_role|');
   +    $this->expectOutputRegex('|label.+null|');
   +    $this->expectOutputRegex('|weight.+null|');
   +    $this->expectOutputRegex('|permissions.+\[\]|');
   +    $this->expectOutputRegex('|is_admin.+null|');
   +    $this->expectOutputRegex('|originalId.+test_role|');
   +    $this->expectOutputRegex('|status.+true|');
   +    $this->expectOutputRegex("|uuid.+$uuid|");
   +    // Assert that `-foo: bar\n` like pattern presents.
   +    $this->expectOutputRegex('|\-.+:.+\\\n|');
   +    $this->expectOutputRegex('|isUninstalling.+false|');
   +    $this->expectOutputRegex('|langcode.+en|');
   +    $this->expectOutputRegex('|third_party_settings.+\[\]|');
   +    $this->expectOutputRegex('|_core.+\[\]|');
   +    $this->expectOutputRegex('|trustedData.+false|');
   +    $this->expectOutputRegex('|entityTypeId.+user_role|');
   +    $this->expectOutputRegex('|enforceIsNew.+true|');
   +    $this->expectOutputRegex('|typedData.+null|');
   +    $this->expectOutputRegex('|cacheContexts.+\[\]|');
   +    $this->expectOutputRegex('|cacheTags.+\[\]|');
   +    $this->expectOutputRegex('|cacheMaxAge.+\-1|');
   +    $this->expectOutputRegex('|_serviceIds.+\[\]|');
   +    $this->expectOutputRegex('|_entityStorages.+\[\]|');
   +    $this->expectOutputRegex('|dependencies.+\[\]|');
   +    $this->expectOutputRegex('|isSyncing.+false|');
   

   I agree with #60.2.3 that this is too strict. We don't really care about the the properties of the Role, and we don't want this test to break if one of these properties gets removed or gets a new default value. It should be sufficient to test just a couple of properties like id, label, and permissions.

2. +++ b/core/tests/Drupal/TestTools/DrupalVarDumperHandler.php
   @@ -0,0 +1,46 @@
   +class DrupalVarDumperHandler {
   

   Nothing else in TestTools has its name prefixed with Drupal, might be useful to explain your reasoning here in a comment on this issue.

#3164349: Add symfony/var-dumper as a top-level dev dependency is RTBC by subsystem maintainer so removing tag added in #30 regarding that.

We now have automated tests for the dump output so removing the manual testing tag also.

Thanks @jonathanshaw for reviewing again!

1. #67.1, Removed assertions of unimportant properties, added comments explaining why asserting a few properties only. Did for testVarDump() in both KernelTestBase and BrowserTestBase
2. #67.2, Renamed back to Drupal\TestTools\TestVarDumper instead.

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -925,30 +925,14 @@ public function testVarDump() {
+    // It is too strict to assert all properties of the Role and it's easy to

Should not use both `It is` and `it's`, using one of them is better to be consistent. Addressing.

+++ b/core/tests/Drupal/TestTools/TestVarDumper.php
@@ -0,0 +1,46 @@
+    fwrite(STDOUT, "\n");
...
+          print str_repeat($indent_pad, $depth) . $line . "\n";

Self-review:

Previously, fwrite() used, and I think it should be used still. So that PHPUnit won't throw an exception, this is what we expected written in IS.

Setting back to NW.

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -340,4 +341,24 @@ public function testKernelTestBaseInstallSchema() {
   +    $uuid = $role->uuid();
   +    $this->expectOutputRegex('|Drupal\\\\user\\\\Entity\\\\Role|');
   +    // Assert that `#foo: bar\n` like pattern presents.
   +    $this->expectOutputRegex('|#.+:.+\\\n|');
   +    // It is too strict to assert all properties of the Role and it is easy to
   +    // break if one of these properties gets removed or gets a new default
   +    // value. It should be sufficient to test just a couple of properties.
   +    $this->expectOutputRegex('|id.+test_role|');
   +    $this->expectOutputRegex('|label.+null|');
   +    $this->expectOutputRegex('|permissions.+\[\]|');
   +    $this->expectOutputRegex("|uuid.+$uuid|");
   

   We can not assert output from fwrite() by using expectOutputRegex()

   But after adding dump() calls in the test, if the test passes, then the test is sufficient to me -- by default, PHPUnit treats output from dump() in tests an exception.

Addressing #70, #71, and attaching a screenshot of running the Drupal\KernelTests\KernelTestBaseTest::testVarDump() with two dump() calls inside.

In #28:

Do we need change records for new functionality? I thought it was mostly when an existing API is changed.

I think it's necessary to have a CR still. dump() calls are allowed in Kernel Tests, Functional Tests and Functional Javascript Tests. Which is a new feature and we want module developers to know and to use.

1) We should take the opportunity and deprecate Drupal\KernelTests\AssertLegacyTrait::verbose and replace with this.

BTW we may also want to deprecate debug() in common.inc, just using dump() seems much better. Also, will help remove in D10 from runtime code a function that is only used for development (hopefully...)

Let's do them in separate issues. Tagging "Needs followup"

BTW, debug() might use a custom VarDump handler as KernelTestBase and BrowserTestBase, If so, Drupal\TestTools\TestVarDumper should be moving to other namespace probably if necessary. That's why I renamed it to DrupalVarDumperHandler previously.

after adding dump() calls in the test, if the test passes, then the test is sufficient to me -- by default, PHPUnit treats output from dump() in tests an exception.

I think a test should fail or be marked as risky after execution with "This test printed output" if it invoked dump(). This helps to guard against accidentally leaving debug calls in code. If people really want the dump to stay permanently, they can opt out of phpunit's beStrictAboutTestsThatPrintOutput.

What's the behaviour with the patch as it is?

I think we need to be clear what would be the effects of using dump() in the various contexts:

1) use of dump() in the SUT (i.e. in the runtime code) in Functional and FunctionalJavascript tests - where is the output going?
2) the same for Kernel tests - both when run in isolation and not
3) what would happen if a dump() is called in the context of a Unit or Build test?
4) use of dump() in the test code - where is the output going, both in isolation and not

So to be aligned on scope and what can be tested or not

Made an issue fork and a new branch, 2795567-symfony-dump-kernel-browser-tests.

- committed the most recent patch at #72
- changed the kernel test so that the dump() output is tested, and so it doesn't actually output anything (which could cause problems for test runners that look at output, and also surprise/confuse developers)

I can't get BrowserTestBaseTest::testVarDump() to work.

The call to $body = $this->drupalGet('test-page-var-dump'); is getting a page that's redirecting to install.php.

I tried adding $this->drupalGet('test-page'); to see if an existing test page works, and that does it too.

BrowserTestBaseTest::testDrupalGetHeader() is NOT working correctly, so I don't think it's IT IS my local setup.

(EDIT. ARGH)

> I think we need to be clear what would be the effects of using dump() in the various contexts:

Yes, good point.

This should probably be documented too!

> 1) use of dump() in the SUT (i.e. in the runtime code) in Functional and FunctionalJavascript tests - where is the output going?

Into the web page that the SUT is serving, and so into the HTML output that the test generates.

It's basically similar to installing Devel module and using dpm()

> 2) the same for Kernel tests - both when run in isolation and not

In Kernel tests, dump() in both the test code and the SUT will product output on the command line where the test was run.

> 3) what would happen if a dump() is called in the context of a Unit or Build test?

In a Unit test, dump() will product output on the command line where the test was run.

> 4) use of dump() in the test code - where is the output going, both in isolation and not

In Unit and Kernel test code, dump() produces output on the command line where the test was run.

In Browser tests, it's currently causing a test failure. Fix for that coming!

Latest changes:

- Made dump() working in the test code of browser tests
- Added documentation on how dump() works in kernel and browser tests

I think this is ready for review!

cspell didn't seem to like the latest patch

The cspell errors are all false positives.

How do we get Cspell to accept these?

> /var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:69:21 - Unknown word (Dumper's)
> /var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:41:21 - Unknown word (Dumper's)

'Using Symfony VarDumper's dump()' should be fine. That's just a possessive on a proper noun.

      $consumed += $bucket->datalen;
      stream_bucket_append($out, $bucket);
    }
    return PSFS_FEED_ME;

datalen is a property of a PHP class. We can't change that.

PSFS is part of a PHP constant name.

Ah, I see how to do it. I've added those words to the cspell dictionary.

> If people really want the dump to stay permanently, they can opt out of phpunit's beStrictAboutTestsThatPrintOutput.

@jonathanshaw I assume you mean beStrictAboutOutputDuringTests? That's already enabled by default. The technique we use here to get dump() output to the command line bypasses PHPUnit's checks for output, because AFAIK if we don't, then the dump() output gets caught up in other stuff and it's not usable.

In a Unit test, dump() will produce output on the command line where the test was run.

Really? We have no documentation or test coverage of this, and it's not clear to me why it would be true given that we haven't touched UnitTestCase.

I just tried it, with a dump() both in the unit test method, and in a class being tested. dump() goes straight to the terminal with no errors or complaints.

This is just speculation, but do unit run without a child process? My vague impression is that the problem with dump() in tests is that it gets sucked up into the communication between the test process and the SUT process, but I don't actually understand how it works at all!

Should we add a test for dump() in unit tests?

Should we add a test for dump() in unit tests?

I think so.

do unit run without a child process

I'm not aware that Kernel use a child process either.

> I'm not aware that Kernel use a child process either.

There is definitely something very different between Unit and Kernel tests because *without* the work from this issue, dump() behaves very differently:

Unit test:

Testing Drupal\Tests\UnitTestCaseTest
^ "banana"
R                                                                   1 / 1 (100%)Rcake

Kernel test:

Testing Drupal\KernelTests\KernelTestBaseTest
E                                                                   1 / 1 (100%)R

Time: 1.76 seconds, Memory: 8.00 MB

There was 1 error:

1) Drupal\KernelTests\KernelTestBaseTest::testBootEnvironment
PHPUnit\Framework\Exception: "banana"
a:4:{s:10:"testResult";N;s:13:"numAssertions";i:4;s:6:"result";O:28:"PHPUnit\Framework\TestResult":35:{s:36:"PHPUnit\Framework\TestResultpassed";a:1:{s:58:"Drupal\KernelTests\KernelTestBaseTest::testBootEnvironment";a:2:{s:6:"result";N;s:4:"size";i:-1;}}s:36:"PHPUnit\Framework\TestResulterrors";a:0:{}s:38:"PHPUnit\Framework\TestResultfailures";a:0:{}s:38:"PHPUnit\Framework\TestResultwarnings";a:0:{}s:44:"PHPUnit\Framework\TestResultnotImplemented";a:0:{}s:35:"PHPUnit\Framework\TestResultrisky";a:0:{}s:37:"PHPUnit\Framework\TestResultskipped";a:0:{}s:39:"PHPUnit\Framework\TestResultlisteners";a:0:{}s:38:"PHPUnit\Framework\TestResultrunTests";i:1;s:34:"PHPUnit\Framework\TestResulttime";d:0.6979920864105225;s:42:"PHPUnit\Framework\TestResulttopTestSuite";N;s:42:"PHPUnit\Framework\TestResultcodeCoverage";N;s:61:"PHPUnit\Framework\TestResultconvertDeprecationsToExceptions";b:1;s:55:"PHPUnit\Framework\TestResultconvertErrorsToExceptions";b:1;s:56:"PHPUnit\Framework\TestResultconvertNoticesToExceptions";b:1;s:57:"PHPUnit\Framework\TestResultconvertWarningsToExceptions";b:1;s:34:"PHPUnit\Framework\TestResultstop";b:0;s:41:"PHPUnit\Framework\TestResultstopOnError";b:0;s:43:"PHPUnit\Framework\TestResultstopOnFailure";b:0;s:43:"PHPUnit\Framework\TestResultstopOnWarning";b:0;s:69:"PHPUnit\Framework\TestResultbeStrictAboutTestsThatDoNotTestAnything";b:1;s:60:"PHPUnit\Framework\TestResultbeStrictAboutOutputDuringTests";b:1;s:61:"PHPUnit\Framework\TestResultbeStrictAboutTodoAnnotatedTests";b:0;s:72:"PHPUnit\Framework\TestResultbeStrictAboutResourceUsageDuringSmallTests";b:0;s:46:"PHPUnit\Framework\TestResultenforceTimeLimit";b:0;s:50:"PHPUnit\Framework\TestResulttimeoutForSmallTests";i:1;s:51:"PHPUnit\Framework\TestResulttimeoutForMediumTests";i:10;s:50:"PHPUnit\Framework\TestResulttimeoutForLargeTests";i:60;s:41:"PHPUnit\Framework\TestResultstopOnRisky";b:0;s:46:"PHPUnit\Framework\TestResultstopOnIncomplete";b:0;s:43:"PHPUnit\Framework\TestResultstopOnSkipped";b:0;s:44:"PHPUnit\Framework\TestResultlastTestFailed";b:0;s:46:"PHPUnit\Framework\TestResultdefaultTimeLimit";i:0;s:42:"PHPUnit\Framework\TestResultstopOnDefect";b:0;s:77:"PHPUnit\Framework\TestResultregisterMockObjectsFromTestArgumentsRecursively";b:0;}s:6:"output";s:0:"";}

/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:288
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:171
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:627
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/TextUI/Command.php:204
/Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/TextUI/Command.php:163

Caused by
ErrorException: unserialize(): Error at offset 0 of 2335 bytes in /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:272
Stack trace:
#0 [internal function]: PHPUnit\Util\PHP\AbstractPhpProcess::PHPUnit\Util\PHP\{closure}(8, 'unserialize(): ...', '/Users/joachim/...', 272, Array)
#1 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php(272): unserialize('"banana"\na:4:{s...')
#2 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php(171): PHPUnit\Util\PHP\AbstractPhpProcess->processChildResult(Object(Drupal\KernelTests\KernelTestBaseTest), Object(PHPUnit\Framework\TestResult), '"banana"\na:4:{s...', '')
#3 /Users/joachim/Sites/drupal-core-composer/repos/drupal/sites/simpletest/TestCase.php(761): PHPUnit\Util\PHP\AbstractPhpProcess->runTestJob('<?php\nuse PHPUn...', Object(Drupal\KernelTests\KernelTestBaseTest), Object(PHPUnit\Framework\TestResult))
#4 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/Framework/TestSuite.php(597): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
#5 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(627): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#6 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/TextUI/Command.php(204): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, Array, true)
#7 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/src/TextUI/Command.php(163): PHPUnit\TextUI\Command->run(Array, true)
#8 /Users/joachim/Sites/drupal-core-composer/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#9 {main}
--

> Should we add a test for dump() in unit tests?

I can't actually get a test for this to work!

Using the same technique as the Kernel test to capture the output doesn't work -- the dump() output is still visible, and the capture has nothing.

So I tried ob_start(). That doesn't work either!

I can't figure out which output stream is getting the dump() output in Unit tests.

Given that we are not changing anything to do with Unit tests in this patch, I would suggest that adding a test for dump() behaviour in unit tests could be left to a follow-up.

It would be really nice to get this in -- I think it makes a huge difference to DX for writing tests!

I can't RTBC until we've documented answers to #77 in the IS. Most of these points we've discussed, but some aren't clear to me.

@jonathanshaw updated the IS. Is this the sort of thing that's needed?

Yes, but #77 asks about isolation

Sorry, didn't mean to rtbc

My vague understanding was that ALL kernel tests run in isolation, and that's why we get the serialization crash if we try to use dump() in them at the moment.

Is there any documentation about how Drupal tests use PHPUnit process isolation functionality? I can't find very much about it.

Ah yes sorry, Kernel and Functional always run in isolation, my mistake. They extend from PHPUnit TestCase and set

protected $runTestInSeparateProcess = TRUE;

in the base classes, so that PHPUnit knows it will have to spawn a subprocess to execute the test.

The point of #77 (I should have been more clear) is that for Unit tests, they normally run in the same process where PHPUnit is executing, UNLESS you specify differently (setting the protected property or using the annotation @runTestsInSeparateProcesses. See for example Drupal\Tests\Core\Site\SettingsTest.

So, using dump() in a Unit test that is set to run in isolation, what happens vs. a 'normal' Unit test?

Thanks for the explanation! (And I'm glad that matches with my vague understanding!!)

I'll investigate unit tests run in isolation.

After this, we might go back to #2875038: AssertLegacyTrait::verbose() doesn't do anything and maybe just deprecate AssertLegacyTrait::verbose() and remove its usages.

> So, using dump() in a Unit test that is set to run in isolation, what happens vs. a 'normal' Unit test?

The answer was that in an isolated unit test, dump() wasn't working -- it was crashing in the same was as in #90.

New commits fix that. As a byproduct, testing dump() in non-isolated unit tests works too!

@joachim Could you add these notes on isolation to the IS?

Done.

Seems like we're there.

Added a CR. See: https://www.drupal.org/node/3192283.

Thanks! That's great!

I've given it a little bit of a tweak.

Works nicely! Thanks for working on this, RTBC++

NW for lowercasing the words in the dictionary, or/and adding a comment for VarDumper::setHandler(TestVarDumper::class . '::cliHandler'); in UnitTestCase explaining why it's not being called in setUpBeforeClass(). Thanks!

> lowercasing the words in the dictionary

Could you explain more please? I followed the instructions at https://www.drupal.org/node/3122084, which don't mention the case of words.

> explaining why it's not being called in setUpBeforeClass().

Good catch. It should be in setUpBeforeClass(), as it only needs to be done once for all tests in a class. Will fix.

Also while investigating the cSpell configuration I've realised that our dictionary should be all lowercase. cSpell is case insensitive and all their dictionaries are lowercase too.

@joachim, see @alexpott's comment on the original issue here, please. That CR needs updating, then. -- CR updated (edited)

Fixed both of the issues in #107, and rebased the branch.

@joachim: You have merged the rebase and the fixes in one update. The problem with that as a reviewer there is now no interdiff.txt equavalent to easily review the changes that you made. I get an interdiff.txt with 251 files with +2056 lines added and -1164 lines removed.

Maybe, here worthy a comment, as we set the handler in setUpBeforeClass() in KernelTestBase.

@jungle asked for adding a comment, only one was not added.

@daffie, probably, you can go to the commit tab on gitlab to review the latest 2 commits :), which is easier to review.

> Maybe, here worthy a comment, as we set the handler in setUpBeforeClass() in KernelTestBase.

The context was:

The handler in KernelTestBase was set in its setUpBeforeClass(), while the handler in UnitTestCase was set in its setup(). The same handler was set in different methods, that's why I asked for a comment.

In @joachim's latest commit, the set handler code for UnitTestCase was moved into setUpBeforeClass(), now, no more difference. So to me, a comment is unnecessary now.

> Surprised this is a word in a dictionary :)

PSFS_FEED_ME is a word in the dictionary still, just lowercased to psfs_feed_me. I have not seen any valid word has _. I believe there is something wrong, still. or needs a follow up to figure it out.

$ yarn spellcheck:core
yarn run v1.22.10
$ cspell "**/*" ".*" "../composer/**/*" "../composer.json"
/Users/jungle/ran/drupal/9.2.x/core/tests/Drupal/KernelTests/KernelTestBase.php:69:21 - Unknown word (Dumper's)
/Users/jungle/ran/drupal/9.2.x/core/tests/Drupal/Tests/BrowserTestBase.php:41:21 - Unknown word (Dumper's)
/Users/jungle/ran/drupal/9.2.x/core/tests/Drupal/Tests/StreamCapturer.php:15:29 - Unknown word (datalen)
/Users/jungle/ran/drupal/9.2.x/core/tests/Drupal/Tests/StreamCapturer.php:18:12 - Unknown word (PSFS)
/Users/jungle/ran/drupal/9.2.x/core/tests/Drupal/Tests/UnitTestCase.php:21:21 - Unknown word (Dumper's)

Checked locally, psfs is a new word, not PSFS_FEED_ME/psfs_feed_me. Please change psfs_feed_me in the dictionary to psfs

@daffie, probably, you can go to the commit tab on gitlab to review the latest 2 commits :), which is easier to review.

To correct, it's the Commits tab on the MR page, not commit tab. Sorry.

> @joachim: You have merged the rebase and the fixes in one update

Sorry... I figured it would be less confusing rather than adding commits that would immediately be obsolete. I didn't think of the need to see an interdiff.

> Please change psfs_feed_me in the dictionary to psfs

Done.

Thanks for your guidance on cspell configuration @jungle!

It looks ready to me unless the bot rejects it. Thanks!

Filed #3193163: Deprecate AssertLegacyTrait::verbose and remove its usage

Needs a reroll.

Rerolled, patch context changes only.

Added a code review to gitlab...

Addressed all the points from @alexpott's review & rebased on 9.2.x.

Back to RTBC then, thanks.

Committed f7dd26a and pushed to 9.2.x. Thanks!

As per @mondrake's suggestion let's add a follow-up to deprecate common.inc's debug() function.

Fixed some things on commit.

diff --git a/core/modules/system/tests/modules/test_page_test/src/Controller/TestPageTestController.php b/core/modules/system/tests/modules/test_page_test/src/Controller/TestPageTestController.php
index 423d144c4f..f23fc92cf5 100644
--- a/core/modules/system/tests/modules/test_page_test/src/Controller/TestPageTestController.php
+++ b/core/modules/system/tests/modules/test_page_test/src/Controller/TestPageTestController.php
@@ -26,7 +26,7 @@ public function testPage() {
   }
 
   /**
-   * Returns a test page and with the call to the dump() method.
+   * Returns a test page and with the call to the dump() function.
    */
   public function testPageVarDump() {
     $role = Role::create(['id' => 'test_role']);
diff --git a/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
index b84a0e30d1..10489ebb5f 100644
--- a/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -952,7 +952,7 @@ public function testDrupalGetHeader() {
   }
 
   /**
-   * Tests the dump() method provided by the Symfony component var-dump.
+   * Tests the dump() function provided by the var-dumper Symfony component.
    */
   public function testVarDump() {
     // Append the stream capturer to the STDOUT stream, so that we can test the
@@ -970,7 +970,7 @@ public function testVarDump() {
     $this->assertStringContainsString('Drupal\user\Entity\Role', StreamCapturer::$cache);
     $this->assertStringContainsString('authenticated', StreamCapturer::$cache);
 
-    // Visit a Drupal page with call to the dump() method to check that dump()
+    // Visit a Drupal page with call to the dump() function to check that dump()
     // in site code produces output in the requested web page's HTML.
     $body = $this->drupalGet('test-page-var-dump');
     $this->assertSession()->statusCodeEquals(200);
diff --git a/core/tests/Drupal/KernelTests/KernelTestBase.php b/core/tests/Drupal/KernelTests/KernelTestBase.php
index 7f958037d2..1c520289de 100644
--- a/core/tests/Drupal/KernelTests/KernelTestBase.php
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -66,7 +66,7 @@
  * KernelTestBase::installEntitySchema(). Alternately, tests which need modules
  * to be fully installed could inherit from \Drupal\Tests\BrowserTestBase.
  *
- * Using Symfony's dump() function() in Kernel tests will produce output on the
+ * Using Symfony's dump() function in Kernel tests will produce output on the
  * command line, whether the call to dump() is in test code or site code.
  *
  * @see \Drupal\Tests\KernelTestBase::$modules
diff --git a/core/tests/Drupal/KernelTests/KernelTestBaseTest.php b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
index 9c6f7de614..025c5a490b 100644
--- a/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
+++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
@@ -399,7 +399,7 @@ public function testKernelTestBaseInstallSchema() {
   }
 
   /**
-   * Tests the dump() method provided by the Symfony component var-dump.
+   * Tests the dump() function provided by the var-dumper Symfony component.
    */
   public function testVarDump() {
     // Append the stream capturer to the STDOUT stream, so that we can test the
diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php
index bb41d8b8d9..8da29400b4 100644
--- a/core/tests/Drupal/Tests/BrowserTestBase.php
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -38,7 +38,7 @@
  * translation functionality. For example, avoid wrapping test text with t()
  * or TranslatableMarkup().
  *
- * Using Symfony's dump() function() in functional test test code will produce
+ * Using Symfony's dump() function in functional test test code will produce
  * output on the command line; using dump() in site code will produce output in
  * the requested web page, which can then be inspected in the HTML output from
  * the test.
diff --git a/core/tests/Drupal/Tests/UnitTestCaseTest.php b/core/tests/Drupal/Tests/UnitTestCaseTest.php
index a264688d49..aac3b709c2 100644
--- a/core/tests/Drupal/Tests/UnitTestCaseTest.php
+++ b/core/tests/Drupal/Tests/UnitTestCaseTest.php
@@ -20,7 +20,7 @@ public function testAssertArrayEquals() {
   }
 
   /**
-   * Tests the dump() method in a test run in the same process.
+   * Tests the dump() function in a test run in the same process.
    */
   public function testVarDumpSameProcess() {
     // Append the stream capturer to the STDOUT stream, so that we can test the
@@ -41,7 +41,7 @@ public function testVarDumpSameProcess() {
   }
 
   /**
-   * Tests the dump() method in a test run in a separate process.
+   * Tests the dump() function in a test run in a separate process.
    *
    * @runInSeparateProcess
    */

dump() is a function not a method.
dump() function() is a lot of brackets.

Existing issue for #123 follow-up #2002514-56: Deprecate debug(); remove references to _drupal_debug_message().

> dump() function() is a lot of brackets.

Oops!

I've published the draft CR.

Ran into the stream wrapping here triggering an edge case where symfony's deprecation handler triggers a php warning and leading to false positive failures.

Opened a pull with symfony.
https://github.com/symfony/symfony/pull/43149

Two followups were asked for in #74.

1) We should take the opportunity and deprecate Drupal\KernelTests\AssertLegacyTrait::verbose and replace with this.

2)BTW we may also want to deprecate debug() in common.inc, just using dump() seems much better. Also, will help remove in D10 from runtime code a function that is only used for development (hopefully...)

The first is #3193163: Deprecate AssertLegacyTrait::verbose and remove its usage
The second is #2002514: Deprecate debug(); remove references to _drupal_debug_message().

I am removing the tag.