Replace usages of assertArrayEquals, deprecate it

Thanks for the patch.

1) Patch applies cleanly to 9.1 and deprecation starts with that version so I didn't try it on previous versions.

2) Searched for assertArrayEquals after applying the patch and only found the code that was added in the patch.

./core/tests/Drupal/Tests/UnitTestCaseTest.php:   * Tests deprecation of the ::assertArrayEquals method.
./core/tests/Drupal/Tests/UnitTestCaseTest.php:   * @expectedDeprecation Drupal\Tests\UnitTestCase::assertArrayEquals is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing or ::assertSame instead. See https://www.drupal.org/node/TODO
./core/tests/Drupal/Tests/UnitTestCaseTest.php:    $this->assertArrayEquals([], []);
./core/tests/Drupal/Tests/UnitTestCase.php:  protected function assertArrayEquals(array $expected, array $actual, $message = NULL) {

3) Reviewed the changes:

a) Most of the changes were assertArrayEquals to assertEquals so I checked the parameters didn't change for those.

b) A few (below) were assertArrayEquals to assertEmpty and assertArrayEquals to assertSame. I wasn't sure why assertEmpty was used in one case whereas assertSame for the rest.

1. +++ b/core/modules/views/tests/src/Unit/Plugin/area/ViewTest.php
   @@ -59,10 +59,10 @@ public function testCalculateDependencies() {
   -    $this->assertArrayEquals([], $this->viewHandler->calculateDependencies());
   +    $this->assertEmpty($this->viewHandler->calculateDependencies());
   

   Is this correct? Not mentioned in recommendations.

2. +++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
   @@ -82,8 +82,8 @@ public function testGetStates() {
   -    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getStates()));
   -    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getStates([])));
   +    $this->assertSame([], array_keys($workflow->getTypePlugin()->getStates()));
   +    $this->assertSame([], array_keys($workflow->getTypePlugin()->getStates([])));
   

3. +++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
   @@ -394,8 +394,8 @@ public function testGetTransitions() {
   -    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getTransitions()));
   -    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getTransitions([])));
   +    $this->assertSame([], array_keys($workflow->getTypePlugin()->getTransitions()));
   +    $this->assertSame([], array_keys($workflow->getTypePlugin()->getTransitions([])));
   

4. +++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
   @@ -406,29 +406,29 @@ public function testGetTransitions() {
   -    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getTransitions([])));
   +    $this->assertSame([], array_keys($workflow->getTypePlugin()->getTransitions([])));
   

5. +++ b/core/tests/Drupal/Tests/Core/Config/NullStorageTest.php
   @@ -22,7 +22,7 @@ public function testCollection() {
   -    $this->assertArrayEquals([], $collection->getAllCollectionNames());
   +    $this->assertSame([], $collection->getAllCollectionNames());
   

c) Reviewed wording of messages.

1. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -87,8 +87,14 @@ protected function getRandomGenerator() {
   +   *
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use
   +   *   ::assertEquals, ::assertEqualsCanonicalizing or ::assertSame instead.
   

   This is following a typical pattern. Side note, some use in drupal:10.0.0 and some from drupal:10.0.0 but it's pretty even:

   [mac:kristen:drupal-9.1.x-dev]$ grep -r "is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0" . | wc
          5     111    1366
   [mac:kristen:drupal-9.1.x-dev]$ grep -r "is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0" . | wc
          6     167    1887
   

   Nitpick: Add Oxford comma after ::assertEqualsCanonicalizing.

2. +   * @see https://www.drupal.org/node/TODO
   

   Needs change record so tagging this. Obviously, this will need updating afterwards. :)

3. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -87,8 +87,14 @@ protected function getRandomGenerator() {
   +    @trigger_error(__METHOD__ . " is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing or ::assertSame instead. See https://www.drupal.org/node/TODO", E_USER_DEPRECATED);
   

   Follows a typical pattern. I saw some messages with () after __METHOD__, e.g.

       @trigger_error(__METHOD__ . '() is deprecated in drupal:8.3.0 and will throw an exception from drupal:10.0.0. Use the \Drupal\Core\Url object instead. See https://www.drupal.org/node/2820197', E_USER_DEPRECATED);
   

   but I don't think that needs to be added.

   Nitpick: Add Oxford comma after ::assertEqualsCanonicalizing.

4. +++ b/core/tests/Drupal/Tests/UnitTestCaseTest.php
   @@ -0,0 +1,22 @@
   +   * @expectedDeprecation Drupal\Tests\UnitTestCase::assertArrayEquals is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing or ::assertSame instead. See https://www.drupal.org/node/TODO
   +   */
   +  public function testAssertArrayEquals() {
   +    $this->assertArrayEquals([], []);
   

   Follows a typical pattern.

   Nitpick: Add Oxford comma after ::assertEqualsCanonicalizing.

4) Moving back to "Needs work" to possibly address items in 3) and to create change record and then update the patch with the change record node id.

Thanks for the update.

1) Reviewed the change record at:

https://www.drupal.org/node/3136304

and it seems clear enough to me although I wonder if assertEmpty should be included in the list given that it's been used to replace an number of asserts in this issue, or maybe that would be confusing without an example.

2) Reviewed the interdiff:

a) Regarding assertEmpty vs assertSame:

here it could be questionable one way or another, because empty does not mean the variable under test is an array. But that does not seem the relevant part of the assertions here.

