Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Various test classes (both Simpletest + PHPUnit) do not end in "Test[.php]".
PHPUnit tests actually must end in *Test.php
to get discovered by PHPUnit.
For consistency, and because it is a simple + sensible + well-defined standard, all Simpletest tests should follow that lead.
Comment | File | Size | Author |
---|---|---|---|
#12 | test.suffix.2.sans-useraccountslinkstest.patch | 18.55 KB | sun |
#2 | test.suffix.2.patch | 19.34 KB | sun |
#2 | interdiff.txt | 1.02 KB | sun |
test.suffix.0.patch | 18.18 KB | sun | |
Comments
Comment #2
sunComment #3
sunComment #4
ParisLiakos CreditAttribution: ParisLiakos commented+1 for Theme => Engine prefix renames
Comment #5
sunTo clarify ahead of time:
A "Test[.php]" suffix is not a requirement yet - only for PHPUnit tests.
Making it a requirement would certainly improve test discovery performance (~10% less files to reflect/introspect), but separate issue.
Comment #6
chx CreditAttribution: chx commentedNope. That stupid thing *Test.php should be removed from phpunit instead. It just makes everyone's life harder. getinfo or class annotation is enough to discern the class no need to make it harder.
Comment #7
sunI think many others will disagree with that stance. Consistency makes everyone's life easier.
PHPUnit filters by *Test.php for a good reason; only a Test is a test. Other .php files may exist, but are not tests.
It is not a good idea to diverge from PHPUnit's native upstream behavior. Drupal would have to write its own custom documentation for an industry standard tool. New developers would have to learn "Drupal's PHPUnit" first.
Already in D7, all Simpletest tests were commonly using a suffix of "TestCase". In D8, the suffix has been shortened to just "Test". That happens to be consistent with PHPUnit tests.
As visible in the patch, only 20 test classes (1.4%) do not have a Test suffix. The only change performed by this patch is to rename those few outliers to be consistent with all others.
It's not clear why we cannot rename those classes for consistency.
Comment #8
chx CreditAttribution: chx commentedI will link this issue every time someone asks in frustration why her test is not picked up. PHPunit's behavior is extremely annoying and I have been frustrated to no end because my tests often didn't get picked up. Newsflash: just because a third party library does something braindead does not mean Drupal should be doing something braindead as well. In fact, we could fix it. But I know this is absolutely anathema to Drupal 8, being developer friendly is absolutely second to being a sycophant to third party libraries. I shouldn't have talked up and the amount of issues I comment on (none) versus I did try here should give everyone pause that this might actually be important. Bleat on, please, I am out.
Comment #9
pwolanin CreditAttribution: pwolanin commentedCan you pull the menu-related ones out for now?
Comment #10
sun@pwolanin: Only two related tests are affected here:
Drupal\language\Tests\Menu\LanguageLocalTasksTest (phpunit)
Drupal\user\Tests\UserAccountLinksTest (web)
Shouldn't they be mostly agnostic to a Menu API/subsystem refactoring under the hood?
Comment #11
pwolanin CreditAttribution: pwolanin commented@sun - we aren't touching local tasks, so actually just the UserAccountLinksTest
We are changing the implementation of the link there, so I'd rather not have to deal with a file move.
Comment #12
sunSure.
Comment #13
chx CreditAttribution: chx commentedOr maybe not. It's not a well-defined standard by any means but an idiocy that PHPunit forces on us. The breadth of this patch: there are language, migrate, node, theme, update, blocks and views tests suffering from this, even if they are not many, should give everyone pause and a glimpse into the future -- if we enforce the *Test.php suffix for everything -- which this patch precludes -- will cause everyone a lot of grief and not picked up tests. Instead, this issue should be won't fixed, #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit) finished and the discovery made namespace + annotation based. Which, by the way, is already used extensively throughout D8: plugins. It would decrease the number of patterns one would need to learn to work with D8 which, to be honest, would be a small miracle. Except that phpunits annotations are not the same syntax as Doctrine annotations. Well defined standards, eh.
Comment #14
sun@chx: It is clear by now that you do not like PHPUnit's filtering for *Test.php files. Others may not necessarily share your opinion. Repeating the same opinion all over again does not add any value to the discussion.
Besides that, this patch only renames a few tests to make them consistent with all others. It does not introduce a filter for *Test.php files for Simpletest tests. (Only PHPUnit filters for *Test.php files by default, for good reasons.)
The discussion about filtering Simpletest tests to *Tests.php belongs into a separate issue, which I may or may not create later on, pending further analysis. (In case I do, I'll link to it from here.)
Comment #15
sun@chx: You may now continue to express your opinion over in:
#2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix
That said, I think I've outlined the main concern a bit more accurately in the summary of the new issue already (which, FWIW, would not have required any passive-aggressive rants and whatnot at all, because it's a perfectly legit concern to raise).
Comment #16
alexpottCommitted fea9090 and pushed to 8.x. Thanks!