Replace assertions involving calls to is_numeric() with assertIsNumeric()/assertIsNotNumeric()

I used this egrep -r ".*assert.*is_numeric" core | awk -F: '{print $1}' | nl to find lines to change.

NR for testing.

Please review the patch.

Oh my, I was commenting and uploading a my new patch when I found out it was already done. @Suresh Prabhu Parkala, to avoid more than one person working on a patch please assign it to yourself. Or, in this case, since it was only an hour since I uploaded my patch maybe ask first or wait a day?

@jungle, I have looked at the two issues referred to plus the Meta for this one and I don't see any decision in those issues that states that messages are to be moved from all assertions. This isn't an area I know much about, maybe that requirement should be added to the Meta?

I was a bit overzealous in converting the assertions.

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
@@ -316,8 +316,9 @@ public function providerSortByTitleProperty() {

-    $this->assertTrue(($expected < 0 && $result < 0) || ($expected > 0 && $result > 0) || ($expected === 0 && $result === 0), 'Numbers are either both negative, both positive or both zero.');
+    $this->assertIsNumeric($expected);

I think the assertion message here is worth converting into a comment - it's a long comparison to scan.

The method assertIsNumeric() does not exist for Drupal 8 and the patch does not apply for 8.8.

Postponing this issue on #3126787: [D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7.

Patch looks good for 9.0. Just 1 nitpick:

+++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
@@ -286,7 +286,8 @@ public function testModuleMetaData() {
+    $this->assertTrue(($test_mtime > 0));

@@ -317,7 +318,8 @@ public function testThemeMetaData() {
+    $this->assertTrue(($test_mtime > 0));

Double round brackets are not needed.

The shim is committed to 8.9.x, which might be as far as it goes. So this can continue.

1. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
   @@ -286,7 +286,8 @@ public function testModuleMetaData() {
   +    $this->assertTrue(($test_mtime > 0));
   
   @@ -317,7 +318,8 @@ public function testThemeMetaData() {
   +    $this->assertTrue(($test_mtime > 0));
   

   These can be $this->assertGreaterThan(0, $test_mtime);

2. +++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
   @@ -316,8 +316,9 @@ public function providerSortByTitleProperty() {
   +    $this->assertTrue(($expected < 0 && $result < 0) || ($expected > 0 && $result > 0) || ($expected === 0 && $result === 0));
   

   Removal of the custom $message is out of scope of this patch. (It could be part of a general issue chain to remove redundant messages, but not here specifically)

Fixes for #19 and #21.1.

#21,2. No change. In #5 it was requested that messages are to be removed. See also #8 and #9.

Re. #21.2, in this case this is in an assert method. If you drop the message entirely, all you'll get in case of test failure will be

Failed asserting that false is true.

which is very obscure since you were calling $this->assertBothNegativePositiveOrZero($expected, $result);.

So probably we may take PHPUnit's way here and come up with sth like

Failed asserting that both numbers are negative, or both positive, or both zero.

But again IMHO this is out of scope in this issue.

Yes, I see. This fixes 21.2

And for my own sanity I see that was introduced in #6 and I missed it.

Arg, I didn't mean to abort the test.

+++ b/core/tests/Drupal/Tests/Component/Utility/SortArrayTest.php
@@ -316,7 +316,8 @@ public function providerSortByTitleProperty() {
    *   Actual comparison function return value.
    */
   protected function assertBothNegativePositiveOrZero($expected, $result) {
-    $this->assertTrue(is_numeric($expected) && is_numeric($result), 'Parameters are numeric.');
+    $this->assertIsNumeric($expected);
+    $this->assertIsNumeric($result);
     $this->assertTrue(($expected < 0 && $result < 0) || ($expected > 0 && $result > 0) || ($expected === 0 && $result === 0), 'Numbers are either both negative, both positive or both zero.');
   }
 

This is a testing a component, and extending the test directly from PHPUnit\Framework\TestCase, so by itself does not have the logic to load the compatibility methods.

To get this running on D8.9, we'll have to add the trait

/**
 * Tests the SortArray component.
 *
 * @group Utility
 *
 * @coversDefaultClass \Drupal\Component\Utility\SortArray
 */
class SortArrayTest extends TestCase {

  use PhpunitCompatibilityTrait;

or, just leave this untouched in D8.9.

I think this is OK for D9 where it’s not been committed, yet. Needs a specific D8.9 patch.

Thanks. Decided to make a patch for 8.9.x. The 9.1.x patch here is the same as the one in #25, just renamed.

Alright, I'm OK with the comment from #14 and looked at the method myself, and I'm confortable since the method is so short and it's documented in the docblock that that is what it does.

Saving credit while I've pushed 9.1.x only to do an experiment about a problem I'm getting with crossposts from the bot...

Committed to 9.1.x, 9.0.x, and 8.9.x. Thanks!