Define a standard for documenting data providers in PHPUnit-based tests

👏

It'd be great if we could exempt PHPUnit data providers from that rule (of having to document every parameter/return value). I'm not sure if that's possible though. The exemption would look like this:

Any class that extends \PHPUnit\Framework\TestCase should no docblock for any method whose name has the prefix provider.

That'd remove a nice chunk of annoying boilerplate!

Any class that extends \PHPUnit\Framework\TestCase should no [sic] docblock for any method whose name has the prefix provider.

Do we actually specify anywhere that data provider methods should be prefixed with provider? Otherwise, we should probably add that, too.
Also, I'd just allow omitting the doc block, not require it to be omitted.
Also, having a doc block but with just an @see tag linking to the test method could be handy – especially for longer test methods or in case one data provider method is used for multiple test methods. In that case, we could also allow having a doc block without a description or a @return tag.

Thinking about this a bit more, though, I think I'm against this proposal. Not strongly, but I think I prefer to be consistent here and also have proper doc blocks for methods that don't really need them. Otherwise, PhpStorm will mark all of those as warnings (for me), which is always annoying, and it will also look kind of weird, "naked" – a method without a doc block. Also, it's just like half a minute to write such a doc block, so it's not like we'd save a lot of time. A little space, though, that's true.

However, if we mandate that every set of arguments is keyed by a meaningful string then this is super useful.

Yes, that definitely, +1 from me! I at first didn't know this was even possible, but it's really very helpful to have and should be best practice. If we add coding standards for data providers, this should definitely be included.

In PHPUnit 11 we'll need to use the #[DataProvider] attribute. Not sure we need to enforce any particular naming convention if we have this.

EDIT: That attribute goes on the test method, not the provider, so maybe it is in fact better to have some convention for the provider method.

In #3455550: Rename mis-named data providers discovered in #3421418, we also noticed a few data providers that were named testSomethingSomethingProvider(), rather than the more common providerTestSomethingSomething(). It might be worth having an explicit standard about the naming, especially since the pattern of those three methods made it look like they were test methods themselves rather than data providers. (I don't know if PHPUnit actually tries to run them, but it is confusing.) Added to the IS.

Adding the new template

As I mentioned on the other issue, I also am not sure if lumping naming with documentation expectations is correct. It should possibly be separate issues.

The issue title "Discuss the standards for data providers in PHPUnit-based tests" to me suggests a holistic approach. Happy for someone else to split it in to multiple tickets, but it makes sense to me to discuss it as a whole in this issue and split off once concrete decisions are made.

Moved the naming work to #3456134: Define a standard for naming data providers in PHPUnit-based tests

> However, if we mandate that every set of arguments is keyed by a meaningful string then this is super useful.

That is super useful, but it's code, not documentation. Out of scope for this issue?

> If we follow the core standards of documenting @return and @param tests will have a lot of boilerplate that does not add much.

Documenting both the @return of the provider and the @params of the tests is writing the same thing twice, and redundant, but *one* of them should be documented!

> However, if we mandate that every set of arguments is keyed by a meaningful string then this is super useful.

That is super useful, but it's code, not documentation. Out of scope for this issue?

I think this is a very useful thing to do, without it being the same as #2108785: Remove the requirement for doxygen for test methods, can we use this issue for adding that requirement?

Sorry, @borisson. I am not sure what you mean in the last paragraph. Can we use this issue for adding what requirement?

The existing standard and the new one proposed in #2108785: Remove the requirement for doxygen for test methods would include data providers and thus, they would need to be documented. So, that seems fine to me and no change is needed. No, there is the possibility to recommend that the return array uses meaningful string keys. That could be documented at Drupal Test coding standards, which is not part of the Drupal Coding Standards. They seem more akin to best practice.

I think having string keys for data providers seem like a good thing to have, but thinking about it some more, it does seem like it would not be easy to do as a coding standard. I am not sure we can always require this. So documenting it in the Test coding standards seems like a good path forward.

For this issue, I am a bit unsure what the overlap is with #2108785: Remove the requirement for doxygen for test methods, since that already takes care of the requirements for documentation. I think that means we should close this issue?