Assert array partials

[marcioAlmada]

Hi 😄

I've got into some situations involving partial assertion of arrays that could be easily solved if we had the following implementation:

$data = [
    'a' => 'item a',
    'b' => 'item b',
    'c' => 'item c',
    'd' => 'item d'
];

// passes
$this->assertArrayPart(['c' => 'item c', 'a' => 'item a'], $data); 

// fails
$this->assertArrayPart(['c' => 'item c', 'e' => 'item e'], $data); 
$this->assertArrayPart(['c' => 'wrong value'], $data); 

It could also assert recursions:

$data = [
    'a' => 'item a',
    'b' => 'item b',
    'c' => ['a2' => 'item a2', 'b2' => 'item b2']
];

$needle = [
    'a' => 'item a',
    'c' => ['a2' => 'item a2']
];

$this->assertArrayPart($needle, $data);  // passes

I searched through all the docs and found nothing equivalent.

[sun]

In esssence, you mean array_intersect_assoc()?

Doesn't assertContains() do this already?

[marcioAlmada]

Doesn't assertContains() do this already?

That was my first assumption but... nope. Assert contains will look for an array item that corresponds exactly to the needle itself, and I believe this should be the expected behavior :D

The new assertion should detect if the needle represents a [key=>value] fragment from the subject array.

[marcioAlmada]

@whatthejeff @sebastianbergmann do you think it's worth a PR? I did a quick experiment on a local branch and the assertion is apparently ready to rumble :)

[sun]

Ah, almost guessed so. My only note would be to name it assertArrayIntersectAssoc(). It's tough to name something like this "nicely" without forcing everyone to have to study the manual first, so probably best to stick with the PHP core function name, which everyone understands immediately and can be remembered.

[marcioAlmada]

@sun it's not the same behavior as array_intersect_assoc, that function throws warnings if you try to intersect a nested array with a nested string value (it needs arrays with compatible schemes), that would be misleading. Maybe assertArrayFragment? (in case it's accepted).

[whatthejeff]

