drupal_array_merge_deep_array() and its sibling drupal_array_merge_deep() currently have no unit tests, which doesn't help with making them popular.

Writing an actual unit test case for drupal_array_merge_deep_array() actually made me wonder whether the result was really the expected one.

CommentFileSizeAuthor
#13 interdiff.txt8.52 KBtim.plunkett
#13 nested-array-test-1696172-13.patch9.24 KBtim.plunkett
#11 0001-Issue-1696172-add-complex-merge-tests-for-deep-array.patch4.56 KBfgm
#10 0001-Issue-1696172-add-complex-merge-tests-for-deep-array.patch4.92 KBfgm
#9 0001-Issue-1696172-add-complex-merge-tests-for-deep-array.patch3.02 KBfgm
#3 0001-Issue-1696172-add-unit-tests-for-NestedArray-mergeDe.patch3.54 KBfgm
#2 0001-Issue-1696172-Validate-drupal_array_merge_deep_array.patch3.51 KBfgm
#1 0001-Add-unit-tests-for-drupal_array_merge_deep_array-to-.patch2.06 KBfgm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Active » Needs review
FileSize
0001-Add-unit-tests-for-drupal_array_merge_deep_array-to-.patch2.06 KB

Attached patch includes such a test.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
0001-Issue-1696172-Validate-drupal_array_merge_deep_array.patch3.51 KB

Rerolled on today's HEAD and improved after discussion with yched.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
0001-Issue-1696172-add-unit-tests-for-NestedArray-mergeDe.patch3.54 KB

And another version: since that function is now just a wrapper around NestedArray::mergeDeepArray(), test the method call instead of the procedural wrapper.

Sachini’s picture

Sachini CreditAttribution: Sachini commented
Issue summary: View changes

patch applied cleanly for 8.x

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/DeepMergeUnitTest.php
@@ -0,0 +1,112 @@
+class DeepMergeUnitTest extends UnitTestBase {

This isn't a phpunit unit test but a simpletest unit test. Note: \Drupal\Tests\Component\Utility\NestedArrayTest. seem to cover that function already. Maybe you have some more cases than that file?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Closed (won't fix)

So yeah this is probably a won't fix, unless someone reopens it.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Closed (won't fix) » Needs work

@dawehner : at the time I posted these tests, the NestedArrayTest didn't exist: Alex created it one almost one year later :-)

Nonetheless, the current NestedArrayTest does not check some of the scenarios included in this one, so I think they are still needed. However, maybe I should reroll them on PHPunit instead of Simpletest unit tests ?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

You totally should.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Title: Validate drupal_array_merge_deep_array() behavior with unit tests. » Add unit tests for complex deep array merge cases
Status: Needs work » Needs review
FileSize
0001-Issue-1696172-add-complex-merge-tests-for-deep-array.patch3.02 KB

And here it is: there was only one test for deep merges, and it covers only simpler non-conflicting merges.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
0001-Issue-1696172-add-complex-merge-tests-for-deep-array.patch4.92 KB

Rerolled with @covers / @coversDefaultClass annotations.

With this additional tests, not only do we have 100% S0 coverage on the class, but also (I think) full predicate coverage on mergeDeepArray().

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
0001-Issue-1696172-add-complex-merge-tests-for-deep-array.patch4.56 KB

As per @dewehner 's suggestion, using short method names on the @covers.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you for that last bit.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
nested-array-test-1696172-13.patch9.24 KB
interdiff.txt8.52 KB

You actually need the :: with @covers or it does not work. Running a coverage test without it will fail the whole test.

I also switched assertEquals to assertSame since the latter uses strict comparisons.
And unlike simpletest, the assertTrue/assertFalse also use strict comparisons.

Also unlike simpletest, assertSame specifies the params as assertSame($expected, $actual, $message = ''), so I switched the order on some of the assertions that were backwards. This is useful when running tests in PHPStorm for example, because it will show you a comparison between expected and actual

The rest was just fixing doxygen stuff.

EDIT: Ah! I didn't read the actual patch past the missing ::, and I just fixed the file after applying the patch locally. So I touched a bit more than the previous patches did, and many of the things I mentioned above were pre-existing, my apologies :(

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: nested-array-test-1696172-13.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

13: nested-array-test-1696172-13.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community

Random fail.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 2826a61 and pushed to 8.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Component/Utility/NestedArrayTest.php b/core/tests/Drupal/Tests/Component/Utility/NestedArrayTest.php
index 49dc874..08c9f78 100644
--- a/core/tests/Drupal/Tests/Component/Utility/NestedArrayTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/NestedArrayTest.php
@@ -90,7 +90,7 @@ public function testGetValue() {
   }
 
   /**
-   * Tests setting nested array values:
+   * Tests setting nested array values.
    *
    * @covers ::setValue
    */

Fixed during commit.

  • Commit 2826a61 on 8.x by alexpott: 
    Issue #1696172 by fgm, tim.plunkett: Add unit tests for complex deep...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.