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

Created on 21 June 2024, 12 months ago

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}

Add current text in blockquotes

Add proposed text in blockquotes

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

Remaining tasks

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

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

📌 Task
Status

Active

Component

Coding Standards

Created by

🇳🇿New Zealand quietone

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @quietone
  • 🇳🇿New Zealand quietone

    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_...

  • 🇬🇧United Kingdom jonathan1055

    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

  • 🇬🇧United Kingdom jonathan1055

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

  • 🇬🇧United Kingdom jonathan1055

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

    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.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

  • 🇬🇧United Kingdom jonathan1055

    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.

  • 🇳🇿New Zealand quietone

    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 📌 Use 'Drupal.NamingConventions.ValidFunctionName' in core/tests RTBC . Instead of discussing that I chose to work on implementing other standards.

  • 🇳🇿New Zealand quietone

    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.

  • 🇬🇧United Kingdom joachim

    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?

Production build 0.71.5 2024