phpunit icon indicating copy to clipboard operation
phpunit copied to clipboard

Allow providing named arguments from a data provider

Open jnoordsij opened this issue 2 years ago • 5 comments

Resolves #5150

This PR aims to allow passing named arguments from a data provider, to have more flexibility in re-ordering arguments and skipping optional ones without having to provide a default value.

Note 1: I was not entirely sure on where to put the test; if there is a more suitable location to add this, let me know!

Note 2: Combining named arguments with input from a dependency is not supported, as providing positional arguments after named ones in an array is not supported, and data provider arguments will go first as per doc description. I could not find a test to ensure this behavior, so made https://github.com/jnoordsij/phpunit/commit/ac028c8eb14da44268378a5a0fe0b9840aa67e2e this PR doesn't break existing behavior. I also created https://github.com/jnoordsij/phpunit/commit/f7e28c4c7a51c1f805d5ec94ae29feaa5acf63f4 to demonstrate the unsupported case. If wanted, I can add those tests to this PR or a new PR.

jnoordsij avatar Feb 20 '23 10:02 jnoordsij

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (855919d) 89.40% compared to head (dafdc71) 89.40%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5225   +/-   ##
=========================================
  Coverage     89.40%   89.40%           
  Complexity     6425     6425           
=========================================
  Files           683      683           
  Lines         20405    20405           
=========================================
  Hits          18243    18243           
  Misses         2162     2162           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 20 '23 11:02 codecov[bot]

Hey there! Was wondering if there was anything left for me to do that can help getting this reviewed and/or merged?

jnoordsij avatar Mar 24 '23 10:03 jnoordsij

Just rebased this onto main. I saw the 10.1 release is coming up; any chance of this making it into that, or is it too close to the release? Or is there any work required before merge?

jnoordsij avatar Apr 13 '23 08:04 jnoordsij

This will not go into PHPUnit 10.1, sorry.

sebastianbergmann avatar Apr 13 '23 08:04 sebastianbergmann

This will not go into PHPUnit 10.1, sorry.

I understand that it was a bit of a short time to get it in. Is there anything I can do to help it go into the next minor version?

jnoordsij avatar May 11 '23 10:05 jnoordsij

Rebased this on main and tests still seem to be fine locally.

Note: for anyone subscribed interested, testing these changes in your local setup to ensure everything works fine on your end and then leaving a review might be helpful in landing this PR.

jnoordsij avatar Oct 06 '23 14:10 jnoordsij

I also created https://github.com/jnoordsij/phpunit/commit/f7e28c4c7a51c1f805d5ec94ae29feaa5acf63f4 to demonstrate the unsupported case. If wanted, I can add those tests to this PR or a new PR.

Yes, please send a pull request for test(s) for the unsupported case(s). Thanks!

sebastianbergmann avatar Jan 13 '24 14:01 sebastianbergmann