Replace assert*() involving greater/less comparison operators with assert(Greater|Less)Than(OrEqual)

Per Slack thread with @jungle, I'm helping to give these issues human readable titles.

However, in this case, I'm not sure the proposed change is an improvement. ;) I think I'm -1 to this one. I don't see how this improves readability at all, and it takes a lot more characters to make it work...

Also, it's not clear why we care about count() here at all. Seems like the intention would be to replace:

assertTrue(foo >= bar) with assertGreaterThanOrEqual(foo, bar), regardless of what foo and bar are...

I am reopening this since I think we should also consider the output in case of failure.

Taking example from #3, in case of failing assertion

::assertTrue($foo >= $bar);

will produce an error like the following, not very helpful IMHO

PHPUnit 9.0.0 by Sebastian Bergmann and contributors.

F

Time: 0 seconds, Memory: 5.00Mb

There was 1 failure:

1) TrueTest::testFailure
Failed asserting that false is true.

OTOH, in case of failure of

::assertGreaterThanOrEqual($foo, $bar);

the message will be far more readable, plus the types of $bar and $foo will be automatically checked by PHPUnit to ensure they are comparable

PHPUnit 9.0.0 by Sebastian Bergmann and contributors.

F

Time: 0 seconds, Memory: 5.25Mb

There was 1 failure:

1) GreatThanOrEqualTest::testFailure
Failed asserting that 1 is equal to 2 or is greater than 2.

I would suggest a much broader scope,

$ grep -Er '>assert(True|False).*( > | < | <= | >= )' .

EDIT - let's agree it makes sense before attacking the code

Working on this, with regex >assert(Not)?(Tr|Fa|Eq|Id).*( < | > | >= | <= )

Not really sure so testing a patch so far.

Unsure if you wanted reviews yet but I have a few comments here:

1. +++ b/core/modules/comment/tests/src/Functional/CommentBlockTest.php
   @@ -77,7 +77,7 @@ public function testRecentCommentBlock() {
   -        $this->assertTrue($position > $previous_position, new FormattableMarkup('Comment @a appears after comment @b', ['@a' => 10 - $i, '@b' => 11 - $i]));
   +        $this->assertGreaterThan($previous_position, $position);
   

   The message seems useful here.

2. +++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php
   @@ -71,7 +71,8 @@ public function testInstallProfileConfigOverwrite() {
   -    $this->assertTrue(Uuid::isValid($site_uuid) && strlen($site_uuid) > 0, "Site UUID '$site_uuid' is valid");
   ...
   +    $this->assertGreaterThan(0, strlen($site_uuid));
   

   I think this is equivalent to and more readable as assertNotEmpty() - although does Uuid::isValid('') return TRUE?

3. +++ b/core/modules/field/tests/src/Kernel/FieldValidationTest.php
   @@ -67,7 +67,7 @@ public function testFieldConstraints() {
        // The test is only valid if the field cardinality is greater than 2.
   -    $this->assertTrue($cardinality >= 2);
   +    $this->assertGreaterThanOrEqual(2, $cardinality);
   

   Comment says > but test says >= !

4. +++ b/core/modules/file/tests/src/Functional/FileManagedTestBase.php
   @@ -156,7 +156,7 @@ public function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
   -    $this->assertTrue($file->id() > 0, 'The file was added to the database.', 'Create test file');
   +    $this->assertGreaterThan(0, $file->id());
   

   This could be assertNotEmpty()?

Thanks @longwave. I will take this in next patch.

Only addressing failures in #13 to get to a clear reference for conversion further along, and #14.

#14:
1. OK done
2. Yes, that seems redundant but not sure we can remove it here
3. OK, corrected the comments that seemed wrong to me
4. Not IMHO - id() should return a number that is a sequence from a serial column db table, so I suggest to keep

If this passes, it's up for review.

Thank you, @jungle

I checked each of the changes and they all look fine to me.

+++ b/core/modules/file/tests/src/Kernel/SaveTest.php
@@ -45,8 +45,8 @@ public function testFileSave() {
-    $this->assertTrue($file->getChangedTime() >= $file->getChangedTime(), "Timestamp didn't go backwards.", 'File');
     $loaded_file = File::load($file->id());
+    $this->assertGreaterThanOrEqual($file->getChangedTime(), $loaded_file->getChangedTime());

Good spot, this never could have failed before :)

Reviewing.

Thanks! I like where this is heading. My main concern is that there are a few places where the assertion message is adding information about how the test works that's lost when we remove it. A lot of these will end up with failure messages that are like "Failed asserting that 3 is greater than 5"... which is useful debugging as far as it goes, but the test at that line should explain why we care about the relative values of 3 and 5.

We can retain the information where it's useful by adding an inline comment that explains the purpose of the assertion, while still getting the raw debug values from the assertion.

1. +++ b/core/modules/block/tests/src/Functional/BlockRenderOrderTest.php
   @@ -78,8 +78,8 @@ public function testBlockRenderOrder() {
   -    $this->assertTrue($position['stark_powered'] < $position['stark_by'], 'Blocks with different weight are rendered in the correct order.');
   -    $this->assertTrue($position['stark_drupal'] < $position['stark_by'], 'Blocks with identical weight are rendered in alphabetical order.');
   +    $this->assertLessThan($position['stark_by'], $position['stark_powered']);
   +    $this->assertLessThan($position['stark_by'], $position['stark_drupal']);
   

   Here we're losing information provided by the assertion messages. Can we move them to inline comments above the respective assertions?
   // Verify that... (etc.).

2. +++ b/core/modules/block_content/tests/src/Kernel/Migrate/d6/MigrateBlockContentTest.php
   --- a/core/modules/comment/tests/src/Functional/CommentBlockTest.php
   +++ b/core/modules/comment/tests/src/Functional/CommentBlockTest.php
   
   +++ b/core/modules/comment/tests/src/Functional/CommentBlockTest.php
   @@ -77,7 +77,7 @@ public function testRecentCommentBlock() {
   -        $this->assertTrue($position > $previous_position, new FormattableMarkup('Comment @a appears after comment @b', ['@a' => 10 - $i, '@b' => 11 - $i]));
   +        $this->assertGreaterThan($previous_position, $position, sprintf('Comment %d does not appear after comment %d', 10 - $i, 11 - $i));
   

   This is a weird assertion to begin with.

   I confirmed there's still another FormattableMarkup in there, which is why the use statement stays.

   (Nothing actionable, just making note because I gave it consideration.)

3. +++ b/core/modules/editor/tests/src/Functional/EditorAdminTest.php
   @@ -62,7 +62,8 @@ public function testNoEditorAvailable() {
   -    $this->assertTrue($roles_pos < $editor_pos && $editor_pos < $filters_pos, '"Text Editor" select appears in the correct location of the text format configuration UI.');
   +    $this->assertLessThan($editor_pos, $roles_pos);
   +    $this->assertLessThan($filters_pos, $editor_pos);
   
   +++ b/core/modules/field_ui/tests/src/Functional/EntityDisplayModeTest.php
   @@ -204,7 +204,7 @@ public function testAlphabeticalDisplaySettings() {
   -      $this->assertTrue($new_pos > $pos, 'Order of ' . $name . ' is correct on page');
   +      $this->assertGreaterThan($pos, $new_pos);
   
   +++ b/core/modules/file/tests/src/Functional/FileManagedFileElementTest.php
   @@ -59,7 +59,7 @@ public function testManagedFile() {
   -          $this->assertTrue($last_fid > $last_fid_prior, 'New file got saved.');
   +          $this->assertGreaterThan($last_fid_prior, $last_fid);
   
   @@ -72,7 +72,7 @@ public function testManagedFile() {
   -          $this->assertTrue($last_fid > $last_fid_prior, 'New file got uploaded.');
   +          $this->assertGreaterThan($last_fid_prior, $last_fid);
   
   +++ b/core/modules/file/tests/src/Functional/FileManagedTestBase.php
   @@ -156,7 +156,7 @@ public function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
   -    $this->assertTrue($file->id() > 0, 'The file was added to the database.', 'Create test file');
   +    $this->assertGreaterThan(0, $file->id());
   
   +++ b/core/modules/file/tests/src/Functional/PrivateFileOnTranslatedEntityTest.php
   @@ -117,7 +117,7 @@ public function testPrivateLanguageFile() {
   -    $this->assertTrue($last_fid > $last_fid_prior, 'New file got saved.');
   +    $this->assertGreaterThan($last_fid_prior, $last_fid);
   
   +++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
   @@ -100,7 +100,7 @@ protected function setUp(): void {
   -    $this->assertTrue($max_fid_after > $this->maxFidBefore, 'A new file was created.');
   +    $this->assertGreaterThan($this->maxFidBefore, $max_fid_after);
   
   +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
   @@ -95,7 +95,7 @@ protected function setUp(): void {
   -    $this->assertTrue($max_fid_after > $this->maxFidBefore, 'A new file was created.');
   +    $this->assertGreaterThan($this->maxFidBefore, $max_fid_after);
   
   +++ b/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php
   @@ -169,7 +169,7 @@ public function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
   -    $this->assertTrue($file->id() > 0, 'The file was added to the database.', 'Create test file');
   +    $this->assertGreaterThan(0, $file->id());
   
   +++ b/core/modules/file/tests/src/Kernel/SaveTest.php
   @@ -28,12 +28,12 @@ public function testFileSave() {
   -    $this->assertTrue($file->id() > 0, 'A new file ID is set when saving a new file to the database.', 'File');
   +    $this->assertGreaterThan(0, $file->id());
   
   +++ b/core/modules/locale/tests/src/Functional/LocaleJavascriptTranslationTest.php
   @@ -163,7 +163,7 @@ public function testLocaleTranslationJsDependencies() {
   -    $this->assertTrue(strpos($content, $js_filename) < strpos($content, 'core/misc/drupal.js'), 'Translations are included before Drupal.t.');
   
   +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateTest.php
   @@ -142,10 +142,10 @@ public function testUpdateImportSourceRemote() {
   -    $this->assertTrue($history['contrib_module_one']['de']->timestamp >= $this->timestampNow, 'Translation of contrib_module_one is imported');
   -    $this->assertTrue($history['contrib_module_one']['de']->last_checked >= $this->timestampNow, 'Translation of contrib_module_one is updated');
   ...
   -    $this->assertTrue($history['contrib_module_two']['de']->last_checked >= $this->timestampNow, 'Translation of contrib_module_two is updated');
   
   @@ -196,10 +196,10 @@ public function testUpdateImportSourceLocal() {
   -    $this->assertTrue($history['contrib_module_one']['de']->timestamp >= $this->timestampMedium, 'Translation of contrib_module_one is imported');
   ...
   -    $this->assertTrue($history['contrib_module_two']['de']->last_checked >= $this->timestampNow, 'Translation of contrib_module_two is updated');
   
   +++ b/core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php
   @@ -361,8 +361,7 @@ public function testVeryLongLineWrap() {
   -    $verbose = 'Maximum line length found was ' . $maximum_line_length . ' octets.';
   
   +++ b/core/modules/system/tests/src/Functional/Module/ModuleTestBase.php
   @@ -195,8 +195,8 @@ public function assertLogMessage($type, $message, $variables = [], $severity = R
   -    $this->assertTrue($count > 0, new FormattableMarkup('watchdog table contains @count rows for @message', ['@count' => $count, '@message' => new FormattableMarkup($message, $variables)]));
   
   +++ b/core/modules/system/tests/src/Functional/Theme/ThemeSuggestionsAlterTest.php
   @@ -113,7 +113,7 @@ public function testSpecificSuggestionsAlter() {
   -    $this->assertTrue(strpos($raw_content, 'theme_test_specific_suggestions__variant') < strpos($raw_content, 'theme_test_specific_suggestions__variant__foo'), 'Specific theme call is added to the suggestions array before the suggestions alter hook.');
   +    $this->assertLessThan(strpos($raw_content, 'theme_test_specific_suggestions__variant__foo'), strpos($raw_content, 'theme_test_specific_suggestions__variant'));
   
   +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
   @@ -99,7 +99,7 @@ public function testTranslationUI() {
   -      $this->assertTrue($row->count < 2, 'Term does not have translations.');
   +      $this->assertLessThan(2, $row->count);
   
   +++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
   @@ -375,9 +375,9 @@ public function testRenderDifferentWeight() {
   -    $this->assertTrue(strpos($rendered_js, 'lighter.css') < strpos($rendered_js, 'first.js'), 'Lighter CSS assets are rendered first.');
   -    $this->assertTrue(strpos($rendered_js, 'lighter.js') < strpos($rendered_js, 'first.js'), 'Lighter JavaScript assets are rendered first.');
   -    $this->assertTrue(strpos($rendered_js, 'before-jquery.js') < strpos($rendered_js, 'core/assets/vendor/jquery/jquery.min.js'), 'Rendering a JavaScript file above jQuery.');
   

   Other places where the assertion message contains potentially useful info we could move to an inline comment.

4. +++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
   @@ -397,7 +397,7 @@ public function testAlter() {
   -    $this->assertTrue(strpos($rendered_js, 'alter.js') < strpos($rendered_js, 'core/misc/tableselect.js'), 'Altering JavaScript weight through the alter hook.');
   

   The previous message isn't great, but it at least hints at the point of the order being altered by... whatever.

(Saving issue credit.)

I'd suggest we split this into two scopes: First, take care of the assertions that already don't have assertion messsages; then, handle the cases where we're removing the assertion messages (since those may provide documentation value and need to be reviewed differently).

Working on this.

Patch re-rolled and addressed the second part of the scope of this ticket as suggested in #28.

Also includes #25.1, #25.3, #25.4

Hi,

I've reviewed the patches and got some suggestions:

Comment's first letter should in capital and a fullstop is expected at the end.

+      // Verify that translation creation timestamp of the target translation language
+      // is newer than the creation timestamp of the source translation default
+      // language for translatable created field.

+    // length to the character length.

Sorry, I just missed the diff and reviwed wrongly.

The patch is fine, moving it to RTBC

Nice work! That helps keep the debugging information in the tests, without obscuring the actual results of the assertions.

We need some small cleanup on many of the comments, mostly to do with comments wrapping too early or late, or with some small grammar fixes now that the phrases of the assertion messages are whole sentences as inline comments.

1. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -399,7 +399,8 @@ protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholde
   +    // Verify that BigPipe start signal appears before stop signal.
   

   We need to add the word "the" now that this is a sentence:

   "Verify that the BigPipe start signal appears before the stop signal."

2. +++ b/core/modules/block/tests/src/Functional/BlockRenderOrderTest.php
   @@ -78,8 +78,10 @@ public function testBlockRenderOrder() {
   +    // Verify that blocks with different weight are rendered in the correct order.
   ...
   +    // Verify that blocks with identical weight are rendered in alphabetical order.
   
   +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -148,9 +148,10 @@ protected function doTestBasicTranslation() {
   +      // Verify that translation creation timestamp of the target translation language
   
   +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
   @@ -318,6 +318,7 @@ public function testThemeMetaData() {
   +    // Verify that the bartik.info.yml file modification time field contains a timestamp.
   
   +++ b/core/modules/views/tests/src/Kernel/Handler/AreaOrderTest.php
   @@ -70,7 +70,8 @@ public function testAreaOrder() {
   +    // Verify that block bartik-powered is positioned before block bartik-branding.
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityFieldMethodInvocationOrderTest.php
   @@ -61,7 +61,8 @@ public function testFieldMethodInvocationOrder() {
   +    // Verify that the field presave method has been invoked in the correct entity translation order.
   
   @@ -69,7 +70,8 @@ public function testFieldMethodInvocationOrder() {
   +    // Verify that the field presave method has been invoked in the correct entity translation order.
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTranslationTest.php
   @@ -51,8 +51,10 @@ public function testNewRevisionAfterTranslation() {
   +    // Verify that the saved translation in new revision has a newer revision id.
   

   These comments are over 80 characters and need to be wrapped.

3. +++ b/core/modules/block_content/tests/src/Functional/BlockContentRevisionsTest.php
   @@ -103,7 +103,8 @@ public function testRevisions() {
   +    // Verify that revision id is greater than default revision id.
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTranslationTest.php
   @@ -51,8 +51,10 @@ public function testNewRevisionAfterTranslation() {
   +    // Verify that the entity from the storage has a newer revision id.
   

   We've had issues to clean this up before, but "ID" should be capitalized when it's used in a sentence. Lowercase "id" is a concept in psychology.

   The assertion messages already got this wrong, but since we're upgrading some of these from sentence fragments to actual comment sentences, we can capitalize it in all cases in the new comments.

   Also, we should say "Verify that the revision ID is greater than the article ID." (Since it's a sentence now.)

4. +++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php
   @@ -71,7 +71,9 @@ public function testInstallProfileConfigOverwrite() {
   +    // Verify that site UUID '$site_uuid' is valid.
   

   "the site UUID"

5. +++ b/core/modules/editor/tests/src/Functional/EditorAdminTest.php
   @@ -62,7 +62,10 @@ public function testNoEditorAvailable() {
   +    // Verify that "Text Editor" select appears in the correct location
   +    // of the text format configuration UI.
   
   +++ b/core/modules/system/tests/src/Functional/Theme/ThemeSuggestionsAlterTest.php
   @@ -113,7 +113,9 @@ public function testSpecificSuggestionsAlter() {
   +    // Verify that specific theme call is added to the suggestions
   +    // array before the suggestions alter hook.
   
   +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -201,7 +201,9 @@ public function testUpdateContribOrder() {
   +    // Verify that 'BBB Update test' project is listed before the
   +    // 'CCC Update test' project.
   
   +++ b/core/modules/views/tests/src/Functional/Wizard/SortingTest.php
   @@ -54,7 +54,10 @@ public function testSorting() {
   +    // Verify that the nodes appear in the expected order
   +    // in a view that sorts by oldest first.
   
   @@ -79,7 +82,10 @@ public function testSorting() {
   +    // Verify that the nodes appear in the expected order
   +    // in a view that sorts by newest first.
   

   Conversely, these are wrapping too early.

   Also, I think it should be:

   Verify that the "Text Editor" select field appears in the correct location in the user interface.

   (We don't need the extra words.)

   And, "the 'BBB Update test' project".

6. +++ b/core/modules/field_ui/tests/src/Functional/EntityDisplayModeTest.php
   @@ -181,7 +181,8 @@ public function testAlphabeticalDisplaySettings() {
   +      // Verify that order of view mode is correct on page.
   
   @@ -204,7 +205,8 @@ public function testAlphabeticalDisplaySettings() {
   +      // Verify that order of view mode is correct on page.
   

   "the order of the view mode"

   ...That said, an invdividual item doesn't have an order; it has a position. Maybe let's move the comment outside the foreach and say "Verify that the view modes are in the correct order on the page."

7. +++ b/core/modules/file/tests/src/Functional/FileManagedFileElementTest.php
   @@ -59,7 +59,8 @@ public function testManagedFile() {
   +          // Verify that new file got saved.
   
   @@ -72,7 +73,8 @@ public function testManagedFile() {
   +          // Verify that new file got uploaded.
   
   +++ b/core/modules/file/tests/src/Functional/PrivateFileOnTranslatedEntityTest.php
   @@ -117,7 +117,8 @@ public function testPrivateLanguageFile() {
   +    // Verify that new file got saved.
   

   "Verify that the new file was saved".

8. +++ b/core/modules/file/tests/src/Kernel/SaveTest.php
   @@ -28,12 +28,14 @@ public function testFileSave() {
   +    // Verify that new file ID is set when saving a new file to the database.
   

   "a new file ID"

9. +++ b/core/modules/file/tests/src/Kernel/SaveTest.php
   @@ -28,12 +28,14 @@ public function testFileSave() {
   +    // Verify that file size was set correctly.
   

   "the new file size"

10. +++ b/core/modules/file/tests/src/Kernel/SaveTest.php
    @@ -45,8 +47,9 @@ public function testFileSave() {
    +    // Verify that timestamp didn't go backwards.
    

    "the timestamp"

11. +++ b/core/modules/file/tests/src/Kernel/ValidatorTest.php
    @@ -97,8 +97,10 @@ public function testFileValidateImageResolution() {
    +      // Verify that image scaled to correct width.
    ...
    +      // Verify that image scaled to correct height.
    

    "the image"

12. +++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
    @@ -118,7 +118,8 @@ public function testFailedPageCreation() {
    +    // Verify that rollback explanatory error logged to watchdog.
    

    "that the rollback explanatory error was logged"

13. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php
    @@ -241,7 +241,8 @@ public function testRevisions() {
    +    // Verify that revision vid is greater than default revision vid.
    

    "Verify that this revision's ID is greater than the default revision's ID."

14. +++ b/core/modules/node/tests/src/Functional/NodeViewTest.php
    @@ -93,7 +93,9 @@ public function testLinkHeader() {
         $title = '🐝';
    

    Aside: That's adorable!

15. +++ b/core/modules/node/tests/src/Functional/NodeViewTest.php
    @@ -93,7 +93,9 @@ public function testLinkHeader() {
    +    // To ensure that title has multi-byte characters we compare the byte
    

    There should be a comma before "we".

16. +++ b/core/modules/node/tests/src/Kernel/SummaryLengthTest.php
    @@ -90,7 +90,8 @@ public function testSummaryLength() {
    +    // Verify that teaser is less than 600 characters long.
    
    @@ -109,7 +110,8 @@ public function testSummaryLength() {
    +    // Verify that teaser is less than 200 characters long.
    

    "the teaser"

17. +++ b/core/modules/search/tests/src/Kernel/SearchSimplifyTest.php
    @@ -56,7 +56,8 @@ public function testSearchSimplifyUnicode() {
    +      // Verify that nothing is removed from string.
    

    "from the string."

18. +++ b/core/modules/system/tests/src/Functional/Entity/EntityRevisionsTest.php
    @@ -145,7 +145,8 @@ protected function runRevisionsTests($entity_type) {
    +      // Verify that revision ID changed.
    

    "the revision ID".

19. +++ b/core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php
    @@ -361,8 +361,8 @@ public function testVeryLongLineWrap() {
    +    // Verify that maximum line length found was maximum_line_length octets.
    

    "the maximum line length"

    Also, the $ is missing from $maximum_line_length.

20. +++ b/core/modules/system/tests/src/Functional/Module/InstallTest.php
    @@ -52,9 +52,11 @@ public function testEnableUserTwice() {
    +    // Verify that system module version is > 0.
    ...
    +    // Verify that user module version is > 0.
    

    "the System/User module version" (Note the capitalization as well, since module names are proper nouns.)

21. +++ b/core/modules/system/tests/src/Functional/System/CronRunTest.php
    @@ -78,7 +78,8 @@ public function testAutomatedCron() {
    +    // Verify that cron runs when the cron interval is passed.
    

    This is wrong in the assertion message, too, but "has passed" rather than "is passed".

22. +++ b/core/modules/system/tests/src/Kernel/Action/ActionTest.php
    @@ -43,7 +43,8 @@ protected function setUp(): void {
    +    // Verify that action definitions are found.
    

    "the action definitions"

23. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
    @@ -99,7 +99,8 @@ public function testTranslationUI() {
    +      // Verify that term does not have translations.
    

    "the term"

24. +++ b/core/modules/user/tests/src/Functional/UserRegistrationTest.php
    @@ -260,7 +260,8 @@ public function testRegistrationDefaultValues() {
    +    // Verify that correct creation time.
    

    "Verify that the creation time is correct."

25. +++ b/core/modules/views_ui/tests/src/Functional/PreviewTest.php
    @@ -116,12 +116,14 @@ public function testPreviewUI() {
    +    // Verify that statistics shown above the preview.
    ...
    +    // Verify that statistics shown below the preview.
    

    I think we don't need these comments; the inline comments above the hunks are sufficient.

26. +++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
    @@ -375,9 +375,12 @@ public function testRenderDifferentWeight() {
    +    // Verify that rendering a JavaScript file above jQuery.
    

    "that a JavaScript file is rendered before jQuery."

27. +++ b/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php
    @@ -397,7 +400,8 @@ public function testAlter() {
    +    // Verify that altering JavaScript weight through the alter hook.
    

    "Verify that JavaScript weight is correctly altered by the alter hook."

28. +++ b/core/tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
    @@ -138,7 +138,9 @@ public function testSetGet() {
    +    // Verify that created time is correct.
    
    @@ -147,7 +149,9 @@ public function testSetGet() {
    +    // Verify that created time is correct.
    
    @@ -155,7 +159,9 @@ public function testSetGet() {
    +    // Verify that created time is correct.
    
    @@ -165,7 +171,9 @@ public function testSetGet() {
    +    // Verify that created time is correct.
    
    @@ -175,7 +183,9 @@ public function testSetGet() {
    +    // Verify that created time is correct.
    
    @@ -317,7 +327,9 @@ public function testGetMultiple() {
    +    // Verify that created time is correct.
    
    @@ -395,7 +407,9 @@ public function testSetMultiple() {
    +    // Verify that created time is correct.
    

    "the created time"

29. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
    @@ -39,7 +39,8 @@ public function testDefaultJoin() {
    +      // Verify that results returned in correct order.
    
    @@ -65,7 +66,8 @@ public function testLeftOuterJoin() {
    +      // Verify that results returned in correct order.
    
    @@ -87,7 +89,8 @@ public function testGroupBy() {
    +      // Verify that results returned in correct order.
    
    @@ -124,8 +127,9 @@ public function testGroupByAndHaving() {
    +      // Verify that results returned in correct order.
    
    +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectOrderedTest.php
    @@ -23,7 +23,8 @@ public function testSimpleSelectOrdered() {
    +      // Verify that results returned in correct order.
    
    @@ -75,7 +76,8 @@ public function testSimpleSelectOrderedDesc() {
    +      // Verify that results returned in correct order.
    

    "Verify that the results are returned in the correct order".

    Also, I think this is another case where we can move the comment outside the foreach.

30. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
    @@ -409,7 +413,8 @@ public function testJoinConditionObject() {
    -      $this->assertTrue($record->$priority_field >= $last_priority, 'Results returned in correct order.');
    +      // Verify that taskless person not selected.
    +      $this->assertGreaterThanOrEqual($last_priority, $record->$priority_field);
    

    Wrong comment. :)

31. +++ b/core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
    @@ -101,7 +101,8 @@ protected function transactionInnerLayer($suffix, $rollback = FALSE, $ddl_statem
    +    // Verify that transaction depth is has increased with new transaction.
    

    This is wrong in the original assertion message as well, but "Verify that the transaction depth increases with a new transaction."

32. +++ b/core/tests/Drupal/Tests/Component/Serialization/JsonTest.php
    @@ -69,7 +69,8 @@ public function testEncodingAscii() {
    +    // Verify that a JSON encoded string is larger than the source string.
    

    "JSON-encoded"

33. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
    @@ -69,7 +69,8 @@ public function testBubblingWithoutPreRender() {
    +    // Verify that the element was retrieved from cache.
    

    "from the cache."

A lot of bullets, but all small fixes. :) Thanks!

Thanks. Looks great to the eyes of a non-English-native human being. :)

Thanks @jungle and @mondrake. A few more "the" need to be added. (English articles... they're everywhere, like insects.)

1. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -399,7 +399,8 @@ protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholde
   +    // Verify that the BigPipe start signal appears before stop signal.
   

   Missing the second "the" ("before the stop signal").

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -148,9 +148,10 @@ protected function doTestBasicTranslation() {
   +      // Verify that translation creation timestamp of the target translation
   

   "the translation creation timestamp"

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -148,9 +148,10 @@ protected function doTestBasicTranslation() {
   +      // default language for translatable created field.
   

   "the translatable created field"

4. +++ b/core/modules/field_ui/tests/src/Functional/EntityDisplayModeTest.php
   @@ -179,9 +179,10 @@ public function testAlphabeticalDisplaySettings() {
   +    // Verify that the order of view mode is correct on page.
   
   @@ -202,9 +203,10 @@ public function testAlphabeticalDisplaySettings() {
   +    // Verify that the order of view mode is correct on page.
   

   "Verify that the order of the view modes is correct on the page."

5. +++ b/core/modules/file/tests/src/Kernel/ValidatorTest.php
   @@ -97,8 +97,10 @@ public function testFileValidateImageResolution() {
   +      // Verify that the image scaled to correct width.
   ...
   +      // Verify that the image scaled to correct height.
   

   I think for these two we could combine it and just say "Verify that the image was scaled to the correct width and height" above the hunk.

6. +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateCronTest.php
   @@ -113,8 +113,10 @@ public function testUpdateCron() {
   +    // Verify that timestamp is updated.
   ...
   +    // Verify that last checked is updated.
   

   Similarly here, "Verify that the timestamp and last checked fields were both updated."

7. +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateTest.php
   @@ -142,10 +142,13 @@ public function testUpdateImportSourceRemote() {
   +    // Verify that translation of contrib_module_one is imported.
   ...
   +    // Verify that translation of contrib_module_one is updated.
   

   We can group these too; "Verify that the translation of contrib_module_one is imported and updated.

8. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeSuggestionsAlterTest.php
   @@ -113,7 +113,9 @@ public function testSpecificSuggestionsAlter() {
   +    // Verify that specific theme call is added to the suggestions array before
   

   "a specific theme call"

9. +++ b/core/modules/user/tests/src/Functional/UserRegistrationTest.php
   @@ -260,7 +260,8 @@ public function testRegistrationDefaultValues() {
   +    // Verify that the correct creation time is correct.
   

   Oops, the word "correct" is repeated. It should be merely "Verify that the creation time is correct."

10. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaOrderTest.php
    @@ -70,7 +70,9 @@ public function testAreaOrder() {
    +    // Verify that the block bartik-powered is positioned before block
    +    // bartik-branding.
    

    I think we can omit this comment; the comment above is sufficient.

11. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
    @@ -37,9 +37,10 @@ public function testDefaultJoin() {
    +    // Verify that the results returned in correct order.
    
    @@ -63,9 +64,10 @@ public function testLeftOuterJoin() {
    +    // Verify that the results returned in correct order.
    
    @@ -85,9 +87,10 @@ public function testGroupBy() {
    +    // Verify that the results returned in correct order.
    

    All of these should be "Verify that the results are returned in the correct order."

12. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
    @@ -409,7 +413,8 @@ public function testJoinConditionObject() {
    -      $this->assertTrue($record->$priority_field >= $last_priority, 'Results returned in correct order.');
    +      // Verify that the results returned in correct order.
    +      $this->assertGreaterThanOrEqual($last_priority, $record->$priority_field);
    
    +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectOrderedTest.php
    @@ -23,7 +23,8 @@ public function testSimpleSelectOrdered() {
    -      $this->assertTrue($record->age >= $last_age, 'Results returned in correct order.');
    +      // Verify that results returned in correct order.
    +      $this->assertGreaterThanOrEqual($last_age, $record->age);
    
    @@ -75,7 +76,8 @@ public function testSimpleSelectOrderedDesc() {
    -      $this->assertTrue($record->age <= $last_age, 'Results returned in correct order.');
    +      // Verify that results returned in correct order.
    +      $this->assertLessThanOrEqual($last_age, $record->age);
    

    A few more to move outside their loops.

13. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRevisionTranslationTest.php
    @@ -51,8 +51,11 @@ public function testNewRevisionAfterTranslation() {
    +    // Verify that the saved translation in new revision has a newer revision
    

    "the new revision"

    Actually "in" also doesn't make sense; I would say:

    "Verify that the saved translation for the new translation has a newer revision ID."

On it.

Addressing #42.

@xjm, I have gone through all your review points and updated the patch accordingly.

Thanks!

Getting very close, thanks!

1. +++ b/core/modules/aggregator/tests/src/Functional/FeedLanguageTest.php
   @@ -83,7 +83,8 @@ public function testFeedLanguage() {
   +      // Verify that feed items were created.
   

   // Verify that the feed items were created.

   ;)

2. +++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php
   @@ -71,7 +71,9 @@ public function testInstallProfileConfigOverwrite() {
   +    // Verify that the site UUID '$site_uuid' is valid.
   

   Now that this is a comment, '$site_uuid' doesn't tell us anything. We just said "site UUID".

   // Verify that the site UUID is valid.

3. +++ b/core/modules/field/tests/src/Kernel/FieldValidationTest.php
   @@ -66,8 +66,9 @@ public function testFieldConstraints() {
   +    // The test is only valid if the field cardinality is greater than or equal
   +    // to 2.
   +    $this->assertGreaterThanOrEqual(2, $cardinality);
   

   Maybe out of scope, but this seems clearer:

   // The test is only valid if the field cardinality is greater than 1.
   $this->assertGreaterThan(1, $cardinality);

4. +++ b/core/modules/file/tests/src/Functional/PrivateFileOnTranslatedEntityTest.php
   @@ -117,7 +117,8 @@ public function testPrivateLanguageFile() {
   +    // Verify that new file got saved.
   

   // Verify that the new file got saved.

5. +++ b/core/modules/file/tests/src/Kernel/SaveTest.php
   @@ -45,8 +47,9 @@ public function testFileSave() {
   -    $this->assertTrue($file->getChangedTime() >= $file->getChangedTime(), "Timestamp didn't go backwards.", 'File');
        $loaded_file = File::load($file->id());
   +    // Verify that the timestamp didn't go backwards.
   +    $this->assertGreaterThanOrEqual($file->getChangedTime(), $loaded_file->getChangedTime());
   

   Yikes, buggy assertion. Glad to see this getting fixed here. I had to double-take, but confirming this is correct.

6. +++ b/core/modules/locale/tests/src/Functional/LocaleUpdateTest.php
   @@ -142,10 +142,12 @@ public function testUpdateImportSourceRemote() {
   +    // Verify that translation of contrib_module_one is imported and updated.
   ...
   +    // Verify that translation of contrib_module_two is updated.
   
   @@ -196,10 +198,12 @@ public function testUpdateImportSourceLocal() {
   +    // Verify that translation of contrib_module_one is imported.
   ...
   +    // Verify that translation of contrib_module_two is updated.
   

   ...the translation...

7. +++ b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php
   @@ -241,7 +241,8 @@ public function testRevisions() {
   +    // Verify that this revision's ID is greater than the default revision's ID.
   +    $this->assertGreaterThan($default_revision_vid, $new_node_revision->getRevisionId());
   

   I think we can kill this comment, since it's just an English translation of the assertion, without any added context or information.

8. +++ b/core/modules/node/tests/src/Functional/Views/NodeLanguageTest.php
   @@ -144,7 +144,7 @@ public function testLanguages() {
   -    $this->assertTrue($pos_es_max < $pos_fr_min, 'Spanish translations appear before French on ' . $message);
   +    $this->assertLessThan($pos_fr_min, $pos_es_max, "Spanish translations does not appear before French on $message");
   

   A) "translations" shouldn't be plural here. If it should be plural for some reason, we need "do not" not "does not".

   B) Although we haven't formally resolved #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages (ping/bump), the emerging consensus is that "negative" assertion messages like this are confusing and weird, and we should use something like:

   "The Spanish translation should appear before the French one on $message."

9. +++ b/core/modules/node/tests/src/Kernel/SummaryLengthTest.php
   @@ -90,7 +90,8 @@ public function testSummaryLength() {
        // Render the node as a teaser.
        $content = $this->drupalBuildEntityView($node, 'teaser');
   -    $this->assertTrue(strlen($content['body'][0]['#markup']) < 600, 'Teaser is less than 600 characters long.');
   +    // Verify that the teaser is less than 600 characters long.
   +    $this->assertLessThan(600, strlen($content['body'][0]['#markup']));
   

   Given the previous 2 lines of context, I don't think the new comment here adds anything.

10. +++ b/core/modules/node/tests/src/Kernel/SummaryLengthTest.php
    @@ -109,7 +110,8 @@ public function testSummaryLength() {
         // Render the node as a teaser again and check that the summary is now only
         // 200 characters in length and so does not include 'What is a Drupalism?'.
         $content = $this->drupalBuildEntityView($node, 'teaser');
    -    $this->assertTrue(strlen($content['body'][0]['#markup']) < 200, 'Teaser is less than 200 characters long.');
    +    // Verify that the teaser is less than 200 characters long.
    +    $this->assertLessThan(200, strlen($content['body'][0]['#markup']));
    

    Same here.

11. +++ b/core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php
    @@ -361,8 +361,9 @@ public function testVeryLongLineWrap() {
    +    // Verify that the maximum line length found was $maximum_line_length
    +    // octets.
    +    $this->assertLessThanOrEqual(1000, $maximum_line_length);
    

    Uhh, that's not what the assertion is testing. Or at least, that's a really confusing comment.

    Where's 1000 coming from? That's what this comment should explain.

    // Verify that the maximum line length found was less than or equal to [whatever it is that makes us assert 1000 here].

12. +++ b/core/modules/system/tests/src/Functional/Module/InstallTest.php
    @@ -52,9 +52,11 @@ public function testEnableUserTwice() {
         $version = drupal_get_installed_schema_version('system', TRUE);
    -    $this->assertTrue($version > 0, 'System module version is > 0.');
    +    // Verify that the System module version is > 0.
    +    $this->assertGreaterThan(0, $version);
         $version = drupal_get_installed_schema_version('user', TRUE);
    -    $this->assertTrue($version > 0, 'User module version is > 0.');
    +    // Verify that the User module version is > 0.
    +    $this->assertGreaterThan(0, $version);
    

    Given context, I don't think either of these new comments are needed/helpful.

13. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
    @@ -318,6 +318,8 @@ public function testThemeMetaData() {
         $test_mtime = !empty($themes['bartik']->info['mtime']) ? $themes['bartik']->info['mtime'] : 0;
         // Ensure the mtime field contains a number that is greater than zero.
    +    // Verify that the bartik.info.yml file modification time field contains a
    +    // timestamp.
         $this->assertIsNumeric($test_mtime);
         $this->assertGreaterThan(0, $test_mtime);
    

    Not sure why we're adding this comment here. Seems out of scope (and unneeded).

14. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -201,7 +201,9 @@ public function testUpdateContribOrder() {
    +    // Verify that the 'BBB Update test' project is listed before the
    +    // 'CCC Update test' project.
    

    In #35.5, @xjm complained this wasn't wrapped properly. However, given 'CCC Update test' as a distinct thing, maybe this is fine not to wrap the "'CCC" part to the previous line. +1 for keeping this as-is...

15. +++ b/core/modules/user/tests/src/Functional/UserRegistrationTest.php
    @@ -260,7 +260,8 @@ public function testRegistrationDefaultValues() {
    +    // Verify that the creation time is correct.
    +    $this->assertGreaterThan(REQUEST_TIME - 20, $new_user->getCreatedTime());
    

    Probably out of scope, but it'd be even more helpful to explain why "- 20" here. ;)

16. +++ b/core/modules/user/tests/src/Kernel/UserAccountFormFieldsTest.php
    @@ -113,7 +113,7 @@ protected function assertFieldOrder(array $elements) {
    -    $this->assertTrue($name_weight < $pass_weight, "'name' field weight ($name_weight) is smaller than 'pass' field weight ($pass_weight).");
    +    $this->assertLessThan($pass_weight, $name_weight, "'name' field weight ($name_weight) is not smaller than 'pass' field weight ($pass_weight).");
    

    If we're going to change this message, let's do:

    "'name' field weight ($name_weight) should be smaller than 'pass' field weight ($pass_weight)."

17. +++ b/core/modules/views/tests/src/Functional/Handler/FieldEntityOperationsTest.php
    @@ -75,7 +75,8 @@ public function testEntityOperations() {
    +        // Verify that there are operations.
    +        $this->assertNotEmpty($operations);
    
    +++ b/core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
    @@ -373,7 +373,8 @@ public function testMissingRelationship() {
    +    // Ensure the result of the view is not empty.
    +    $this->assertNotEmpty($view->result);
    

    Comment doesn't add anything.

18. +++ b/core/modules/views/tests/src/Unit/ViewsDataTest.php
    @@ -163,7 +163,8 @@ public function testFetchBaseTables() {
    -      $this->assertTrue($prev['weight'] <= $current['weight'] && $prev['title'] <= $prev['title'], 'The tables are sorted as expected.');
    +      $this->assertLessThanOrEqual($current['weight'], $prev['weight']);
    +      $this->assertLessThanOrEqual($prev['title'], $prev['title']);
    

    Existing assertion bug, but per above, the 2nd here should be:

    $this->assertLessThanOrEqual($current['title'], $prev['title']);

19. +++ b/core/tests/Drupal/KernelTests/Core/Database/DeleteTruncateTest.php
    @@ -58,7 +58,8 @@ public function testSimpleDelete() {
         $num_records_before = $this->connection->query("SELECT COUNT(*) FROM {test}")->fetchField();
    -    $this->assertTrue($num_records_before > 0, 'The table is not empty.');
    +    // Verify that the table is not empty.
    +    $this->assertNotEmpty($num_records_before);
    

    Given context, I don't think the comment adds anything.

20. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
    @@ -169,7 +169,8 @@ public function testSchema() {
    -    $this->assertTrue($max2 > $max1, 'The serial is monotone.');
    +    // Verify that the serial is monotone.
    
    @@ -189,7 +190,8 @@ public function testSchema() {
    +    // Verify that the serial is monotone.
    

    This was an existing usage, but "monotone" seems wrong here. ;)

    I think these want to say:

    // Verify that the serial is monotonically increasing.
    ?

    I could be totally off, but I've never heard of using "monotone" like this, and Google isn't turning up anything that contradicts me.

21. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
    @@ -122,10 +125,11 @@ public function testGroupByAndHaving() {
    +    // Verify that the results are returned in the correct order.
         foreach ($result as $record) {
           $num_records++;
    -      $this->assertTrue($record->$count_field >= 2, 'Record has the minimum count.');
    -      $this->assertTrue($record->$count_field >= $last_count, 'Results returned in correct order.');
    +      $this->assertGreaterThanOrEqual(2, $record->$count_field);
    +      $this->assertGreaterThanOrEqual($last_count, $record->$count_field);
    

    Seems fairly self-documenting, but if we're going to preserve a comment here, I'd keep the "minimum count" one, too.:

    foreach (...) {
      $num_records++;
      // Verify that the record has the minimum count.
      $this->assertGreatThanOrEqual(2, ...);
      // Verify that the results are returned in the correct order.
      ...
    }
    

22. +++ b/core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
    @@ -101,7 +101,8 @@ protected function transactionInnerLayer($suffix, $rollback = FALSE, $ddl_statem
    -    $this->assertTrue($depth < $depth2, 'Transaction depth is has increased with new transaction.');
    +    // Verify that the transaction depth increases with a new transaction.
    +    $this->assertLessThan($depth2, $depth);
    

    Existing weirdness, but if the message is "... increases", it'd be less cognitive dissonance to use:

    $this->assertGreaterThan($depth, $depth2);

23. +++ b/core/tests/Drupal/Tests/Component/Graph/GraphTest.php
    @@ -166,7 +166,7 @@ protected function assertWeights($graph, $expected_orders) {
    +        $this->assertLessThan($graph[$vertex]['weight'], $graph[$previous_vertex]['weight'], sprintf('Weights of %s and %s are not correctly relative to each other', $previous_vertex, $vertex));
    

    This message is weird:

    A) "correctly" isn't grammatically correct. ;)

    B) Again, we (or at least I *grin*) don't want "negative" assertions like this.

    sprintf("Weight of vertex %s should be less than vertex %s.", $previous_vertex, $vertex)

24. +++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
    @@ -69,7 +69,8 @@ public function testBubblingWithoutPreRender() {
    +    $this->assertGreaterThan(0, strlen($this->renderer->renderRoot($element)));
    

    Maybe this is cleaner?

    $this->assertNotEmpty($this->renderer->renderRoot($element))
    

    ?

25. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -487,7 +487,8 @@ public function testRenderSorting() {
         // The lowest weight element should appear last in $output.
    -    $this->assertTrue(strpos($output, $second) > strpos($output, $first), 'Elements were sorted correctly by weight.');
    +    // Verify that elements were sorted correctly by weight.
    +    $this->assertGreaterThan(strpos($output, $first), strpos($output, $second));
    

    Given context, I think we should remove the new comment and leave the original.

    If we actually need to keep the new comment, it should wrap into the previous comment line.

26. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTest.php
    @@ -522,7 +523,8 @@ public function testRenderSortingWithSetHashSorted() {
         // The elements should appear in output in the same order as the array.
    -    $this->assertTrue(strpos($output, $second) < strpos($output, $first), 'Elements were not sorted.');
    +    // Verify that elements were not sorted.
    +    $this->assertLessThan(strpos($output, $first), strpos($output, $second));
    

    Same here.

Thanks!
-Derek

Thanks, @dww for such a detailed review of the patch.

Addressing #48,

Except #48.14, #48.15 and #48.21 Patch includes all the recommended changes.

#48.14
No action was required.

#48.15
Not sure why "- 20". :P

#48.21
To avoid the addition of comment inside loop here, a single comment was added outside.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest.php
@@ -122,10 +125,11 @@ public function testGroupByAndHaving() {
+    // Verify that the results are returned in the correct order.
     foreach ($result as $record) {
       $num_records++;
-      $this->assertTrue($record->$count_field >= 2, 'Record has the minimum count.');
-      $this->assertTrue($record->$count_field >= $last_count, 'Results returned in correct order.');
+      $this->assertGreaterThanOrEqual(2, $record->$count_field);
+      $this->assertGreaterThanOrEqual($last_count, $record->$count_field);

#48.24
As we want to ensure that the element was retrieved from the cache.

Thanks!

Drive-by Hit-n-Run fixing of missing semi-colon.

Patch fails on #48.18:

+++ b/core/modules/views/tests/src/Unit/ViewsDataTest.php
@@ -163,7 +163,8 @@ public function testFetchBaseTables() {
-      $this->assertTrue($prev['weight'] <= $current['weight'] && $prev['title'] <= $prev['title'], 'The tables are sorted as expected.');
+      $this->assertLessThanOrEqual($current['weight'], $prev['weight']);
+      $this->assertLessThanOrEqual($prev['title'], $prev['title']);

Existing assertion bug, but per above, the 2nd here should be:

$this->assertLessThanOrEqual($current['title'], $prev['title']);

Hi,
@Spokje, I can confirm your suggestions.

I think the comparison between the current and previous titles are not really required there. We can test the order and moreover we can also test for empty title.

Rerolled patch #55, please review.

Reroll patch #55.

This patch only did a reroll operation, please review. Thanks

Reroll only.

All review points have been addressed, this looks ready to go.

Re-Rolling as we speak

Reroll of #64

Since this is a simple reroll: Back to RTBC as per #65

Initially was going to ask why some assertion custom messages were converted to comments and some left, but actually all the decisions look right to me - we show the raw comparison where the message wasn't adding anything, and then the custom messages that are left are explaining what's actually happening.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!