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

Variadic support for #[TestWith] attribute #5163

Closed
davidbyoung opened this issue Feb 3, 2023 · 3 comments
Closed

Variadic support for #[TestWith] attribute #5163

davidbyoung opened this issue Feb 3, 2023 · 3 comments
Labels
feature/data-provider Data Providers feature/metadata/attributes feature/metadata Issues related to attributes and annotations type/enhancement A new idea that should be implemented

Comments

@davidbyoung
Copy link
Sponsor

davidbyoung commented Feb 3, 2023

Loving PHPUnit 10, thank you so much for it! I've switched a lot of my @dataProvider annotations to use the #[TestWith] attribute, but I have found the fact that it takes an array of parameters rather than variadic parameters to be awkward. For example, I think this is cleaner:

#[TestWith(20, 21, false)]
#[TestWith(22, 21, true)]
public function testAgeRequirement(int $age, int $minimumAge, bool $expectedToPass): void
{
    $this->assertSame($expectedToPass, $age >= $minimumAge);
}

...than this:

#[TestWith([20, 21, false])]
#[TestWith([22, 21, true])]
public function testAgeRequirement(int $age, int $minimumAge, bool $expectedToPass): void
{
    $this->assertSame($expectedToPass, $age >= $minimumAge);
}

...because the former almost looks like an invocation of the test method, which to me feels intuitive. I feel the latter looks like you're passing in array as a parameter. Since TestWith::__construct() does not take in any other parameters, I think it would be a better developer experience to take in a variadic parameter instead. Obviously, this would be a breaking change, which might mean we should use a new attribute instead. Using a variadic parameter would also complicate supporting a feature such as named data sets. I'm curious what your thoughts are on this and whether we should support variadic parameters or introduce a new attribute to prevent the breaking change. If so, I'll contribute a pull request.

I should note that NUnit and xUnit both use the variadic parameter approach.

@davidbyoung davidbyoung added the type/enhancement A new idea that should be implemented label Feb 3, 2023
@sebastianbergmann sebastianbergmann added feature/data-provider Data Providers feature/metadata Issues related to attributes and annotations feature/metadata/attributes labels Feb 4, 2023
@sebastianbergmann
Copy link
Owner

I would consider a PR for this, but only if introduces a new/separate attribute for this and supports named data sets (first parameter of type string for the name followed by ...mixed will likely work). Right now I do not have a good name for the new attribute, though.

@sebastianbergmann sebastianbergmann added version/10 Something affects PHPUnit 10 and removed version/10 Something affects PHPUnit 10 labels Feb 4, 2023
@davidbyoung
Copy link
Sponsor Author

Sounds good. I'll start a PR. Again, thanks for 10.0!

@davidbyoung
Copy link
Sponsor Author

davidbyoung commented Feb 4, 2023

One thing - I noticed that PHUnit typically uses the @no-named-arguments Psalm annotation. Are you open to using a named argument with the new attribute, eg:

#[TestData('foo', 'bar', name: 'Test with foo and bar')]
public function testSomething(string $a, string $b): void
{
    // ... 
}

Is this a non-starter and I should find another way, or should I continue with such an approach? I think it would be more declarative and reduce unintended side effects of always interpreting the first string parameter as a name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/data-provider Data Providers feature/metadata/attributes feature/metadata Issues related to attributes and annotations type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

2 participants