Problem/Motivation

As title

Proposed resolution

Recommended regex for searching

assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#78 raw-interdiff-72-78.txt2.08 KBjungle
#78 3126965-78.patch410.15 KBjungle
#72 interdiff-70-72.txt1.26 KBjungle
#72 3126965-72.patch410.11 KBjungle
#70 interdiff-69-70.txt2.49 KBjungle
#70 3126965-70.patch411.37 KBjungle
#69 raw-interdiff-67-69.txt1.67 KBjungle
#69 3126965-69.patch413.86 KBjungle
#67 raw-interdiff-62-66.txt43.37 KBjungle
#67 3126965-66.patch413.88 KBjungle
#62 interdiff-48-62.txt8.99 KBjungle
#62 3126965-62.patch398.96 KBjungle
#54 rawdiff-8.8.x-8.9.x-54-54.txt53.15 KBjungle
#54 interdiff-8.8.x-51-54.txt3.25 KBjungle
#54 3126965-8.9.x-54.patch418.11 KBjungle
#54 3126965-8.8.x-54.patch438.9 KBjungle
#51 3126965-51.patch441.09 KBjungle
#48 interdiff-47-48.txt2.67 KBjungle
#48 3126965-48.patch399.09 KBjungle
#47 3126965-47.patch401.46 KBjungle
#47 raw-interdiff-19-47.txt20.72 KBjungle
#37 interdiff-34-37.txt21.93 KBjungle
#37 3126965-37.patch406.46 KBjungle
#34 raw-interdiff-29-34.txt19.46 KBjungle
#34 3126965-34.patch389.54 KBjungle
#29 3126965-29.patch390.14 KBjungle
#29 interdiff-26-29.txt16.11 KBjungle
#26 interdiff.txt1.38 KBquietone
#26 3126965-26.patch393.64 KBquietone
#23 3126965-23.patch393.64 KBquietone
#23 interdiff-22-23.txt683 bytesquietone
#22 3126965-22.patch393.64 KBquietone
#22 interdiff-21-22.txt632 bytesquietone
#21 3126965-21.patch393.64 KBquietone
#21 interdiff-19-21.txt201.09 KBquietone
#19 3126965-19.patch402.38 KBquietone
#19 diff-18-91.txt4.99 KBquietone
#18 interdiff-17-18.txt1.74 KBjungle
#18 3126965-18.patch403.53 KBjungle
#17 interdiff-16-17.txt900 bytesjungle
#17 3126965-17.patch403.52 KBjungle
#16 3126965-16.patch403.52 KBjungle
#8 3126965-8.patch59.87 KBlongwave
#3 3126965-3.patch13.1 KBjungle
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle’s picture

Status: Active » Needs review
FileSize
13.1 KB
mondrake’s picture

Status: Needs review » Needs work

Nice, but I think the scope should be larger: try

assert(?:Same|Identical|Equal|Equals)\(.*count\(

and there's much more to convert.

At least here, let's convert assertSame and assertIdentical, which are equivalent. Note also that the order of expected and actual may be swapped.

jungle’s picture

jungle’s picture

Issue summary: View changes

@mondrake, Thank you! let's enlarge the scope.

jungle’s picture

Title: Replace assertSame(*, count()) with assertCount() » Replace assert*(*, count()) with assertCount()
longwave’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
59.87 KB

Done a few more of these, but a rough grep shows there are about another 1000 to check, most of which need changing. Bumped the version to 9.1.x as I think this will have to be done there first and then backported, and there are several differences between the branches now.

Discovered #3128773: AssertButtonsTrait is unused while looking at this as well, going to leave that one alone as we can't test it.

longwave’s picture

Actually I realise I checked more than the regex in #4 - there are a lot of assertTrue(count(...) == ...) as well, and then things like this:

-    $this->assertTrue(count($cache_items) >= 2);
+    $this->assertGreaterThanOrEqual(2, count($cache_items));

Should we narrow this down a bit to make it easier to review?

jungle’s picture

Title: Replace assert*(*, count()) with assertCount() » Replace assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\) with assertCount()

Then let's break this into 4 subtasks.

  1. assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\), examples:

    -    $this->assertSame(0, count($processed_placeholders));
    +    $this->assertCount(0, $processed_placeholders);
    -    $this->assertEqual(count($result), 0, 'No autocompletion without access user profiles.');
    +    $this->assertCount(0, $result, 'No autocompletion without access user profiles.');
    
  2. assert.+\(count\(.+\), count\(.+\)(|.+), examples:
    -    $this->assertEqual(count($options), count($expected_nids));
    +    $this->assertCount(count($options), $expected_nids);
    -    $this->assertEqual(count($options), count($expected_nids), 'messge');
    +    $this->assertCount(count($options), $expected_nids, 'message');
    
  3. assert.+\(count\(.+\) (===|==) \d+(|.+)\), examples:
    -    $this->assertTrue(count($body) == 1, 'A body field exists.');
    +    $this->assertCount(1, $body, 'A body field exists.');
    -    $this->assertTrue(count($body) === 1, 'A body field exists.');
    +    $this->assertCount(1, $body, 'A body field exists.');
    -    $this->assertTrue(count($body) == 1);
    +    $this->assertCount(1, $body);
    -    $this->assertTrue(count($body) === 1);
    +    $this->assertCount(1, $body);
    
  4. assert.+\(count\(.+\) (>=|<=) \d+\), example:
    -    $this->assertTrue(count($cache_items) >= 2);
    +    $this->assertGreaterThanOrEqual(2, count($cache_items));
    

Rescope this one to focus on replacing assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)

jungle’s picture

Status: Needs review » Needs work
dww’s picture

Title: Replace assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\) with assertCount() » Replace assert* involving count() and an integer literal with assertCount()

Based on Slack thread, giving this a human-readable title.

jungle’s picture

Assigned: Unassigned » jungle
jungle’s picture

Assigned: jungle » Unassigned
Status: Needs work » Needs review
FileSize
403.52 KB

Coming a big patch

jungle’s picture

Status: Needs review » Needs work
FileSize
403.52 KB
900 bytes

Fixed PHPLint error but setting back to NW for removing messages.

jungle’s picture

Fixing failed CI run in #17

quietone’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
402.38 KB

Needs a reroll.

quietone’s picture

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

NW for removing message

quietone’s picture

Status: Needs work » Needs review
FileSize
201.09 KB
393.64 KB

I've had a few interruptions while working on this so I won't be surprised if there are errors.

Messages were left in this files because there were in a for loop.

  • core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
  • core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetTest.php
  • core/modules/options/tests/src/Functional/OptionsDynamicValuesValidationTest.ph
  • core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php
  • core/modules/system/tests/src/Functional/Database/SelectPagerDefaultTest.php
  • core/modules/system/tests/src/Functional/Form/ElementsTableSelectTest.php
  • core/modules/text/tests/src/Functional/TextFieldTest.php
  • core/modules/views/tests/src/Functional/Handler/FieldEntityOperationsTest.php
  • core/modules/views/tests/src/Functional/Handler/FieldEntityOperationsTest.php
  • core/modules/views_ui/tests/src/Functional/HandlerTest.php
  • core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
  • core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php

Messages were left in these files as well, on a first look the message contains a variable and looks useful.

  • core/modules/comment/tests/src/Kernel/CommentValidationTest.php
  • core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAdminTest.php
  • core/modules/system/tests/src/Functional/Theme/ThemeInfoTest.php
  • core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.p
  • core/tests/Drupal/KernelTests/Core/Menu/MenuTreeStorageTest.php
quietone’s picture

Oops, lost a semicolon.

quietone’s picture

