Problem/Motivation

Part of #2057905: [policy, no patch] Discuss the standards for phpunit based tests

Problem/Motivation

We also do not have any restrictions on how they are named, but a best practice of naming single-use providers with this pattern has emerged:

  • For test method testWhateverThing(),
  • the data provider is often called providerTestWhateverThing().

Proposed resolution

tbd

Remaining tasks

Come up with an enforceable standard

Benefits

If we adopted this change, the Drupal Project would benefit by ...

Three supporters required

  1. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)
  2. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)
  3. https://www.drupal.org/u/{userid} (yyyy-mm-dd they added support)

Proposed changes

Provide all proposed changes to the Drupal Coding standards. Give a link to each section that will be changed, and show the current text and proposed text as in the following layout:

1. {link to the documentation heading that is to change}

Current text

Add current text in blockquotes

Proposed text

Add proposed text in blockquotes

2. Repeat the above for each page or sub-page that needs to be changed.

Remaining tasks

  1. Create this issue in the Coding Standards queue, using the defined template
  2. Add supporters
  3. Create a Change Record
  4. Review by the Coding Standards Committee
  5. Coding Standards Committee takes action as required
  6. Discussed by the Core Committer Committee, if it impacts Drupal Core
  7. Final review by Coding Standards Committee
  8. Documentation updates
    1. Edit all pages
    2. Publish change record
    3. Remove 'Needs documentation edits' tag
  9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a full explanation of these steps see the Coding Standards project page

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented

Björn Brala (bbrala) commented in the coding standards meeting, #3456119: Coding Standards Meeting Tuesday 2024-06-18 2100 UTC, that there is a rector for removing test from dataproviders, https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_...

Edit: try this link, https://getrector.com/find-rule?query=removedata

jonathan1055’s picture

jonathan1055 commented

The agreement and documenting of this standard has become more urgent in the last few weeks, because the PHPStan in contrib pipelines running 'next minor' (which is currently 11.2-dev) now report errors such as 

----- --------------------------------------------------------------- 
  Line   tests/src/Functional/SchedulerTokenReplaceTest.php                  
 ------ --------------------------------------------------------------- 
  17     @dataProvider dataSchedulerTokenReplacement() related method not    
         found.                                                              
         🪪  phpunit.dataProviderMethod

I don't know what method name it is expecting, but whatever it is I guess this should be our new standard, to be documented in the Coding ld be. Probably it would be a new paragraph in php/php-coding-standards#naming

jonathan1055’s picture

jonathan1055 commented

The link in #2 is broken. Any ideas where it was meant to point?

jonathan1055’s picture

jonathan1055 commented
Related issues: +#3351089: Fix PHPStan L1 errors "@dataProvider Foo related method not found."

I found the core issue that fixed the names - #3351089: Fix PHPStan L1 errors "@dataProvider Foo related method not found."

But actually, PHPStan at level 1 is not actually checking any expected name of the data provider function. The warning above is when checking that the test function contains the expected tag @dataProvider whateverYouCalledIt but without the () after the name.

This could also be added into the new section of the standards page.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Strong -1 from me
We don't enforce method names anywhere else, so I don't think enforcing one for data providers is appropriate

jonathan1055’s picture

jonathan1055 commented

Yes, the urgency which I stated in #3 was in fact not the case. The PHPStan warning is only complaining about the matching @dataProvider tag in the test function's doc block.

quietone’s picture

quietone commented

We do have a standard for method/function names and class names which core violates. I am not convinced that Drupal should not enforce it's own standard.

As for core, they are not enforced because a core committer has pushed back on implementing the naming standard for methods. Instead of discussing that I chose to work on implementing other standards.

quietone’s picture

quietone commented
Parent issue: » #2057905: [policy, no patch] Discuss the standards for phpunit based tests
quietone’s picture

quietone commented

For completeness that standards for naming a function are in item 2, 3, and 4 of Classes, Methods and Properties. It states the following

Methods and class properties should use lowerCamel naming. In Drupal 8, properties of configuration entities are exempt of these conventions. Those properties are allowed to use underscores.
If an acronym is used in a class or method name, make it CamelCase too (SampleXmlClass, not SampleXMLClass). [Note: this standard was adopted in March 2013, reversing the previous standard.]

It is about style not the actual name.

joachim’s picture

joachim commented

I'm +1 on this.

But:

> *single-use* providers

What happens when a single-use provider is converted to a multi-use provider because we add another test which uses it too?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

The link in #2 no longer works.
I also tend to agree with @joachim in #11, single use providers can easily go to provide for multiple test methods.

I also just commented this in the slack discussion:

I don't think the names have to match the test names. I have gotten into the habit of doing something like provideMenuLinks - a name that is based on what is being returned. If there are multiple needs it could be provideInvalidMenuLinks/provideValidMenuLinks or even provideMenuLinksAllUpperCase.
I agree with the having provide or provider in the method names, and I personally prefer the earlier, because that way we kind of force people to come up with a name that means something?

#6:

We don't enforce method names anywhere else, so I don't think enforcing one for data providers is appropriate

I mean you're right about this, it's not in the coding standards, but all test methods start with test (public function testSomething). It's also possible to use another name and annotate with @test, but I don't think we do that anywhere? At least I couldn't find something.

quietone’s picture

quietone commented

On the other hand, if dataproviders are required to be documented then the name doesn't really matter.

quietone’s picture

quietone commented

The link in #2 is copied from a comment made in a Coding Standards meeting. I did some searching and found this, https://getrector.com/find-rule?query=removedata,