I like the simplicity of assertEmpty vs assertSame but I was wondering if the point of the tests was to ensure the value was an empty array and not just any empty value. From your comment, it seems the ones you changed don't need to check it's an array. The first change you did does check it's an empty array in two steps with the assertIsArray and then the assertEmpty. If we needed two calls for each check, then I'd prefer to go back to using assertSame with the empty array ([]) instead so it's all handled in one check. But, this approach is clear and I don't have a strong opinion.

+++ b/core/modules/views/tests/src/Unit/Plugin/area/ViewTest.php
@@ -62,6 +62,7 @@ public function testCalculateDependencies() {
+    $this->assertIsArray($this->viewHandler->calculateDependencies());
     $this->assertEmpty($this->viewHandler->calculateDependencies());

b) See the change record node id was added in place of the TODOs.

c) See the Oxford commas were added.

3) Reviewed the patch again:

a) I see a couple assertSame checks remain which might be fine per discussion above.

+++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
@@ -102,21 +102,21 @@ public function testGetStates() {
     // An empty array does not load all states.
-    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getStates([])));
+    $this->assertSame([], array_keys($workflow->getTypePlugin()->getStates([])));

+++ b/core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php
@@ -349,7 +349,7 @@ public function testDefaultValuesFormSubmit() {
 
-    $this->assertArrayEquals([], $field_list->defaultValuesFormSubmit([], $form, $form_state));
+    $this->assertSame([], $field_list->defaultValuesFormSubmit([], $form, $form_state));

b) I have the same thought from above about adding assertEmpty to the lists here but, without an example, it may be confusing.

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -87,8 +87,14 @@ protected function getRandomGenerator() {
+   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use
+   *   ::assertEquals, ::assertEqualsCanonicalizing, or ::assertSame instead.

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -87,8 +87,14 @@ protected function getRandomGenerator() {
+    @trigger_error(__METHOD__ . " is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing, or ::assertSame instead. See https://www.drupal.org/node/3136304", E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/UnitTestCaseTest.php
@@ -0,0 +1,22 @@
+   * @expectedDeprecation Drupal\Tests\UnitTestCase::assertArrayEquals is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing, or ::assertSame instead. See https://www.drupal.org/node/3136304

4) Patch applies cleanly to 9.1.

5) Test are still running.

6) In order for me to feel comfortable marking RTBC, I'd like to review the assertEmpty vs assertSame usage and see if the test needs to check if it's an empty array or just an empty value. Unfortunately, I'm not sure I can do that today.

As oer the comment on #10 updated the below lines

+++ b/core/modules/views/tests/src/Unit/Plugin/area/ViewTest.php
@@ -62,6 +62,7 @@ public function testCalculateDependencies() {
+    $this->assertIsArray($this->viewHandler->calculateDependencies());
     $this->assertEmpty($this->viewHandler->calculateDependencies());

@nitesh624 When you add updated patches, it's very helpful if you would add a comment why you are making changes. The interdiff is very useful but understanding the motivation for the update is important. Thanks.

Working on it.

I have checked the above patch locally and it's not showing any error for the first two cases, let me re-add a test to verify with the current core version what else we need to fix.

Adding the screenshot of the test output for reference.

Thanks
Rajandro

Adding the updated patch with the test case failure fixing.

-        
+++ b/core/tests/Drupal/Tests/Core/Config/StorageCopyTraitTest.php
@@ -154,7 +154,7 @@
-        $this->assertArrayEquals($source->read($name), $target->read($name));
+        $this->assertEquals($source->read($name), $target->read($name));

PS: Since this fix is a change in the Trait, so if this is not required or out of scope, then we can revert the changes. Please review it.

Thanks
Rajandro

New usages of assertArrayEquals were introduced since the latest patch #18.

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -87,8 +87,14 @@ protected function getRandomGenerator() {
+    @trigger_error(__METHOD__ . " is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing, or ::assertSame instead. See https://www.drupal.org/node/3136304", E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/UnitTestCaseTest.php
@@ -0,0 +1,22 @@
+   * @expectedDeprecation Drupal\Tests\UnitTestCase::assertArrayEquals is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use ::assertEquals, ::assertEqualsCanonicalizing, or ::assertSame instead. See https://www.drupal.org/node/3136304

The method names should end with () to make it clearer.

Addressed #25

Thanks, looks good to me.

+++ b/core/modules/workflows/tests/src/Unit/WorkflowTest.php
@@ -102,21 +102,21 @@ public function testGetStates() {
 
     // An empty array does not load all states.
-    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getStates([])));
+    $this->assertSame([], array_keys($workflow->getTypePlugin()->getStates([])));
   }
 
   /**
@@ -394,8 +394,8 @@ public function testGetTransitions() {

@@ -394,8 +394,8 @@ public function testGetTransitions() {
     $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow');
 
     // Getting transitions works when there are none.
-    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getTransitions()));
-    $this->assertArrayEquals([], array_keys($workflow->getTypePlugin()->getTransitions([])));
+    $this->assertEmpty(array_keys($workflow->getTypePlugin()->getTransitions()));
+    $this->assertEmpty(array_keys($workflow->getTypePlugin()->getTransitions([])));

Why assertSame() for one empty comparison then assertEmpty() for the next.

Also we don't need the array_keys() for these since an empty array has no keys.

Addressed #28

I'd probably go for:

$this->assertSame([], ...);

Working on changes suggested in #31

Made the changes, please review.

Please review.

Made the changes suggested in #37.

Committed 47355e3 and pushed to 9.1.x. Thanks!