Another missing semi colon. (In my defense since I updated PhpStorm the editor hasn't been the same.) Lets try one more time.

jungle’s picture

Well, I am inserting a CONTINUE statement for the next iteration, good luck! 💪

jungle’s picture

Status: Needs review » Needs work
quietone’s picture

Status: Needs work » Needs review
FileSize
393.64 KB
1.38 KB

@jungle, thanks. Must be the price I pay for upgrading PhpStorm and not taking a walk today.

And again!! This time I didn't rely on phpStorm.

jungle’s picture

Assigned: quietone » jungle

Thanks @quietone! Taking over to continue, found things to do while reviewing.

quietone’s picture

@jungle, thanks. I mean to unassign myself before I went to bed and forgot.

jungle’s picture

Assigned: jungle » Unassigned
FileSize
16.11 KB
390.14 KB
jungle’s picture

+++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
@@ -979,7 +979,7 @@ public function testGetIndividual() {
-    $this->assertGreaterThanOrEqual(2, count($cache_items));
+    $this->assertTrue(count($cache_items) >= 2);

+++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
@@ -114,7 +114,7 @@ public function testFailedPageCreation() {
-    $this->assertGreaterThan(0, count($records));
+    $this->assertTrue(count($records) > 0, 'Rollback explanatory error logged to watchdog.');

Continue to #29, reverted out of scope changes, and removed more redundant ones. By reviewing #26, confirmed that assertion messages in loops have not been touched -- that's good.

Thanks!

longwave’s picture

Incomplete review as the patch is so big, but some thoughts so far:

  1. +++ b/core/modules/aggregator/tests/src/Kernel/FeedValidationTest.php
    @@ -39,7 +39,7 @@ public function testValidation() {
         $violations = $feed->validate();
    -    $this->assertEqual(count($violations), 0);
    +    $this->assertCount(0, $violations);
    

    There are a lot of assertCount(0, ...) in here. Would these be better as assertEmpty()? This would give more helpful error messages on failure.

  2. +++ b/core/modules/book/tests/src/Functional/BookTest.php
    @@ -300,13 +300,13 @@ public function testGetTableOfContents() {
    -    $this->assertEqual(count($options), count($expected_nids));
    +    $this->assertCount(count($options), $expected_nids);
    

    This one feels better as assertEquals, as we are comparing two counts?

  3. +++ b/core/modules/book/tests/src/Functional/BookTest.php
    @@ -300,13 +300,13 @@ public function testGetTableOfContents() {
    -    $this->assertEqual(count($options), count($expected_nids));
    +    $this->assertCount(count($options), $expected_nids);
    

    Same here.

dww’s picture

@longwave Re: #31:

1. +1. assertEmpty() indeed seems better than assertCount(0)

2 + 3: Indeed. I thought we already had that discussion at #3128813: Replace assertEqual() or assertSame() on two calls to count() with assertCount() and agreed to leave them as assertEqual().

However, #NeedsFollowup?

$this->assertEqual(count($options), count($expected_nids));

It's supposed to be:

assertEqual($expected, $actual)

So really the above should be:

$this->assertEqual(count($expected_nids), count($options));

Seems that'll be impossible to grep for, and therefore, it'll be a huge PITA to actually fix those. :/

Thoughts?

Thanks/sorry,
-Derek

jungle’s picture

Assigned: Unassigned » jungle

On this

jungle’s picture

Rerolled a patch from #29 first.

jungle’s picture

Thanks @longwave and @dww for reviewing!

  1. #31.1/#32.1 I would like keep them untouched, as the patch is already too big, and hard to review already, I'd suggest doing it in a separate issue if necessary
  2. #31.2,3/#32.2, for $this->assert*(count(), count()); ones. there is an issue filed for that, closed it because of the original one has a better readability, and it's out of the rescoped scope here, I will check it to see if there are any changes like that in the patch and revert them. Reopen/rescope #3128813 if necessary

What I am going to do next:

  1. Revert assert*(count(), count()); relevant changes if there are any.
  2. Check if there are assertions missed and replace them if found.
mondrake’s picture

#35 let's file a follow up to convert assertCount(0, ...) to assertEmpty(...) - it should be checked case by case.

jungle’s picture

Assigned: jungle » Unassigned
FileSize
406.46 KB
21.93 KB

Replaced more, and reverted $this->assert*(count(), count()); ones found

TR’s picture

However, #NeedsFollowup?

$this->assertEqual(count($options), count($expected_nids));

It's supposed to be:

assertEqual($expected, $actual)

Actually, you got that backwards.
The old Simpletest assertEqual() uses ($actual, $expected). That method now lives in AssertLegacyTrait.

The new assertEquals() with an 's' has the arguments reversed - ($expected, $actual)

I think the confusion over this and the need to reverse the arguments is why core bailed on removing the old assertEqual() from D9.0. That change is now deferred until D10.0.

AssertLegacyTrait::assertEqual() looks like this:

protected function assertEqual($actual, $expected, $message = '') {
  $this->assertEquals($expected, $actual, (string) $message);
}
longwave’s picture

Raised #3133291: Add XPath existence assertions as a lot of these are looking for the existence of XPath elements, and I think we can do better.

dww’s picture

@TR re: #38 wow, confusing! ;) Thanks for clarifying. I didn't even notice the Equal vs. Equals. I'm just used to PHPUnit style assertions, where expected is always first. Too bad we did it backwards in SimpleTestLandia. What a mess. I'll be sure to pay more attention to Equal vs. Equals in the future. ;)

mondrake’s picture

mondrake’s picture

Some review:

  1. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
    @@ -392,7 +392,7 @@ protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholde
    -    $this->assertEqual(count($expected_big_pipe_placeholders_with_replacements), preg_match_all('/' . preg_quote('<script type="application/vnd.drupal-ajax" data-big-pipe-replacement-for-placeholder-with-id="', '/') . '/', $this->getSession()->getPage()->getContent()));
    +    $this->assertCount(preg_match_all('/' . preg_quote('<script type="application/vnd.drupal-ajax" data-big-pipe-replacement-for-placeholder-with-id="', '/') . '/', $this->getSession()->getPage()->getContent()), $expected_big_pipe_placeholders_with_replacements);
    

    This is very hard to read. I suggest to split in two,

    +    $expected_count = preg_match_all('/' . preg_quote('<script type="application/vnd.drupal-ajax" data-big-pipe-replacement-for-placeholder-with-id="', '/') . '/', $this->getSession()->getPage()->getContent());
    +    $this->assertCount($expected_count, $expected_big_pipe_placeholders_with_replacements);
    
  2. +++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAdminTest.php
    @@ -210,7 +210,7 @@ public function testFieldAdminHandler() {
    -    $this->assertIdentical(0, count($result), "No taxonomy terms exist with the name '$term_name'.");
    +    $this->assertCount(0, $result);
    

    This makes us lose the context in the message.

  3. +++ b/core/modules/field/tests/src/Kernel/FieldAttachStorageTest.php
    @@ -256,7 +256,7 @@ public function testFieldAttachDelete() {
    -      $this->assertEqual(count($revision->{$this->fieldTestData->field_name}), $cardinality, "The test entity revision $vid has $cardinality values.");
    +      $this->assertCount($cardinality, $revision->{$this->fieldTestData->field_name});
    

    This makes us lose the context in the message.

  4. +++ b/core/modules/field/tests/src/Kernel/FieldAttachStorageTest.php
    @@ -265,13 +265,13 @@ public function testFieldAttachDelete() {
    -      $this->assertEqual(count($revision->{$this->fieldTestData->field_name}), $cardinality, "The test entity revision $vid has $cardinality values.");
    +      $this->assertCount($cardinality, $revision->{$this->fieldTestData->field_name});
    

    This makes us lose the context in the message.

  5. +++ b/core/modules/field/tests/src/Kernel/FieldAttachStorageTest.php
    @@ -265,13 +265,13 @@ public function testFieldAttachDelete() {
    -    $this->assertEqual(count($current->{$this->fieldTestData->field_name}), $cardinality, "The test entity current revision has $cardinality values.");
    +    $this->assertCount($cardinality, $current->{$this->fieldTestData->field_name});
    

    This makes us lose the context in the message.

  6. +++ b/core/modules/field/tests/src/Kernel/FieldDataCountTest.php
    @@ -139,7 +138,7 @@ public function testEntityCountAndHasData() {
    -    $this->assertEqual(count($entity->{$this->fieldTestData->field_name_2}), $cardinality, new FormattableMarkup('Revision %revision_id: expected number of values.', ['%revision_id' => $first_revision]));
    +    $this->assertCount($cardinality, $entity->{$this->fieldTestData->field_name_2});
    

    This makes us lose the context in the message.

  7. +++ b/core/modules/field/tests/src/Kernel/FieldImportDeleteTest.php
    @@ -76,7 +76,7 @@ public function testImportDelete() {
    -    $this->assertEqual(count($deletes), 5, 'Importing configuration will delete 3 fields and 2 field storages.');
    +    $this->assertCount(5, $deletes);
    

    Need to have a comment why we expect 5.

+++ b/core/modules/rdf/tests/src/Functional/GetRdfNamespacesTest.php
@@ -35,16 +35,16 @@ public function testGetRdfNamespaces() {
     $element = $driver->find('//html[contains(@prefix, "rdfs: http://www.w3.org/2000/01/rdf-schema#")]');
-    $this->assertCount(1, $element, 'A prefix declared once is displayed.');
+    $this->assertCount(1, $element);
 
     $element = $driver->find('//html[contains(@prefix, "foaf: http://xmlns.com/foaf/0.1/")]');
-    $this->assertCount(1, $element, 'The same prefix declared in several implementations of hook_rdf_namespaces() is valid as long as all the namespaces are the same.');
+    $this->assertCount(1, $element);
 
     $element = $driver->find('//html[contains(@prefix, "foaf1: http://xmlns.com/foaf/0.1/")]');
-    $this->assertCount(1, $element, 'Two prefixes can be assigned the same namespace.');
+    $this->assertCount(1, $element);
 
     $element = $driver->find('//html[contains(@prefix, "dc: http://purl.org/dc/terms/")]');
-    $this->assertCount(1, $element, 'When a prefix has conflicting namespaces, the first declared one is used.');
+    $this->assertCount(1, $element);

Needs comments to replace the messages removed.

+++ b/core/modules/search/tests/src/Functional/SearchMultilingualEntityTest.php
@@ -329,7 +329,7 @@ protected function assertDatabaseCounts($count_node, $count_foo, $message) {
-    $this->assertEqual($count_foo, count($results), 'Foo count was ' . $count_foo . ' for ' . $message);
+    $this->assertCount($count_foo, $results);

This makes us lose the context in the message.

+++ b/core/modules/user/tests/src/Kernel/UserValidationTest.php
@@ -199,7 +199,7 @@ public function testValidation() {
-    $this->assertEqual(count($violations), $count, "Violation found when $field_name is too long.");
+    $this->assertCount($count, $violations);

This makes us lose the context in the message.

+++ b/core/modules/views/tests/src/Functional/Plugin/DisplayAttachmentTest.php
@@ -48,13 +48,13 @@ public function testAttachment() {
-    $this->assertEqual(count($result), 2, 'Both actual view and the attachment is rendered.');
+    $this->assertCount(2, $result);
 
     $result = $this->xpath('//div[contains(@class, "attachment-after")]');
-    $this->assertEqual(count($result), 0, 'The attachment is not rendered after the actual view.');
+    $this->assertCount(0, $result);
 
     $result = $this->xpath('//div[contains(@class, "attachment-before")]');
-    $this->assertEqual(count($result), 1, 'The attachment is rendered before the actual view.');
+    $this->assertCount(1, $result);

@@ -75,13 +75,13 @@ public function testDisabledAttachments() {
     $result = $this->xpath('//div[contains(@class, "view-content")]');
-    $this->assertEqual(count($result), 3, 'The page view and the attachments are rendered.');
+    $this->assertCount(3, $result);
 
     $result = $this->xpath('//div[contains(@class, "attachment-before")]');
-    $this->assertEqual(count($result), 1, 'The attachment is rendered before the page view.');
+    $this->assertCount(1, $result);
 
     $result = $this->xpath('//div[contains(@class, "attachment-after")]');
-    $this->assertEqual(count($result), 1, 'The attachment is rendered after the page view.');
+    $this->assertCount(1, $result);
 
     // Disable the attachment_1 display.
     $view->displayHandlers->get('attachment_1')->setOption('enabled', FALSE);

Needs comments to replace the messages removed.

+++ b/core/modules/views/tests/src/Kernel/Plugin/RelationshipTest.php
@@ -173,9 +173,15 @@ public function testRelationshipRender() {
+    $this->assertCount(1, $this->xpath($xpath, [
+      ':id' => 1,
+      ':author' => $author1->getAccountName(),
+    ]));
+    $this->assertCount(1, $this->xpath($xpath, [
+      ':id' => 2,
+      ':author' => $author2->getAccountName(),
+    ]));
+    $this->assertCount(1, $this->xpath($xpath, [':id' => 3, ':author' => '']));

This is hard to read. I suggest to split

+++ b/core/modules/views/tests/src/Kernel/Plugin/RelationshipTest.php
@@ -173,9 +173,15 @@ public function testRelationshipRender() {
+    $elements = $this->xpath($xpath, [':id' => 1, ':author' => $author1->getAccountName()]);
+    $this->assertCount(1, $elements);
+++ b/core/modules/views_ui/tests/src/Functional/HandlerTest.php
@@ -283,7 +283,7 @@ public function testErrorMissingHelp() {
-    $this->assertEqual(1, count($elements), $field_name . ' appears just once in ' . $entity_type . '.');
+    $this->assertCount(1, $elements);

This makes us lose the context in the message.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
@@ -303,13 +303,13 @@ protected function doTestReadWrite($entity_type) {
-    $this->assertEqual(count($entity->name), 3, new FormattableMarkup('%entity_type: List has 3 items.', ['%entity_type' => $entity_type]));
+    $this->assertCount(3, $entity->name);
     unset($entity->name[1]);
-    $this->assertEqual(count($entity->name), 2, new FormattableMarkup('%entity_type: Second list item has been removed.', ['%entity_type' => $entity_type]));
+    $this->assertCount(2, $entity->name);
     $this->assertEqual($entity->name[1]->value, 'Third name', new FormattableMarkup('%entity_type: The subsequent items have been shifted up.', ['%entity_type' => $entity_type]));
     $this->assertEqual($entity->name[1]->getName(), 1, new FormattableMarkup('%entity_type: The items names have been updated to their new delta.', ['%entity_type' => $entity_type]));
     $entity->name[1] = NULL;
-    $this->assertEqual(count($entity->name), 2, new FormattableMarkup('%entity_type: Assigning NULL does not reduce array count.', ['%entity_type' => $entity_type]));
+    $this->assertCount(2, $entity->name);
     $this->assertTrue($entity->name[1]->isEmpty(), new FormattableMarkup('%entity_type: Assigning NULL empties the item.', ['%entity_type' => $entity_type]));
 
     // Test using isEmpty().
@@ -318,15 +318,15 @@ protected function doTestReadWrite($entity_type) {

@@ -318,15 +318,15 @@ protected function doTestReadWrite($entity_type) {
     $entity->name->value = NULL;
     $this->assertTrue($entity->name[0]->isEmpty(), new FormattableMarkup('%entity_type: Name item is empty.', ['%entity_type' => $entity_type]));
     $this->assertTrue($entity->name->isEmpty(), new FormattableMarkup('%entity_type: Name field is empty.', ['%entity_type' => $entity_type]));
-    $this->assertEqual(count($entity->name), 1, new FormattableMarkup('%entity_type: Empty item is considered when counting.', ['%entity_type' => $entity_type]));
+    $this->assertCount(1, $entity->name);
     $this->assertEqual(count(iterator_to_array($entity->name->getIterator())), count($entity->name), new FormattableMarkup('%entity_type: Count matches iterator count.', ['%entity_type' => $entity_type]));
     $this->assertTrue($entity->name->getValue() === [0 => ['value' => NULL]], new FormattableMarkup('%entity_type: Name field value contains a NULL value.', ['%entity_type' => $entity_type]));
 
     // Test using filterEmptyItems().
     $entity->name = [NULL, 'foo'];
-    $this->assertEqual(count($entity->name), 2, new FormattableMarkup('%entity_type: List has 2 items.', ['%entity_type' => $entity_type]));
+    $this->assertCount(2, $entity->name);
     $entity->name->filterEmptyItems();
-    $this->assertEqual(count($entity->name), 1, new FormattableMarkup('%entity_type: The empty item was removed.', ['%entity_type' => $entity_type]));
+    $this->assertCount(1, $entity->name);
     $this->assertEqual($entity->name[0]->value, 'foo', new FormattableMarkup('%entity_type: The items were renumbered.', ['%entity_type' => $entity_type]));

This makes us lose the context in the message.

  1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php
    @@ -255,24 +255,24 @@ protected function doTestMultilingualProperties($entity_type) {
         $entities = $storage->loadMultiple();
    -    $this->assertEqual(count($entities), 3, new FormattableMarkup('%entity_type: Three entities were created.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(3, $entities);
         $entities = $storage->loadMultiple([$translated_id]);
    -    $this->assertEqual(count($entities), 1, new FormattableMarkup('%entity_type: One entity correctly loaded by id.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(1, $entities);
         $entities = $storage->loadByProperties(['name' => $name]);
    -    $this->assertEqual(count($entities), 2, new FormattableMarkup('%entity_type: Two entities correctly loaded by name.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(2, $entities);
         // @todo The default language condition should go away in favor of an
         // explicit parameter.
         $entities = $storage->loadByProperties(['name' => $properties[$langcode]['name'][0], $default_langcode_key => 0]);
    -    $this->assertEqual(count($entities), 1, new FormattableMarkup('%entity_type: One entity correctly loaded by name translation.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(1, $entities);
         $entities = $storage->loadByProperties([$langcode_key => $default_langcode, 'name' => $name]);
    -    $this->assertEqual(count($entities), 1, new FormattableMarkup('%entity_type: One entity correctly loaded by name and language.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(1, $entities);
     
         $entities = $storage->loadByProperties([$langcode_key => $langcode, 'name' => $properties[$langcode]['name'][0]]);
    -    $this->assertEqual(count($entities), 0, new FormattableMarkup('%entity_type: No entity loaded by name translation specifying the translation language.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(0, $entities);
         $entities = $storage->loadByProperties([$langcode_key => $langcode, 'name' => $properties[$langcode]['name'][0], $default_langcode_key => 0]);
    -    $this->assertEqual(count($entities), 1, new FormattableMarkup('%entity_type: One entity loaded by name translation and language specifying to look for translations.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(1, $entities);
         $entities = $storage->loadByProperties(['user_id' => $properties[$langcode]['user_id'][0], $default_langcode_key => NULL]);
    -    $this->assertEqual(count($entities), 2, new FormattableMarkup('%entity_type: Two entities loaded by uid without caring about property translatability.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(2, $entities);
     
    

    This makes us lose the context in the message.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTranslationTest.php
    @@ -284,7 +284,7 @@ protected function doTestMultilingualProperties($entity_type) {
    -    $this->assertEqual(count($result), 1, new FormattableMarkup('%entity_type: One entity loaded by name and uid using different language meta conditions.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(1, $result);
     
         // Test mixed property and field conditions.
         $storage->resetCache($result);
    @@ -304,7 +304,7 @@ protected function doTestMultilingualProperties($entity_type) {
    
    @@ -304,7 +304,7 @@ protected function doTestMultilingualProperties($entity_type) {
           ->condition($default_langcode_group)
           ->condition($langcode_group)
           ->execute();
    -    $this->assertEqual(count($result), 1, new FormattableMarkup('%entity_type: One entity loaded by name, uid and field value using different language meta conditions.', ['%entity_type' => $entity_type]));
    +    $this->assertCount(1, $result);
    

    This makes us lose the context in the message.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Menu/MenuTreeStorageTest.php
    @@ -415,7 +415,7 @@ protected function assertMenuLink($id, array $expected_properties, array $parent
    -    $this->assertEqual(count($all), 1, "Found link $id matching all the expected properties");
    +    $this->assertCount(1, $all);
    

    This makes us lose the context in the message.

  4. +++ b/core/tests/Drupal/Tests/Component/Graph/GraphTest.php
    @@ -149,7 +149,7 @@ protected function assertComponents($graph, $expected_components) {
           }
    -      $this->assertEquals(1, count(array_unique($result_components)), sprintf('Expected one unique component for vertices %s, got %s', $this->displayArray($component), $this->displayArray($result_components)));
    +      $this->assertCount(1, array_unique($result_components));
         }
    

    This makes us lose the context in the message.

jungle’s picture

@mondrake, thanks for your review.

But to me, this is too frustrating, especially, back and forth many times.

#42 is too long to follow one by one. I have a small 13-inch screen and no dual monitors. I need switching between the browser and the editor frequently to get all points addressed. Similar to #3133162: Replace the start verb Test with Tests in method comments of tests I've worked on yesterday, frankly, it drove me crazy. 41 revision points listed, what a long list!

I would suggest sticking with the scope as the title indicated "Replace assert* involving count() and an integer literal with assertCount()", STRICKLY. leaving the task of removing redundant assertion messages to a separate issue.

If we can agree on that. What to do next is to revert all assertion messages removed.

jungle’s picture

> If we can agree on that. What to do next is to revert all assertion messages removed.

Actually, If we can agree on that, we can continue with #19 which is a reroll of #18 without messages removed.

jungle’s picture

About redundant assertion messages, so far, there is no feasible definition of what assertion message is redundant or not. Would be great to conduct an agreement before proceeding.

"Removed all assertion messages, except when they're used in a loop" is the one we follow lately, but this rule is fragile. It's not applicable to all cases.

mondrake’s picture

Hi @jungle yes, I agree with #43 and #44. Sorry with that. Let's focus on the important thing (converting to assertCount), and leave the messages only as a very last step if core committers require so, or for a follow up.

jungle’s picture

FileSize
20.72 KB
401.46 KB

@mondrake, thanks for your agreement!

> Fixed PHPLint error but setting back to NW for removing messages.

I started this in #17, actually. 🤦

First, rerolled a patch from #19

jungle’s picture

Status: Needs work » Needs review
FileSize
399.09 KB
2.67 KB

Reverting out of scope changes by repeating #30 and #35.2

jungle’s picture

Due to rescoping, apologize to all for your efforts wasted.

jungle’s picture

Assigned: Unassigned » jungle

As there are two meta-issues already, #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages and this one's parent. One is about assertion message removal, one is to replace assertion with more appropriate ones, So I won't put a needs follow-up tag, and I do not think it's necessary.

One more thing that, assertCount() is available in PHPUnit 6 and 7, that says it is applicable to all branches down to 8.8.x. So I will work on patch(es) for other branches next.

jungle’s picture

jungle’s picture

Assigned: jungle » Unassigned

Status: Needs review » Needs work

The last submitted patch, 51: 3126965-51.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jungle’s picture

  1. Reverted changes to simpletest-relevant two files trying to fix the failed tests in #51
  2. Rerolled a patch for 8.9.x based on the new patch for 8.8.x
  3. 9.x patch is in #48
mondrake’s picture

@jungle practical suggestion: unless specifically required by a committer, I would suggest to avoid working on backport patches at the moment. You will just spend a lot of time trying to keep 4 versions aligned, better do it when it makes sense. BTW I am not seeing any of these chain of issues backported D8.8, really.

I'll try to review the D9.1 one - it's a 400kb+ patch, it will take some time - but if that's RTBCed and committed then you will have a clear reference for the backport, not sooner.

Any reviewers, patch to review is at #48.

jungle’s picture

Issue summary: View changes

Thanks for your suggestion! #3131474: Replace assertions involving calls to array_search() with assertContains()/assertNotContains() is the only one in this series of issues backported to 8.8.x so far

Updating the recommended regex to IS:

assert.+\((\d+, count\(.+\)|count\(.+\), \d+)(|.+)\)

mondrake’s picture

Status: Needs review » Needs work

Reviewing, just realized #3133291: Add XPath existence assertions is a do-do for follow up... apart from the many assertCount(n, $this->xpath(...));, there sre probably even more that assign the xpath result to a variable and assert on the variable itself.

A few points:

  1. +++ b/core/modules/file/tests/src/Kernel/ValidatorTest.php
    @@ -45,11 +45,11 @@ protected function setUp(): void {
       public function testFileValidateExtensions() {
         $file = File::create(['filename' => 'asdf.txt']);
         $errors = file_validate_extensions($file, 'asdf txt pork');
    -    $this->assertEqual(count($errors), 0, 'Valid extension accepted.', 'File');
    +    $this->assertCount(0, $errors, 'Valid extension accepted.', NULL);
     
         $file->setFilename('asdf.txt');
         $errors = file_validate_extensions($file, 'exe png');
    -    $this->assertEqual(count($errors), 1, 'Invalid extension blocked.', 'File');
    +    $this->assertCount(1, $errors, 'Invalid extension blocked.', NULL);
       }
     
       /**
    @@ -58,11 +58,11 @@ public function testFileValidateExtensions() {
    
    @@ -58,11 +58,11 @@ public function testFileValidateExtensions() {
       public function testFileValidateIsImage() {
         $this->assertFileExists($this->image->getFileUri());
         $errors = file_validate_is_image($this->image);
    -    $this->assertEqual(count($errors), 0, 'No error reported for our image file.', 'File');
    +    $this->assertCount(0, $errors, 'No error reported for our image file.', NULL);
     
         $this->assertFileExists($this->nonImage->getFileUri());
         $errors = file_validate_is_image($this->nonImage);
    -    $this->assertEqual(count($errors), 1, 'An error reported for our non-image file.', 'File');
    +    $this->assertCount(1, $errors, 'An error reported for our non-image file.', NULL);
       }
     
       /**
    @@ -73,19 +73,19 @@ public function testFileValidateIsImage() {
    
    @@ -73,19 +73,19 @@ public function testFileValidateIsImage() {
       public function testFileValidateImageResolution() {
         // Non-images.
         $errors = file_validate_image_resolution($this->nonImage);
    -    $this->assertEqual(count($errors), 0, 'Should not get any errors for a non-image file.', 'File');
    +    $this->assertCount(0, $errors, 'Should not get any errors for a non-image file.', NULL);
         $errors = file_validate_image_resolution($this->nonImage, '50x50', '100x100');
    -    $this->assertEqual(count($errors), 0, 'Do not check the resolution on non files.', 'File');
    +    $this->assertCount(0, $errors, 'Do not check the resolution on non files.', NULL);
     
         // Minimum size.
         $errors = file_validate_image_resolution($this->image);
    -    $this->assertEqual(count($errors), 0, 'No errors for an image when there is no minimum or maximum resolution.', 'File');
    +    $this->assertCount(0, $errors, 'No errors for an image when there is no minimum or maximum resolution.', NULL);
         $errors = file_validate_image_resolution($this->image, 0, '200x1');
    -    $this->assertEqual(count($errors), 1, 'Got an error for an image that was not wide enough.', 'File');
    +    $this->assertCount(1, $errors, 'Got an error for an image that was not wide enough.', NULL);
         $errors = file_validate_image_resolution($this->image, 0, '1x200');
    -    $this->assertEqual(count($errors), 1, 'Got an error for an image that was not tall enough.', 'File');
    +    $this->assertCount(1, $errors, 'Got an error for an image that was not tall enough.', NULL);
         $errors = file_validate_image_resolution($this->image, 0, '200x200');
    -    $this->assertEqual(count($errors), 1, 'Small images report an error.', 'File');
    +    $this->assertCount(1, $errors, 'Small images report an error.', NULL);
     
         // Maximum size.
    

    (and others) just drop the 4th argument instead of changing to NULL, it has no purpose in assertCount

  2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
    @@ -189,10 +189,10 @@ public function testRead() {
    +    $this->assertCount(1, array_filter(array_keys($single_output['meta']['omitted']['links']), function ($key) {
           return $key !== 'help';
    -    })));
    +    }));
    

    for readibility, I would split this in two statements, one assigning the result of array_filter to a variable, the second to assertCount on that variable

  3. +++ b/core/modules/views/tests/src/Kernel/Plugin/RelationshipTest.php
    @@ -173,9 +173,15 @@ public function testRelationshipRender() {
    -    $this->assertEqual(1, count($this->xpath($xpath, [':id' => 1, ':author' => $author1->getAccountName()])));
    -    $this->assertEqual(1, count($this->xpath($xpath, [':id' => 2, ':author' => $author2->getAccountName()])));
    -    $this->assertEqual(1, count($this->xpath($xpath, [':id' => 3, ':author' => ''])));
    +    $this->assertCount(1, $this->xpath($xpath, [
    +      ':id' => 1,
    +      ':author' => $author1->getAccountName(),
    +    ]));
    +    $this->assertCount(1, $this->xpath($xpath, [
    +      ':id' => 2,
    +      ':author' => $author2->getAccountName(),
    +    ]));
    +    $this->assertCount(1, $this->xpath($xpath, [':id' => 3, ':author' => '']));
    

    I'd keep these as one-liners if possible

jungle’s picture

Assigned: Unassigned » jungle

Thanks for your review again! On it.

darrenwh’s picture

Some observations that could be updated.

  1. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -1399,7 +1399,7 @@ public function testDateStorageSettings() {
    +    $this->assertCount(1, $result, "Changing datetime setting is disabled.");
    

    can use single quotes

  2. +++ b/core/modules/field/tests/src/Functional/Boolean/BooleanFormatterSettingsTest.php
    @@ -127,7 +127,7 @@ public function testBooleanFormatterSettings() {
    +      $this->assertCount(1, $result, "Boolean formatter settings summary exist.");
    

    exists

  3. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
    @@ -293,7 +293,7 @@ public function testPurgeWithDeletedAndActiveField() {
    +    $this->assertCount(0, $fields, 'The field is gone');
    

    The field has gone

  4. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
    @@ -352,14 +352,14 @@ public function testPurgeField() {
    +    $this->assertCount(0, $fields, 'The field is gone');
    

    The field has gone

  5. +++ b/core/modules/image/tests/src/FunctionalJavascript/ImageFieldValidateTest.php
    @@ -36,7 +36,7 @@ public function testAJAXValidationMessage() {
    +    $this->assertCount(1, $elements, 'Ajax validation messages are displayed once.');
    

    AJAX

  6. +++ b/core/modules/taxonomy/tests/src/Kernel/TermKernelTest.php
    @@ -108,11 +108,11 @@ public function testTaxonomyVocabularyTree() {
    +    $this->assertCount(1, $tree, 'We have one parent with depth 1.');
    

    There is one parent...

  7. +++ b/core/modules/taxonomy/tests/src/Kernel/TermKernelTest.php
    @@ -108,11 +108,11 @@ public function testTaxonomyVocabularyTree() {
    +    $this->assertCount(8, $tree, 'We have all vocabulary tree elements.');
    

    There is one parent...

  8. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarMenuTranslationTest.php
    @@ -71,7 +71,7 @@ public function testToolbarClasses() {
    +    $this->assertCount(1, $xpath, 'The menu item class ok before translation.');
    

    OK

  9. +++ b/core/modules/user/tests/src/Functional/UserAccountLinksTest.php
    @@ -70,7 +70,7 @@ public function testSecondaryMenu() {
    +    $this->assertCount(1, $link, 'Log in link is in secondary menu.');
    

    is in the secondary

  10. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormCheckboxesTest.php
    @@ -165,7 +165,7 @@ public function testExposedIsAllOfFilter() {
    +    $this->assertCount(2, $rows, 'Correct rows are displayed when a tid is selected.');
    

    TID perhaps?

  11. +++ b/core/modules/views_ui/tests/src/Functional/SettingsTest.php
    @@ -110,7 +110,7 @@ public function testEditUI() {
    +    $this->assertCount(0, $xpath, 'The views sql is hidden.');
    

    SQL

  12. +++ b/core/modules/views_ui/tests/src/Functional/SettingsTest.php
    @@ -122,7 +122,7 @@ public function testEditUI() {
    +    $this->assertCount(1, $xpath, 'The views sql is shown.');
    

    SQL

  13. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectTest.php
    @@ -248,7 +248,7 @@ public function testIsNotNullCondition() {
    +    $this->assertCount(2, $names, 'Correct number of records found withNOT NULL age.');
    

    with NOT

  14. +++ b/core/tests/Drupal/KernelTests/Core/File/ScanDirectoryTest.php
    @@ -52,7 +52,7 @@ public function testReturn() {
    +    $this->assertCount(2, $all_files, 'Found two, expected javascript files.');
    

    JavaScript

  15. +++ b/core/tests/Drupal/KernelTests/Core/File/ScanDirectoryTest.php
    @@ -78,18 +78,18 @@ public function testOptionCallback() {
    +    $this->assertCount(2, $all_files, 'Found two, expected javascript files.');
    

    JavaScript

  16. +++ b/core/tests/Drupal/KernelTests/Core/File/ScanDirectoryTest.php
    @@ -100,11 +100,11 @@ public function testOptionCallback() {
    +    $this->assertCount(2, $all_files, 'Found two, expected javascript files.');
    

    JavaScript

  17. +++ b/core/tests/Drupal/KernelTests/Core/File/ScanDirectoryTest.php
    @@ -148,7 +148,7 @@ public function testOptionRecurse() {
    +    $this->assertCount(2, $files, 'With recursion we found the expected javascript files.');
    

    JavaScript

mondrake’s picture

Actually, regex in #56 seems too tight.

I tried both assert.*\(count\( and assert.*\(.*, count\( and I can see several more that slipped between the cracks... amidst many false positives though, I admit.

EDIT - disregard, this comment was not in line with the scope declared.

mondrake’s picture

#59 - please note we suggest not to touch any message in this issue. What we had before, we'll have after this patch. See #43 and following. The patch is already 400kb+ and if we start questioning messages we will never do it.

Let's address messages separately in one or more follow ups. Actually many of them could be just removed, probably. Please refer to #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for more discussion.

jungle’s picture

Status: Needs work » Needs review
FileSize
398.96 KB
8.99 KB

Addressed #57 only.

@darrenwh, much appreciated for your review. but I'd say sorry. as #61 commented, we now choose to stick with the current scope strictly.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

@jungle thanks. Now it LGTM. The size of the patch is significant, so we propose not to address changes/removals to custom assertion messages here, but to defer that to other issues, to be spin offs from #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages. Grepping the codebase with the regex in IS yields no results. There are potentially more conversions to be done, but those will not involve literal integers, so out of scope here.

  • catch committed 40a5bd7 on 9.1.x
    Issue #3126965 by jungle, quietone, longwave, mondrake, dww: Replace...

  • catch committed 2b3ae7c on 9.0.x
    Issue #3126965 by jungle, quietone, longwave, mondrake, dww: Replace...
catch’s picture

Title: Replace assert* involving count() and an integer literal with assertCount() » [backport] Replace assert* involving count() and an integer literal with assertCount()
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Needs work

Committed 40a5bd7 and pushed to 9.1.x, cherry-picked to 9.0.x. Thanks!

Will need a re-roll for 8.9.x

jungle’s picture

Status: Needs work » Needs review
FileSize
413.88 KB
43.37 KB

Status: Needs review » Needs work

The last submitted patch, 67: 3126965-66.patch, failed testing. View results

jungle’s picture

Rerolled the patch first, fixing tests next.

jungle’s picture

Assigned: jungle » Unassigned
Status: Needs work » Needs review
FileSize
411.37 KB
2.49 KB

Reverting the changes to simpletest related assertions as assertCount() is not available in Drupal\simpletest\WebTestBase

Status: Needs review » Needs work

The last submitted patch, 70: 3126965-70.patch, failed testing. View results

jungle’s picture

Status: Needs work » Needs review
FileSize
410.11 KB
1.26 KB

Two more to revert in core/modules/simpletest/src/Tests/BrowserTest.php

Status: Needs review » Needs work

The last submitted patch, 72: 3126965-72.patch, failed testing. View results

jungle’s picture

Status: Needs work » Needs review

A random failure. Queued again

sja112’s picture

Assigned: Unassigned » sja112

On this.

sja112’s picture

Assigned: sja112 » Unassigned
Status: Needs review » Reviewed & tested by the community

I have reviewed the patch #72 for the 8.9.x version.
Interdiffs submitted in #67, #70, #72 looks good.
The scope of this issue is covered in the patch.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll, +Avoid commit conflicts, +beta target

No longer applies (already). This is another one you can ping me once it's RTBC again. Thanks!

jungle’s picture

Status: Needs work » Needs review
FileSize
410.15 KB
2.08 KB

Thank you, @xjm, I will ping you once the testing passes.

xjm’s picture

Great, thanks! First thing tomorrow I guess then for me, but I'll ping other committers and let them know I'm going to revert anything else that conflicts with this between now and then.

FWIW given the size of this patch I might have scoped it into two issues, one where the count() was around the first parameter in the original code, and another where it was around the second. Too late now since it's in D9 already, but just as an example for future reference. :) (It's still maybe in --color-words territory rather than --porcelain, although I might also have written a --porcelain script if I'd been reviewing it on all four branches, or I still might if my eyes start to glaze over too much on it even after sleeping.)

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

I've scanned every change using git diff --color-words and all the changes are the expected change to assertCount() and nothing stood out as wonky.

  • catch committed 8090ecf on 8.9.x
    Issue #3126965 by jungle, quietone, longwave, mondrake, dww, sja112, xjm...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8090ecf and pushed to 8.9.x. Thanks!

Status: Fixed » Closed (fixed)

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