Page MenuHomePhabricator

CodeSniffer should detect non-static data provider in phpunit tests and report them
Open, Needs TriagePublic

Description

Non-static data providers are deprecated since PHPUnit 10.0. To assist T332865: PHPUnit data providers should be simple static functions that return plain data the CodeSniffer could detect non-static data providers in PHPUnit tests and auto-fix simple cases and possible report not so simple cases.

  • PHPUnit files are detected by PHPUnitTestTrait::isTestFile already in CodeSniffer.
  • Data providers are detect already by CodeSniffer with the naming /^provide|Provider$/.

This functions should be required to be static to increase the performance of PHPUnit on collecting all test cases. This is helpful when using --filter option.

Simple cases for auto-fix means in this case:

  • The provider only creates array(s) and is using strings or scalar in it.
  • Keep use of constant also as simple.
  • Only static calls in the provider (the StaticClosureSniff has some detection of a "static context", maybe it can be reused)
  • static closures or anon classes are okay
  • ...

Related Objects

Mentioned In
rSTULa9f20934eb9c: tests: Make some PHPUnit data providers static
rECOD1e0233e79ecc: tests: Make PHPUnit data providers static
rEMEU130161ecc7a7: tests: Make PHPUnit data providers static
rEOSA992513248b40: tests: Make PHPUnit data providers static
rETWF39b5f8b4075c: tests: Make PHPUnit data providers static
rEURE28d47d81cb61: tests: Make PHPUnit data providers static
rEUWB49b9d1c7d074: tests: Make PHPUnit data providers static
rEWSD63ac1aa771b5: tests: Make PHPUnit data providers static
rEMFCd993a8d45ce7: tests: Make PHPUnit data providers static
rEPCO99dd5e9114a8: tests: Make PHPUnit data providers static
rEEUL2cf33fbff00d: tests: Make PHPUnit data providers static
rELSA901e1913b5fc: tests: Make PHPUnit data providers static
rECXP9f5679c07d7b: tests: Make PHPUnit data providers static
rECWf18dd9e52d93: tests: Make PHPUnit data providers static
rEBVKa5dd3cafe54f: tests: Make some PHPUnit data providers static
rELNAf33b41d14dbf: tests: Make PHPUnit data providers static
rEMEE8c4a5ba3019c: tests: Make PHPUnit data providers static
rEWLC70d1140cadad: tests: Make some PHPUnit data providers static
rENLI0198b1d6a808: tests: Make PHPUnit data providers static
rEWBN5413c72af830: tests: Make PHPUnit data providers static
rESIE47ef0bedf3a8: tests: Make PHPUnit data providers static
rSTULabdb1f19a91e: tests: Make some PHPUnit data providers static
rEWRKd7e47f56b404: tests: Make PHPUnit data providers static
rEBKLedf5f7138a8f: tests: Make PHPUnit data providers static
rECOS924c896ae61c: tests: Make PHPUnit data providers static
rECPU54739aa088ab: tests: Make PHPUnit data providers static
rEDIOe917eb404b3a: tests: Make PHPUnit data providers static
rELDR226dd380df68: tests: Make PHPUnit data providers static
rESAPc96661629c51: tests: Make PHPUnit data providers static
rEMASa2359a9b56d6: tests: Make PHPUnit data providers static
rMSWA63cf1fbcbd53: tests: Make PHPUnit data providers static
rELGN76bf0ed8e387: tests: Make some PHPUnit data providers static
rEPHN646263a08086: tests: Make PHPUnit data providers static
rESCC06a8953d51fa: tests: Make PHPUnit data providers static
rECHB6a72f7c5e810: tests: Make PHPUnit data providers static
rEPS2772b56f9975: tests: Make PHPUnit data providers static
rESLFda8ba8239380: tests: Make PHPUnit data providers static
rEDIS27bfaf28035b: tests: Make PHPUnit data providers static
rEIWS78afe88004b1: tests: Make PHPUnit data providers static
rEFILEEXPORT5b95e8a47124: tests: Make PHPUnit data providers static
Mentioned Here
T332865: PHPUnit data providers should be simple static functions that return plain data

Event Timeline

Note that some providers would meet the requirements for auto fixing, but cannot be made static because they are an override of a non-static method defined in a parent class (e.g. HookRunnerTestBase).

Using Late Static Binding is a solution for those cases, and what I used in 904876.

Change 921431 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/tools/codesniffer@master] Test for static PHPUnit data providers

https://gerrit.wikimedia.org/r/921431

Change 921431 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/tools/codesniffer@master] Test for static PHPUnit data providers

https://gerrit.wikimedia.org/r/921431

Just some stats: For wmf deployed extensions/skins the sniff found 2534 "issues" in 93 extensions/skins of them, where 459 issues in 45 extensions/skins calls $this in the provider and the rest of 2075 are eligible for easy fix (but needs verify by CI).

Is there a way to see if this actually saves us time, and how much?

Is there a way to see if this actually saves us time, and how much?

I see no effect on CI. The start-up time of phpunit should/could be a small fraction faster (for example when using --filter), maybe the memory footprint of the full run is a bit lower as the objects gets created later and can be destroyed earlier (when mock builder get moved out of provider).

But the most benefit/need is, that the non-static provider are deprecated in PHPunit 10 - https://github.com/sebastianbergmann/phpunit/commit/9caafe2d49b33a21f87db248a8ad6ca7c7bdac09 - so there is already some need to do this in the future.

Change 921431 abandoned by Umherirrender:

[mediawiki/tools/codesniffer@master] Test for non-static PHPUnit data providers (with autofix)

Reason:

The detection and the autofix have false positives, so this may nothing for automated tasks or autofix

https://gerrit.wikimedia.org/r/921431