Replace assertions involving calls to is_array() with assertIsArray()/assertIsNotArray()

@kapilkumar0324 thanks - patch doesn't apply though, and git grep -e '>assert.*is_array' yields 25+ results, so there are some that are not addressed here

Please review!

Did the mentioned git grep -e '>assert.*is_array' and tried to fix them all.
Lets see what TestBot thinks.

@Suresh Prabhu Parkala Oops, sorry! Crossing messages/patches there.

Fixed typo in previous patch.

@kapilkumar0324 @Suresh you probably need to update your dev branch before creating the branch, git pull origin, to avoid patch apply failures on the bot.

Re #6:

1. +++ b/core/modules/views/tests/src/Functional/Handler/AreaTest.php
   @@ -175,7 +175,8 @@ public function testRenderAreaToken() {
   -      $this->assertTrue(!empty($available[$type]) && is_array($available[$type]));
   +      $this->assertNotEmpty($available[$type]) && $this->assertIsArray($available[$type]);
   +
   

   should be split in 2 separate assertions:
   - $this->assertTrue(!empty($available[$type]) && is_array($available[$type]));
   + $this->assertNotEmpty($available[$type]);
   + $this->assertIsArray($available[$type]);
   +

2. +++ b/core/modules/views/tests/src/Kernel/PluginInstanceTest.php
   @@ -65,12 +65,13 @@ protected function setUp($import_test_views = TRUE): void {
   -      $this->assertTrue(is_array($this->definitions[$type]) && !empty($this->definitions[$type]), new FormattableMarkup('Plugin type @type has an array of plugins.', ['@type' => $type]));
   +      $this->assertIsArray($this->definitions[$type]) && $this->assertNotEmpty($this->definitions[$type]);
   

   same

3. +++ b/core/modules/views/tests/src/Kernel/ViewStorageTest.php
   @@ -170,7 +170,7 @@ protected function displayTests() {
   -    $this->assertTrue(isset($values['display']['test']) && is_array($values['display']['test']), 'New display was saved.');
   +    $this->assertTrue(isset($values['display']['test'])) && $this->assertIsArray($values['display']['test']);
   

   same

4. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
   @@ -541,7 +541,7 @@ public function testTypedDataMaps() {
   -    $this->assertTrue(isset($value) && is_array($value));
   +    $this->assertTrue(isset($value)) && $this->assertIsArray($value);
   

   + $this->assertNotNull($value);
   + $this->assertIsArray($value);
   +

5. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
   @@ -1108,7 +1108,7 @@ class A {
   -            $this->assertTrue(is_array($result) && empty($result));
   +            $this->assertIsArray($$result) && $this->assertEmpty($result);
   

   same, split

6. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
   @@ -1130,7 +1130,7 @@ class A {
   -            $this->assertTrue(is_array($result) && empty($result));
   +            $this->assertIsArray($$result) && $this->assertEmpty($result);
   

   same, split

@mondrake Was actually already pondering that split. Thanks for that.

Attached patch incorporates comments in #9

Cool.

+++ b/core/modules/views/tests/src/Kernel/PluginInstanceTest.php
@@ -65,12 +64,13 @@ protected function setUp($import_test_views = TRUE): void {
-      $this->assertTrue(is_array($this->definitions[$type]) && !empty($this->definitions[$type]), new FormattableMarkup('Plugin type @type has an array of plugins.', ['@type' => $type]));
+      $this->assertIsArray($this->definitions[$type]);
+      $this->assertNotEmpty($this->definitions[$type]);

This is in a loop, so $type is useful in the failure message? Without this the developer doesn't know which plugin type failed.

The other ones all look OK.

+++ b/core/modules/views/tests/src/Kernel/PluginInstanceTest.php
@@ -65,12 +64,13 @@ protected function setUp($import_test_views = TRUE): void {
     foreach ($this->pluginTypes as $type) {
       $this->assertArrayHasKey($type, $this->definitions);
-      $this->assertTrue(is_array($this->definitions[$type]) && !empty($this->definitions[$type]), new FormattableMarkup('Plugin type @type has an array of plugins.', ['@type' => $type]));
+      $this->assertIsArray($this->definitions[$type]);
+      $this->assertNotEmpty($this->definitions[$type]);
     }

trying to put in practice my proposal in #3131946-8: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages , these could be

+      $this->assertIsArray($this->definitions[$type], "Plugin type '$type' is expected to have an array of plugins.");
+      $this->assertNotEmpty($this->definitions[$type], "Plugin type '$type' is expected to have an array of plugins.");

In case of failure in PHPUnit, this will lead to the following messaging:

for the first assertion, supposing that $this->definitions['myplugintype'] is null:

1) ArrayTest::testFailure
Plugin type 'myplugintype' is expected to have an array of plugins.
Failed asserting that null is of type "array".

for the second assertion, supposing that $this->definitions['myplugintype'] is an array, but empty:

1) EmptyTest::testFailure
Plugin type 'myplugintype' is expected to have an array of plugins.
Failed asserting that an array is not empty.

Thanks @longwave for the always valuable review and @mondrake for the code, I was scratching my head how to fix that.

New patch attached, based on #12, addressing #14 with #15

#16 looks great!

I double-checked that these methods don't exist in PHPUnit 6, which is presumably why we never used them before. For that reason, this issue can only be backported as far as 9.0.x.

Saving issue credit.

Thanks @jungle. +1 for that issue; I was worrying a bit about how many times a week we'd break HEAD by accidentally backporting new assertion types.

This looks good for the most part. Just a couple points of feedback relating to the assertion message cleanups:

1. +++ b/core/modules/views/tests/src/Kernel/PluginInstanceTest.php
   @@ -2,7 +2,6 @@
   -use Drupal\Component\Render\FormattableMarkup;
   
   @@ -65,12 +64,13 @@ protected function setUp($import_test_views = TRUE): void {
   -      $this->assertTrue(is_array($this->definitions[$type]) && !empty($this->definitions[$type]), new FormattableMarkup('Plugin type @type has an array of plugins.', ['@type' => $type]));
   +      $this->assertIsArray($this->definitions[$type], "Plugin type '$type' is expected to have an array of plugins.");
   +      $this->assertNotEmpty($this->definitions[$type], "Plugin type '$type' is expected to have plugins.");
   

   Dropping the FormattableMarkup from the assertion message is sort of out of scope, but correct, so I'll let it slide since the patch is small enough.

   However, I think we need to work on the assertion messages here. I would leave the assertIsArray() with no custom assertion message, and then make the message for the assertNotEmpty() be:
   Plugin type '$type' should contain plugins.

2. +++ b/core/modules/views/tests/src/Kernel/ViewStorageTest.php
   @@ -170,7 +170,8 @@ protected function displayTests() {
   -    $this->assertTrue(isset($values['display']['test']) && is_array($values['display']['test']), 'New display was saved.');
   +    $this->assertNotNull($values['display']['test']);
   +    $this->assertIsArray($values['display']['test']);
   

   For these two lines, the assertion message was adding information useful to someone reading the test. Removing the message is OK, but can we instead add an inline comment above these two assertions? Something like:

   Verify that the display was saved by ensuring it contains an array of values in the view data.

Reroll of #19 combined with addressing comments in #22

Not quite sure if I can put this back to RTBC, so just to be sure: NR

1. +++ b/core/modules/views/tests/src/Kernel/ViewStorageTest.php
   @@ -170,7 +170,10 @@ protected function displayTests() {
   -    $this->assertTrue(isset($values['display']['test']) && is_array($values['display']['test']), 'New display was saved.');
   +    // Verify that the display was saved by ensuring it contains an array of
   +    // values in the view data.
   +    $this->assertNotNull($values['display']['test']);
   +    $this->assertIsArray($values['display']['test']);
   

   If it's an array, it's certainly not null, even if it's empty. So assertNotNull seems redundant here. Anyway /shrug.

2. +++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
   @@ -541,7 +541,8 @@ public function testTypedDataMaps() {
   -    $this->assertTrue(isset($value) && is_array($value));
   +    $this->assertNotNull($value);
   +    $this->assertIsArray($value);
   

   Same here.

LGTM now.

+++ b/core/modules/views/tests/src/Kernel/ViewStorageTest.php
@@ -170,7 +170,10 @@ protected function displayTests() {
+    $this->assertNotNull($values['display']['test']);
+    $this->assertIsArray($values['display']['test']);

+++ b/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
@@ -541,7 +541,8 @@ public function testTypedDataMaps() {
+    $this->assertNotNull($value);
+    $this->assertIsArray($value);

@mondrake is totally right, these two assertNotNull() are totally redundant and can be removed now. Let's get rid of those quicklike. :)

This is also now safe for 8.9.x due to the shim.

Thanks everyone!

Here's patch #23 with #25/#27 addressed

And a reroll of #28 against D8.9

In D8.9 Drupal\Tests\Component\Annotation\Doctrine\DocParserTest extends PHPUnit\Framework\TestCase which, in phpunit 6.5 doesn't know about assertIsArray nor the shim.

Went back to this->assertTrue(is_array($foo)); for that class only.

So, #28 is for D9, #33 for D8.9.

Re #25/#27:

-    $this->assertTrue(isset($values['display']['test']) && is_array($values['display']['test']), 'New display was saved.');
+    // Verify that the display was saved by ensuring it contains an array of
+    // values in the view data.
+    $this->assertNotNull($values['display']['test']);
+    $this->assertIsArray($values['display']['test']);

I disagree with the assertNotNull() here - "isset" is not the same as "not null". This would be more properly converted as

$this->assertArrayHasKey('test', $values['display']);
$this->assertIsArray($values['display']['test']);

Still, I guess it doesn't really matter, because the test will error if the array key isn't set at the time we reach assertIsArray().

Saving credit.

Committed #28 to 9.1.x and 9.0.x, and #33 to 8.9.x. Thanks!