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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, test.suffix.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
19.34 KB
sun’s picture

Issue tags: +Quick fix
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

+1 for Theme => Engine prefix renames

sun’s picture

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

chx’s picture

Status: Reviewed & tested by the community » Needs work

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

sun’s picture

Status: Needs work » Needs review

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

pwolanin’s picture

Can you pull the menu-related ones out for now?

sun’s picture

@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?

pwolanin’s picture

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

sun’s picture

chx’s picture

Issue summary: View changes

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

sun’s picture

@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.)

sun’s picture

Issue summary: View changes

@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).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fea9090 and pushed to 8.x. Thanks!

  • alexpott committed fea9090 on 8.x
    Issue #2293825 by sun: Fixed Various test classes do not have a "Test"...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.