I rejected a similar PR (#361) during a triage some time ago.

There was a time when the majority of the PRs we received were just random new assertions of dubious usefulness. We probably merged more of them than we should've. The result is that we have a number of assertions that are rarely (if ever) used. It's sort of a slippery slope, I guess. To combat this, I started rejecting the majority of these PRs unless they were heavily requested.

Let me run this by @sebastianbergmann and see what he thinks. Just for reference, what is your current use case for this?

[sun]

FWIW, I also tinkered about that problem space, since I ran into the same question as a maintainer of Drupal's homegrown testing framework. Over time, I concluded the following as the most pragmatic and most sensible approach:

1. Every PHP core function that starts with is_*() MUST be available as an assertion method. (e.g., assertTrue, assertFalse, assertNull, etc.)

2. Every possible PHP code operation for primitive data types SHOULD be available as an assertion method. (e.g., assertSame [===], assertEquals [==], assertContains [strpos/strstr], assertStartsWith [strpos], etc.)

3. Almost every PHP code operation on arrays/collections/structs/younameit SHOULD be available as an assertion method. (e.g., assertCount [count], assert[Not]Contains [array_search], assert[Not]Empty [empty], assertArray[Not]HasKey [isset/array_key_exists], etc.)

   To minimize confusion + manual lookups, all of these methods should be named after their PHP core function equivalents.

   Advanced operations like array_diff_assoc also fall into this bucket (e.g., like this proposal). — However, in order to make sense for test authors, it's of utmost importance to stay rigid with regard to method naming: It's either the equivalent of a PHP core function (lowerCamelCased), or the assertion tool most likely should be merged into an existing, "human"-labeled assertion method.

4. Only when it gets to objects, I'd be cautious, because most often not sensible.

In short, it's primarily about naming, less about purpose or use-cases. It doesn't hurt anyone if there are well-named + well-designed assertion methods, for which you didn't have a use-case (yet).

A good testing framework works intuitively:

Oh, hm. — I need to assert that this array contains exactly these elements, including keys. How would I assert that in plain PHP?
→ array_intersect_assoc($expected, $actual) === $expected;
→ assertArrayIntersectAssoc($expected, $actual);
…Let me immediately try that before looking up the manual…

(…seriously hope this example works; didn't run it through PHP before posting 😛)

As @marcioAlmada pointed out, it's perfectly OK if the assertion method supports a more intelligent/smart behavior on the given data that the corresponding PHP core function. Only if it's not possible to design it in way that doesn't break regular expectations, additional method suffixes should be used (in this case, e.g., assertArrayIntersectAssocRecursive()).

[whatthejeff]

It doesn't hurt anyone if there are well-named + well-designed assertion methods, for which you didn't have a use-case (yet).

I disagree with this sentiment for just about any software, honestly.

[marcioAlmada]

@whatthejeff recently I found a use case right inside a PHPUnit test case while working on #1358. I only needed to test the existence of a given key => value pair inside a larger configuration array but was obligated to assert the entire array scheme :( Link https://github.com/sebastianbergmann/phpunit/blob/master/tests/Util/TestTest.php#L126-L139

With this new assertion, tests would look much more simpler and objective:

$this->assertArrayPart(
  array('message_regex' => '#regex#'),
  PHPUnit_Util_Test::getExpectedException('ExceptionTest', 'testWithRegexMessage')
);

$this->assertArrayPart(
  array('message_regex' => '#regex#'),
  PHPUnit_Util_Test::getExpectedException('ExceptionTest', 'testWithRegexMessageFromClassConstant')
);

$this->assertArrayPart(
  array('message_regex' => 'ExceptionTest::UNKNOWN_MESSAGE_REGEX_CONSTANT'),
  PHPUnit_Util_Test::getExpectedException('ExceptionTest', 'testWithUnknowRegexMessageFromClassConstant')
);

It could be much easier to split these kind of tests - https://github.com/sebastianbergmann/phpunit/blob/master/tests/Util/TestTest.php#L69-L151 - into smaller assertions, since each part of those configuration arrays are pertinent to a specific feature and therefore should be tested separately, not like a matrix of configurations.

PS: I'm using assertArrayPart on the example because this code was already on my local branch before I started this issue :)

[marcioAlmada]

array_intersect_assoc($expected, $actual) === $expected;
assertArrayIntersectAssoc($expected, $actual);
…Let me immediately try that before looking up the manual… …seriously hope this example works; didn't run it through PHP before posting 😛

@sun As said before, array_intersect_assoc will throw warnings if the needle array has more keys than the subject array or has a different scheme. The concept of your idea is perfect (and it looks really close to what I did :) I implemented the constraint this way:

$actual === array_replace_recursive($actual, $expected);

[whatthejeff]

Thanks for the use case, @marcioAlmada. I'll run this by @sebastianbergmann and see what he thinks. Might also be nice to have assertions like assertArrayKey*() that work like the assertAttribute*() assertions. For example:

$this->assertArrayKeyEquals(
  '#regex#', 'message_regex', 
  PHPUnit_Util_Test::getExpectedException('ExceptionTest', 'testWithRegexMessage')
);

Then we can take advantage of the comparator subframework which lets us support deltas, canonicalization, etc.

[marcioAlmada]

@whatthejeff assertArrayKeyEquals sounds simple and effective when you need to assert a single key => value pair. But it wouldn't let you match a partial [key => value] scheme all at once like demonstrated on first comment ;)

[whatthejeff]

@marcioAlmada Yeah, I know. I was suggesting that it might be nice to have both :)

[whatthejeff]

Alright, I pinged @sebastianbergmann. Let's see what he thinks.

[marcioAlmada]

@whatthejeff sorry, I misread your post :) thank you!

[marcioAlmada]

I've just made a PR just in case I'm not able to respond promptly during the next 3 days ;)

