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

Allow providing named arguments from a data provider #5225

Conversation

jnoordsij
Copy link
Contributor

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 jnoordsij@ac028c8 this PR doesn't break existing behavior. I also created jnoordsij@f7e28c4 to demonstrate the unsupported case. If wanted, I can add those tests to this PR or a new PR.

@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jnoordsij jnoordsij force-pushed the data-provider-named-arguments branch from d7eeec1 to 800b79a Compare March 2, 2023 11:34
@jnoordsij
Copy link
Contributor Author

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

@jnoordsij
Copy link
Contributor Author

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?

@sebastianbergmann
Copy link
Owner

This will not go into PHPUnit 10.1, sorry.

@jnoordsij
Copy link
Contributor Author

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 jnoordsij force-pushed the data-provider-named-arguments branch from c9a6f74 to 7132f53 Compare June 5, 2023 09:58
@jnoordsij jnoordsij force-pushed the data-provider-named-arguments branch from 4f4fb45 to d4a9a6e Compare July 26, 2023 10:07
@jnoordsij jnoordsij force-pushed the data-provider-named-arguments branch from d4a9a6e to b926904 Compare August 18, 2023 16:32
@jnoordsij jnoordsij force-pushed the data-provider-named-arguments branch from b926904 to 0f81bc5 Compare October 6, 2023 13:59
@jnoordsij
Copy link
Contributor Author

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.

@sebastianbergmann sebastianbergmann added type/bug Something is broken feature/test-runner CLI test runner type/enhancement A new idea that should be implemented and removed type/bug Something is broken labels Jan 13, 2024
@sebastianbergmann sebastianbergmann merged commit 3878796 into sebastianbergmann:main Jan 13, 2024
25 checks passed
@sebastianbergmann
Copy link
Owner

I also created jnoordsij@f7e28c4 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow providing named arguments from a data provider
2 participants