Replace assert* involving count() and an equality operator with assertCount()

Human-readable title.

Before:

% egrep -r 'assert.+\(count\(.+\) (===|==) \d+.+\)' core | wc
      58     741   10297

After:

% egrep -r 'assert.+\(count\(.+\) (===|==) \d+.+\)' core | wc
       0       0       0

So that's good! ;)

Mostly this looks great. A few quibbles / concerns:

1. +++ b/core/modules/editor/tests/src/Functional/EditorLoadingTest.php
   @@ -274,10 +274,10 @@ public function testSupportedElementTypes() {
   -    $this->assertFalse(count($specific_format_selector) === 1, 'A single text format selector exists on the page and has the "editor" class and a "data-editor-for" attribute with the correct value.');
   +    $this->assertNotCount(1, $specific_format_selector, 'A single text format selector exists on the page and has the "editor" class and a "data-editor-for" attribute with the correct value.');
   

   This one seems weird. The change faithfully reproduces the assertions from before, but the existing assertion is pretty weird:

   A) Why not assertCount(0. ...)?

   B) The message doesn't match what the assertion is testing for.

   Seems in scope to fix this while we're fixing it. ;) But I almost never understand what's supposed to be in scope when. I vote for fixing it as part of this, but perhaps we need a follow-up for this one.

2. +++ b/core/modules/field/tests/src/Kernel/FieldStorageCrudTest.php
   @@ -215,11 +215,13 @@ public function testRead() {
   +    $this->assertTrue(isset($fields[$id]), 'The field was properly read.');
   ...
   +    $this->assertTrue(isset($fields[$id]), 'The field was properly read.');
   

   assertArrayHasKey()?
   Better message?

3. +++ b/core/modules/locale/tests/src/Functional/LocaleConfigTranslationTest.php
   @@ -158,7 +158,9 @@ public function testConfigTranslation() {
   +    $this->assertEquals($string->source, $translation->source, 'Got only one translation for image configuration.');
   +    $this->assertEquals($image_style_label, $translation->translation, 'Got only one translation for image configuration.');
   

   Better messages for these since they're now independent assertions? Otherwise, we need a follow-up to fix these. Again, I vote fix now, not later. ;)

4. +++ b/core/modules/node/tests/src/Functional/NodeLoadMultipleTest.php
   @@ -50,13 +50,11 @@ public function testNodeMultipleLoad() {
   -    $this->assertTrue($count == 2, new FormattableMarkup('@count nodes loaded.', ['@count' => $count]));
   +    $this->assertCount(2, $nodes);
   ...
   -    $this->assertTrue(count($nodes) == 3, new FormattableMarkup('@count nodes loaded', ['@count' => $count]));
   +    $this->assertCount(3, $nodes);
   

   +1 to not replicating these weirdly complicated assertion messages in the replacement code. :)

5. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -296,7 +296,7 @@ public function testBreadCrumbs() {
   -      $this->assertTrue(count($elements) == 1, "Link to {$link_path} appears only once.");
   +      $this->assertCount(1, $elements, "Link to {$link_path} appears only once.");
   

   Out of scope, but I wonder why this uses {$link_path} directly inside a " string, instead of the new FormattableMarkup() song and dance. This seems cleaner, in general. ;)

6. +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -54,7 +54,7 @@ public function assertTourTips($tips = []) {
   -          $this->assertTrue(!empty($elements) && count($elements) === 1, new FormattableMarkup('Found corresponding page element for tour tip with id #%data-id', ['%data-id' => $tip['data-id']]));
   +          $this->assertCount(1, $elements, new FormattableMarkup('Found corresponding page element for tour tip with id #%data-id', ['%data-id' => $tip['data-id']]));
   

   +1 to not separately asserting !empty() here.

7. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -317,20 +317,22 @@ public function testSchemaData() {
   +    $this->assertCount(3, $properties, 'Got the right properties for site page.');
   

   'Got the right number of properties for site page.'?

8. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -317,20 +317,22 @@ public function testSchemaData() {
   +    $this->assertSame($list['front'], $properties['front'], 'Got the right properties for site page.');
   

   'Got the right properties for front page'?

9. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -317,20 +317,22 @@ public function testSchemaData() {
   +    $this->assertCount(3, $values, 'Got the right property values for site page.');
   

   'Got the right number of properties...'?

Thanks for the review @dww!

Addressed 6.1, 6.2, 6.3, 6.7, 6.8, 6.9.

For 6.3 and 6.8 I realised it is probably better to allow PHPUnit's default assertion message to compare expected and actual values rather than try to write our own, so I removed the messages here. For counts I can't decide whether it is more useful to show a custom message or not so I have kept them for now.

Interdiff looks great. All concerns addressed. I was going to RTBC, but I noticed:

https://www.drupal.org/pift-ci-job/1655893

1 coding standards message
✗ 1 more than branch result

/var/lib/drupalci/workspace/jenkins-drupal_patches-42559/source/core/modules/node/tests/src/Functional/NodeLoadMultipleTest.php ✗ 1 more
line 5	Unused use statement

Thanks to the changes here, this is no longer needed:

use Drupal\Component\Render\FormattableMarkup;

Once we rip that out, this is RTBC.

Thanks!
-Derek

@jungle: Again, you're not supposed to RTBC your own patches. ;) Even if it's a trivial fix like this. But I break that rule sometimes, too. ;)

Anyway, +1 RTBC.

Thanks,
-Derek

Committed and pushed 3ea981a11f to 9.1.x and d9a1324911 to 9.0.x. Thanks!

Backported to 9.0.x since this is tests only.

@jungel re self-rtbc one thing that makes a big difference is if someone else has marked in rtbc before. If not then it's best to not mark an issue you've contributed patches to rtbc.

Most everything else from #3128573: [meta] Replace assertions with more appropriate ones seems to be getting backported to 8.9.x. So let's backport this, too.

Per #9, the previous patch doesn't apply cleanly, but it does apply with fuzz/offsets.

However, I just tried to cherry-pick locally, and that worked. So I don't know if uploading this patch is helpful or necessary, but here goes. ;)

Also, taking @jungle's evidence and violating my own advice, here's another self-RTBC for you. ;)

Cheers,
-Derek

p.s. Interdiff is empty, so here's a raw diff of #9 vs. this patch...

Committed 910e008 and pushed to 8.9.x. Thanks!