Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
21 Jul 2012 at 17:33 UTC
Updated:
29 Jul 2014 at 20:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
fgmAttached patch includes such a test.
Comment #2
fgmRerolled on today's HEAD and improved after discussion with yched.
Comment #3
fgmAnd another version: since that function is now just a wrapper around NestedArray::mergeDeepArray(), test the method call instead of the procedural wrapper.
Comment #4
Sachini commentedpatch applied cleanly for 8.x
Comment #5
dawehnerThis 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?
Comment #6
dawehnerSo yeah this is probably a won't fix, unless someone reopens it.
Comment #7
fgm@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 ?
Comment #8
dawehnerYou totally should.
Comment #9
fgmAnd here it is: there was only one test for deep merges, and it covers only simpler non-conflicting merges.
Comment #10
fgmRerolled 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().
Comment #11
fgmAs per @dewehner 's suggestion, using short method names on the @covers.
Comment #12
dawehnerThank you for that last bit.
Comment #13
tim.plunkettYou 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 actualThe 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 :(
Comment #15
tim.plunkett13: nested-array-test-1696172-13.patch queued for re-testing.
Comment #16
tim.plunkettRandom fail.
Comment #17
alexpottCommitted 2826a61 and pushed to 8.x. Thanks!
Fixed during commit.