[whatthejeff]

Nice! I haven't heard back from @sebastianbergmann yet. I'll be sure to follow up once I do.

[sebastianbergmann]

#1371 (comment) makes sense to me. Go ahead :-)

[whatthejeff]

Thanks for the PR, @marcioAlmada. I've left some initial feedback. I'm not sure where I stand on the naming discussions. On one hand, I agree with @sun that mapping it to core functions makes life easier. On the other hand, I would prefer typing assertIsSubset or assertArrayPart over assertArrayIntersectAssocRecursive any day :).

@sebastianbergmann do you have any feedback on the name?

[marcioAlmada]

Hi, thanks @sebastianbergmann and @sun for all the feedback

Despite I like @sun rhetoric (always map assertions to core functions), assertArrayIntersectAssocRecursive looks like a bad idea for some very punctual reasons:

1. Some PHP core functions have bad names that should not be used as a model, otherwise we would have ::assertStrpos(), ::assertStrrev(), etc.
2. IMMO "complex" assertions should have their names decoupled from how they are implemented internally. Current implementation uses array_replace_recursive, but there are other ways to implement the same thing so why map to a core PHP function?
3. At least in my mental model, the name assertArrayIntersectAssocRecursive implies that there might be an assertArrayIntersectAssoc (non recursive) assertion.

@whatthejeff I like assertSubset or assertArraySubset. Both have their immediate meaning revealed and they look as good as assertArrayPart, maybe better :)

[whatthejeff]

Discussed this with @sebastianbergmann. Let's go with assertSubset for the name.

[marcioAlmada]

ok, PR seems to be all set now :)

[sun]

Shouldn't this be assertArraySubset()? It's about arrays only, no?

[whatthejeff]

@sebastianbergmann said he liked assertSubset. I can double check with him, though.

[whatthejeff]

Alright, let's do assertArraySubset(). @marcioAlmada would you mind making that one last change?

[marcioAlmada]

No problem. Search and replace to the rescue :)

[whatthejeff]

Thanks, @marcioAlmada. I'll look into the hhvm-nightly segfault.

[whatthejeff]

Alright, the hhvm-nightly crash is unrelated to your changes. Seems they have a regression in DOMDocument::xinclude.

[whatthejeff]

Alright, the segfault should be fixed in master. Would you mind rebasing so that we can make sure everything passes?

[marcioAlmada]

You mean to git pull --rebase https://github.com/sebastianbergmann/phpunit.git master?

[whatthejeff]

@marcioAlmada Well, however your prefer to bring your branch up to date works for me :)

[whatthejeff]

\o/ Yay, all is green! I think we're good to go :)

[marcioAlmada]

Yeah! Ok, done. Build went fine, thanks to you. No segfaults :)

Do you know why hhvm-nightly buid is so slow? It took 3.79 minutes oO

[whatthejeff]

Do you know why hhvm-nightly buid is so slow? It took 3.79 minutes oO

I think it's probably related to facebook/hhvm@842d6fb, but I haven't confirmed it just yet.

[whatthejeff]

@marcioAlmada Also, HHVM is generally going to be slower in this type of situation thanks to the JIT. We might want to disable it actually.

[marcioAlmada]

Yes, you're right. But hhvm-nightly builds got much slower lately. Thanks for pointing facebook/hhvm@842d6fb out.

Cheers!

[whatthejeff]

Yeah, there's definitely an issue. I'll keep looking into it.

[sebastianbergmann]

Looks like this new assertion is not documented yet: sebastianbergmann/phpunit-documentation#217

Can you provide a patch, @marcioAlmada?

[marcioAlmada]

@sebastianbergmann, If I remember right, @whatthejeff already took care of the docs :)

[sebastianbergmann]

sebastianbergmann/phpunit-documentation#217 is still open and I don't see documentation.

[marcioAlmada]

Oh, that's true. I thought docs were already in place. I'll send a patch this weekend ;)