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

Fix ignored test with duplicate key in dataprovider #3444

Closed

Conversation

@jderusse
Copy link
Contributor

@jderusse jderusse commented Dec 10, 2018

With generators dataProvider may have duplicate keys.

    public static function dataProvider()
    {
        yield 'foo' => ['foo'];
        yield 'foo' => ['bar'];
    }

This PR throws an exception when the dataprovider contains duplicate keys.

@jderusse jderusse force-pushed the fix-duplicate-key branch from 1074f9b to e885a77 Dec 10, 2018
@codecov
Copy link

@codecov codecov bot commented Dec 10, 2018

Codecov Report

Merging #3444 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3444   +/-   ##
=========================================
  Coverage     82.15%   82.15%           
- Complexity     3579     3580    +1     
=========================================
  Files           143      143           
  Lines          9395     9395           
=========================================
  Hits           7718     7718           
  Misses         1677     1677
Impacted Files Coverage Δ Complexity Δ
src/Util/Test.php 92.67% <100%> (ø) 207 <0> (+1) ⬆️

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 c1b8b32...e885a77. Read the comment docs.

Loading

@codecov
Copy link

@codecov codecov bot commented Dec 10, 2018

Codecov Report

Merging #3444 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3444      +/-   ##
============================================
+ Coverage     82.15%   82.16%   +0.01%     
- Complexity     3580     3581       +1     
============================================
  Files           143      143              
  Lines          9398     9404       +6     
============================================
+ Hits           7721     7727       +6     
  Misses         1677     1677
Impacted Files Coverage Δ Complexity Δ
src/Util/Test.php 92.76% <100%> (+0.09%) 207 <0> (+1) ⬆️

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 a6d9814...6ab71b2. Read the comment docs.

Loading

tests/unit/Util/TestTest.php Outdated Show resolved Hide resolved
Loading
@fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 10, 2018

IMHO, it should throw an exception, as this is certainly a mistake and it is currently not working.

Loading

@epdenouden
Copy link
Collaborator

@epdenouden epdenouden commented Dec 10, 2018

@jderusse if you want to fix the build, cherry-pick d607b3e :)

Loading

@jderusse jderusse force-pushed the fix-duplicate-key branch 2 times, most recently from 0654c1d to b43d0a2 Dec 13, 2018
@jderusse
Copy link
Contributor Author

@jderusse jderusse commented Dec 13, 2018

reverted to exception instead of fallback

Loading

@fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 22, 2018

Some people in this PHPStan issue think having the same key twice in a Generator is perfectly valid and that therefore, it's a bug in PHPUnit if it's not working properly.

Loading

@Majkl578
Copy link

@Majkl578 Majkl578 commented Dec 22, 2018

It's indeed a PHPUnit bug that it silently drops provided entries with duplicate names.
More that it also happens if you produce same name from multiple data providers for one test, this needs to be tested and covered too.

Loading

@Majkl578
Copy link

@Majkl578 Majkl578 commented Dec 22, 2018

Also I think expeption is not the solution, at least when duplicate keys come from different data providers. It should be fixed (i.e. by constraining origin data provider), not prevented.

Loading

@jderusse
Copy link
Contributor Author

@jderusse jderusse commented Dec 22, 2018

The key in the dataprovider is used to help the developer to find quickly which datset is responsible of a test failure.

If you have 2 dataproviders with the same Key (or the same key repeated twice in a dataprovider) How would that key help you to find the responsible of the failure?

Each time I had one, it was a typo or a copy/paste from an other rule. Do you have a valid usecase?

I'm agree with the phpstan thread, that duplicate key are valid in PHP. But in the case of phpunit dataprovider, it don't make sense and should be reported to the developer.

Loading

@jderusse jderusse force-pushed the fix-duplicate-key branch 2 times, most recently from d7c3499 to 887793b Dec 22, 2018
@Majkl578
Copy link

@Majkl578 Majkl578 commented Dec 22, 2018

@jderusse You can show both data provider name and key.

Loading

@jderusse
Copy link
Contributor Author

@jderusse jderusse commented Dec 23, 2018

@Majkl578 Concatening the dataprovider with the key in order to, both, give extra information and allowing duplicate keys accross dataproviders is not the purpose of that PR (warn the developer about silently ignored tests), and could be addressed in a dedicated PR as it won't fixes duplicate keys in the same dataprovider.

Loading

@jderusse jderusse force-pushed the fix-duplicate-key branch from 887793b to 6ab71b2 Dec 23, 2018
@Majkl578
Copy link

@Majkl578 Majkl578 commented Dec 23, 2018

I only answered your own question.

Loading

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Dec 27, 2018

Merged manually, thanks.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants