Replace assertions involving calls to empty() with assertEmpty()/assertNotEmpty()/assertArrayNotHasKey()

Messages did not remove yet.

Will look into it later tonight, it's about 14:35 p.m. here, but please feel free to unassign me if someone wants to take over early. Thanks!

Rerolled patch first.

Making CI happy.

+++ b/core/tests/Drupal/Tests/Core/Entity/Enhancer/EntityRouteEnhancerTest.php
@@ -86,7 +86,7 @@ public function testEnhancer() {
-    $this->assertEmpty($defaults['view_mode']);
+    $this->assertArrayNotHasKey('view_mode', $defaults);

As some of them were replaced by assertArrayNotHasKey, so changing the title to reflect it.

Seems random failures.

Messages did not remove yet.

As I commented in #4, setting back to NW to remove redundant assertion messages.

@jungle I am working on it

@jungle as a comment on #4 I have to remove the message.

please review the patch

@kishor_kolekar Thanks, will do after testing finishes running. But I am not eligible any more to RTBC as I did contribute patch(es) as you.

Oops, needs reroll.

Straight up reroll of patch in #15 first.
I still see some assertion messages in the patch, so that needs to be addressed next IMHO.

Re-reroll of #15

*sigh* Use brain, then upload patch...

Retry

Ok, NR to get an answer on when to remove the assertion messages.

Looking at some other similar issues, it looks like: "Removed all assertion messages, except when they're used in a loop".

Is that correct?

@Spokje, no explicit consent reached yet as I know. "Removed all assertion messages, except when they're used in a loop" is the rule we followed so far.

The main purpose is to remove redundant assertion messages, but this is too broad to follow, so "Removed all assertion messages, except when they're used in a loop" was proposed. In fact, by following this rule, a few of the removals were rejected, however, most of them were accepted.

So let's do it by following it for the first iteration. And next, to revert removals according to others' review if necessary.

Thanks!

It is my understanding from working on #3131807: [meta] Replace assertions involving calls to isset() with more appropriate assertions that assertions messages are to be removed, except in loops, or otherwise required for clarity.

Thanks @jungle and @quiteone.

Attached is a patch with deprecations messages removed except when directly in a loop.

Except for classes Drupal\Tests\node\Functional\NodeAdminTest and Drupal\Tests\views_ui\Functional\ViewEditTest
where the message didn't seem helpful to me and was deleted.

Let's see where this gets us.

Right...Let's try that again....

Attached is a patch with deprecations messages removed except when directly in a loop.

Except for classes Drupal\Tests\node\Functional\NodeAdminTest and Drupal\Tests\views_ui\Functional\ViewEditTest
where the message didn't seem helpful to me and was deleted.

Let's see where this gets us.

@mondrake

It takes less to go through and make a new patch than writing it all up, hope nobody minds.

Fine with me, for me it's completely unclear when a message should stay or not at the moment.

Thanks @Spokje and @mondrake!

#30, failed. As an alternative, we can ignore removing assertion messages as @mondrake just agreed with me on #3126965: [backport] Replace assert* involving count() and an integer literal with assertCount(). Patch in #8 is the one without removing assertion messages. we can continue with that optionally,

Found a few and fixed. with a loose regex, assert.+\(empty

Changing status to NW.

1). Patch needs to be re-rolled. Adding it for retest against 9.1.x.

2). There are also many cases where the test author was trying to add additional information. Here are examples of information that was being added by the extra (unused) parameter, beyond other inline comments, which I think we should move to inline comments:

A.

-    $this->assertTrue(!empty($element), 'Make sure the URL appears when re-editing the action.');
+    $this->assertNotEmpty($element);

Ensure that the URL appears when re-editing the action.

B.

-    $this->assertFalse(empty($correct_titles), 'Aggregator feed page is available and has the correct title.');
+    $this->assertNotEmpty($correct_titles);

Verify that the aggregator feed page is available and has the correct title.

Similarly, we can add Verify that as a prefix and add the strings as inline comments.

Re-roll patch created, Please review.

NW for #41.2

@sja112 inline comment added according to #41, Please review.

@vsujeetkumar,

The way you have added the inline comments for the two examples I suggested. In a similar manner You need to add inline comments throughout code where ever you feel it's adding valuable information and is appropriate.

@sja112 can you please provide some reference doc, where it is mentioned that, we need to add comment like this?

@vsujeetkumar,

Here you can see in the related issue, as reviewed by @xjm
#3132964#12: assertResponse() does not actually support a $message parameter, so stop passing one

In many places, the information being provided in the extra parameter is completely documented by the preceding drupalGet() and/or inline comments above. However, there are also many cases where the test author was trying to add additional information.

In a similar manner, we can preserve some valuable information if the inline comment is not present.

Assigning this to add the inline comments.

@sja112 you are choosing wrong example(#3132964) for this issue, because assertEmpty()/assertNotEmpty()/assertArrayNotHasKey() all three can accept the "$message" parameter, According to me message should add along with the above asserts, No need to add inline comment, Please review.

Updated patch to include inline comments.

Rerolled patch #53, please review.

Last #56 failed to apply , so re-rolling the patch , kindly review.

Once more unto the breach, dear friends, once more...

Used 3131348-57.patch as start for a reroll on 9.3.x whilst also (trying to) making sure all the messages are kept. Previous patches were (mostly) deleting those.

Well, at least TestBot liked it.

For the archaeologists out there, attached is a raw reroll diff between 3131348-57.patch and the changes in the MR at the time of posting this comment, which is actually bigger than the original patch..

• Rebased MR on 9.4.x
• Merged latest commits on 9.4.x
• Reactivated MR, @yogeshmpawar seeing this is the second MR I've encountered you put in draft, is there some logic behind it? I personaly don't see a reason to do so, it basically renders the whole MR unmergable IMHO.

The MR looks good. A couple of nitpicks.

Thanks @daffie for his (per usual) eagle-eyed review. Solved all threads.

All changes look good to me.
This issue improves the readability of core.
For me it is RTBC.

@Spokje: Great work!

Seem to be missing a test run on the MR?

Where there was too much noise from an MR displayed in a d.o. issue, there now seems to be none (no comments nor testruns on MR are showing)

I'm a 100% sure there _was_ a testrun, because a push to an MR automagically always triggered a testrun.

EDIT: Also that seemed to have changed, I just merged the latest commits of 9.4.x into the MR (https://git.drupalcode.org/project/drupal/-/merge_requests/1051#note_59278) but nothing happenend testwise.

Also the MR field under the IS stays grey, whilst after clicking through the MR shows as mergeable, which means it should be green.

Attached plain diff from current MR to test.
Back to RTBC and let's hope this is the only issue suffering from this.

I think there's a generalised issue with Drupal.org/gitlab integration - this was the first issue I saw without a test run, but have since seen several others. Have asked in #drupal-infrastructure

GItLab integration seems back to normal again.

Merged latest commits from 9.4.x-dev into MR and hid the .diff file, since the MR is now leading again.

Committed and pushed 09795b6e930 to 10.0.x and 61a44d1b1ee to 9.4.x. Thanks!

Committed aaa470e and pushed to 9.3.x. Thanks!

Backported to 9.3.x to keep tests aligned and this is a test-only change.