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

Add support for checking subsets that contain duplicate values to ArraySubset #3186

Conversation

pfrenssen
Copy link
Contributor

@pfrenssen pfrenssen commented Jun 27, 2018

In #3161 support was added for checking indexed arrays in ArraySubset. The edge case of checking subsets that contain multiple identical values was left out of the scope of that PR. This means that in the current implementation the following example will generate a false positive:

  • Array: [0, 1, 2, 3]
  • Subset: [0, 1, 1, 2]
  • Expected result: FALSE since the value 1 is only present once in the array, but twice in the subset.
  • Actual result: TRUE

This PR adds support for this edge case, so that it will become possible to check subsets with duplicate values.

@codecov-io
Copy link

codecov-io commented Jun 27, 2018

Codecov Report

Merging #3186 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3186      +/-   ##
============================================
+ Coverage     82.37%   82.37%   +<.01%     
- Complexity     3508     3512       +4     
============================================
  Files           140      140              
  Lines          9243     9245       +2     
============================================
+ Hits           7614     7616       +2     
  Misses         1629     1629
Impacted Files Coverage Δ Complexity Δ
src/Framework/Constraint/ArraySubset.php 98.63% <100%> (+0.03%) 37 <0> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 995a578...c0c94e9. Read the comment docs.

@pfrenssen pfrenssen changed the title Test that ArraySubset can handle subsets with duplicate values. Add support for checking subsets that contain duplicate values to ArraySubset Jun 27, 2018
@KristopherWindsor
Copy link

We have a few tests that started failing when phpunit 7.3 was released.
Not good to have a minor phpunit version break tests. 😭 Anyway, we think this PR will make our tests pass again.

07:16:26 There were 4 failures:
07:16:26 
07:16:26 1) CompanyTest\Integration\Lib\Model\Test\ClassNameTest::testHandleFollowUpCacheKeyCallSettingOffAwaySettingOnNoSentReply
07:16:26 Failed asserting that an array has the subset Array &0 (
07:16:26     0 => '30'
07:16:26     1 => '14097220017'
07:16:26     2 => 'A'
07:16:26     3 => 'F'
07:16:26     4 => 'F'
07:16:26     5 => 'N'
07:16:26     6 => 'P'
07:16:26     7 => 'SMS'
07:16:26     8 => 'ProductName'
07:16:26 ).
07:16:26 --- Expected
07:16:26 +++ Actual
07:16:26 @@ @@
07:16:26  Array
07:16:26  (
07:16:26 -    [0] => 30
07:16:26 -    [1] => 14097220017
07:16:26 +    [1] => 30
07:16:26      [2] => A
07:16:26 -    [3] => F
07:16:26 -    [4] => F
07:16:26 -    [5] => N
07:16:26 -    [6] => P
07:16:26 -    [7] => SMS
07:16:26 -    [8] => ProductName
07:16:26 +    [3] => 14097220007
07:16:26 +    [4] => P
07:16:26 +    [5] => F
07:16:26 +    [6] => ProductName
07:16:26 +    [7] => F
07:16:26 +    [8] => N
07:16:26 +    [9] => SMS
07:16:26  )

@pfrenssen
Copy link
Contributor Author

@KristopherWindsor did you try it with this PR? I see you have duplicate values in your subset. That's what this PR is fixing. If it is not, could you provide a test case so I can have a look?

@pfrenssen
Copy link
Contributor Author

In #3240 a regression was reported against assertArraySubset(), but this problem no longer occurs in this PR. I have included the test case for that issue to prove that this is fixed here.

This proves that the bug reported in sebastianbergmann#3240 is also fixed in scope of this PR.
@pfrenssen pfrenssen force-pushed the support-subsets-with-duplicates branch from cbb0f24 to c0c94e9 Compare August 5, 2018 19:58
@RikSomers
Copy link

RikSomers commented Aug 6, 2018

@pfrenssen We use the Laravel framework here at work, and serveral tests stopped working when upgrading to PHPUnit 7.3.0 due to the ArraySubset change.

I can confirm this PR fixes any failing tests that were introduced with the 7.3.0 changes in our companies codebase.

@sebastianbergmann
Copy link
Owner

I have reverted #3161 because of #3240.

@pfrenssen Please send a new pull request against master that contains both #3161 and #3186. I am sorry for having to ask this of you -- and that you have to wait until PHPUnit 7.4 to get these improvements into a stable release of PHPUnit.

@kmbt
Copy link

kmbt commented Jan 4, 2019

Having some background in mathematics and also in python programming language I find this way of thinking really confusing.

In mathematical set theory, an object just may be or may not be a member of a set. In set theory there is no concept of an object being a member "more than once" of a set.

This reasoning is applied in python, where the following expression evaluates as True:

set([[0, 1, 1, 2]) == set([0, 1, 2])

Please kindly take my remarks into your consideration.

@Stadly
Copy link
Contributor

Stadly commented Jan 22, 2019

@pfrenssen Did you have a chance to create a new PR containing both #3161 and #3186? Would you like me to look into it?

@pfrenssen
Copy link
Contributor Author

@Stadly I have this on my to-do list which is admittedly very long :)

I don't think I will get round to this very soon, so feel free to open the PR if you want. I would be glad if you could keep my git history so I can get credit for the work, but if this is not practical for you then don't worry.

Would be nice to see this merged indeed!

@Stadly Stadly mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants