Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assertArraySubset() bad documented or doesn't work as expected #2069

Closed
kgkg opened this Issue Feb 9, 2016 · 13 comments

Comments

Projects
None yet
9 participants
@kgkg
Copy link

kgkg commented Feb 9, 2016

Hi,

please consider 3 calls:

  1. $this->assertArraySubset( ['a', 'b', 'c'], ['a', 'b', 'c', 'd'] );
  2. $this->assertArraySubset( ['b', 'c'], ['a', 'b', 'c', 'd'] );
  3. $this->assertArraySubset( ['a', 'c', 'b'], ['a', 'b', 'c', 'd'] );

1 - works
2 - doesn't work. Why? Isn't ['b','c'] a subset of ['a', 'b', 'c', 'd']?
3 - doesn't work as well. Elements must be in apriopriate order?

Should it work this way?
Can information about this be added to docs? (3 little examples above should do)?
It's sort of confusing right now :)

@GrahamCampbell

This comment has been minimized.

Copy link
Contributor

GrahamCampbell commented Feb 9, 2016

Isn't ['b','c'] a subset of ['a', 'b', 'c', 'd']?

[1 => 'b', 2 => 'c'] is though

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Feb 22, 2016

This is a question for @marcioAlmada who implemented this feature.

@kgkg

This comment has been minimized.

Copy link
Author

kgkg commented Feb 23, 2016

It should be considered that some people are probably depending in their tests on this behaviour, so maybe it's not wise to change it after all. Maybe it just should be a little better documented.
Maybe function working like I described should be added to phpunit as another assert function?

@jboffel

This comment has been minimized.

Copy link

jboffel commented Nov 14, 2016

Hi, I guess this function should have a flag to not consider index position.

For explicit associative array where you name the key this won't be a problem. However for dynamic index array this is indeed a problem to consider the order.

  1. $this->assertArraySubset(['World!' => 'b'], ['Hello' => 'a', 'World!' => 'b']);
    
  2. $this->assertArraySubset(['Hello' => 'a'], ['Hello' => 'a', 'World!' => 'b']);
    
  3. $this->assertArraySubset(['World!' => 'b', 'Hello' => 'a'], ['Hello' => 'a', 'World!' => 'b']);
    
  4. $this->assertArraySubset(['Hello' => 'a', 'World!' => 'b'], ['Hello' => 'a', 'World!' => 'b']);
    

1 to 4 will pass successfully regardless of the order as expected with this assertion function name.

But when it comes to dynamically assigned index

  1. $this->assertArraySubset( ['a', 'b', 'c'], ['a', 'b', 'c', 'd'] );
    
  2. $this->assertArraySubset( ['b', 'c'], ['a', 'b', 'c', 'd'] );
    
  3. $this->assertArraySubset( ['a', 'c', 'b'], ['a', 'b', 'c', 'd'] );
    

As said above:

1 - works
2 - doesn't work.
3 - doesn't work.

A quick fix is to loop over the expected array subset for each key and run assertContains to the actual array to dismiss the index criteria. However you'll have issue with mixed array, partially explicit key definition and dynamic attribution.

So, either you restrict usage of this function to the only array with explicit associate key, or this function should have a flag to mention "This is an associative array" or "This is a non associative array". The mixed case will remain unsupported though.

@marcioAlmada

This comment has been minimized.

Copy link
Contributor

marcioAlmada commented Nov 14, 2016

Hi!

Somehow I missed the notifications on this thread.

Indeed this assertion was only discussed in the context of "associative arrays". I believe that for a pair of non associative arrays the assertion should be represented by an intersection followed by a sorted comparison:

diff --git a/src/Framework/Constraint/ArraySubset.php b/src/Framework/Constraint/ArraySubset.php
index c3cb095..d3569b6 100644
--- a/src/Framework/Constraint/ArraySubset.php
+++ b/src/Framework/Constraint/ArraySubset.php
@@ -49,12 +49,26 @@ public function __construct($subset, $strict = false)
      */
     protected function matches($other)
     {
-        $patched = array_replace_recursive($other, $this->subset);
+        if (! $this->isArrayAssociative($this->subset)) {
+            $patched = array_intersect($other, $this->subset);

-        if ($this->strict) {
-            return $other === $patched;
-        } else {
-            return $other == $patched;
+            sort($patched);
+            sort($this->subset);
+
+            if ($this->strict) {
+                return $this->subset === $patched;
+            } else {
+                return $this->subset == $patched;
+            }
+        } else {          
+
+            $patched = array_replace_recursive($other, $this->subset);
+
+            if ($this->strict) {
+                return $other === $patched;
+            } else {
+                return $other == $patched;
+            }
         }
     }

@@ -82,4 +96,9 @@ protected function failureDescription($other)
     {
         return 'an array ' . $this->toString();
     }
+
+    private function isArrayAssociative(array $subject)
+    {
+        return count(array_filter(array_keys($subject), 'is_string')) > 0;
+    }
 }
diff --git a/tests/Framework/AssertTest.php b/tests/Framework/AssertTest.php
index 984346c..b63fe2d 100644
--- a/tests/Framework/AssertTest.php
+++ b/tests/Framework/AssertTest.php
@@ -182,6 +182,10 @@ public function testAssertArraySubset()
         $this->assertArraySubset(['a' => 'item a', 'c' => ['a2' => 'item a2']], $array);
         $this->assertArraySubset(['a' => 'item a', 'd' => ['a2' => ['b3' => 'item b3']]], $array);

+        $this->assertArraySubset(['a', 'b', 'c'], ['a', 'b', 'c', 'd']);
+        $this->assertArraySubset(['b', 'c'], ['a', 'b', 'c', 'd']);
+        $this->assertArraySubset(['b', 'a'], ['a', 'b', 'c', 'd' => 3, 'x']);
+
         try {
             $this->assertArraySubset(['a' => 'bad value'], $array);
         } catch (PHPUnit_Framework_AssertionFailedError $e) {

This patch could be a starting point, I'll be in transit and wasn't able to PR ✈️ but I'll be back ASAP. It would be good to have a discussion/RFC on any the other expected behaviors before proceeding.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 14, 2016

Thank you for looking into this, @marcioAlmada.

@thiagoaos

This comment has been minimized.

Copy link

thiagoaos commented Jan 31, 2017

Will this patch be applyed in 5.7 version?

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Feb 1, 2017

@marcioAlmada Is #2069 (comment) a bugfix? Can you send a pull request?

@jblotus

This comment has been minimized.

Copy link

jblotus commented Aug 8, 2017

Came across this issue today. Will the patch be implemented at some point @marcioAlmada ?

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Oct 14, 2017

I am still confused about assertArraySubset(). Could this already be fixed by #2237? In case it's not:

@marcioAlmada Is #2069 (comment) backward compatible? Can you please send a pull request with that patch? Thanks!

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 27, 2017

No feedback, closing.

@askovron

This comment has been minimized.

Copy link

askovron commented Feb 2, 2018

Please see comment on #2781:

@eithed

This comment has been minimized.

Copy link

eithed commented Jan 15, 2019

Can you please specify in the documentation that assertArraySubset works only for associative arrays? Given the tickets referencing this and commits/reverts it was hard to track the history of this and what is current behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.