Problem/Motivation

It has been observed in #3266443 that some tests are not being run due to missing Test suffix to the class names.

Rename such test classes and suffix them with Test.

So far the following such test classes are identified.

ckeditor5/tests/src/FunctionalJavascript/CKEditor5CKEditor4Compatibility.php <- not being run!
jsonapi/tests/src/Functional/RestExportJsonApiUnsupported.php
jsonapi/tests/src/Functional/RestJsonApiUnsupported.php
migrate_drupal/tests/src/Kernel/StateFileExists.php -> handled in #3266443
node/tests/src/Functional/NodeAccessCacheabilityWithNodeGrants.php <- not being run!

Steps to reproduce

Proposed resolution

Rename test classes and suffix them with Test.

Remaining tasks

Raise a MR to Fix rename the tests.

User interface changes

API changes

Data model changes

Release notes snippet

Relevant Draft Change Record :

https://www.drupal.org/node/3123888

Issue fork drupal-3268489

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Bhanu951 created an issue. See original summary.

bhanu951’s picture

bhanu951 commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We should try to add a test that checks for this as well, to prevent it from happening again.

Bhanu951 opened merge request !1951

bhanu951’s picture

bhanu951 commented
Status: Active » Needs review
lucienchalom’s picture

lucienchalom commented
Status: Needs review » Needs work

I've run the renamed test files and all 4 returned Ok, all pass.

migrate_drupal/tests/src/Kernel/StateFileExists.php
is listed here on the issue as missing the change on the name but is not on the MR. Why is that?

Also I don't have much idea how to create a test to check if all the test files has Test at the end. Suggestions?

I'll leave as Needs works to be open for discussion.
Thank you

bhanu951’s picture

bhanu951 commented
Issue summary: View changes

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bhanu951’s picture

bhanu951 commented

Rebased to 11.x . Still needs a test to check if test ends with Test

Bhanu951 opened merge request !4332

Bhanu951 closed merge request !4332

catch’s picture

catch
he/him
Primary language English
commented

The ending with test feels like something we could add to phpstan - any non-abstract subclass of whatever the root test base class is. I think we should open a follow-up for that. These specific tests aren't going to be renamed again once this goes in and it'll stop regressions that would cause those tests to fail.

Spokje made their first commit to this issue’s fork.

Spokje opened merge request !4914

spokje’s picture

spokje
Primary language Dutch
commented

Opened https://github.com/mglaman/phpstan-drupal/issues/608 and tried to implement it with https://github.com/mglaman/phpstan-drupal/pull/609

As to be expected, that unveiled more incorrectly named test classes.

Also, we need guidance on *TestBase classes, should they always be abstract?

For giggles, tried that in a new, separate MR, so to not mess up the one from @Bhanu951.
Seems if we make the offending *TestBase classes abstract, nothing breaks.
(The build test failures are from overriding mglaman/phpstan-drupal with my PR containing the new rule)

catch’s picture

catch
he/him
Primary language English
commented

The base classes should always be abstract yes, not sure whether it's documented but it's what should happen.

bhanu951’s picture

bhanu951 commented
Related issues: +#3268490: Rename and organise Test Traits

One more related issue #3268490: Rename and organise Test Traits which we need to work on.

bhanu951’s picture

bhanu951 commented
Related issues: +#2793701: Ensure that phpunit tests end with Test.php, +#2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix, +#2301251: Remove the *Test.php suffix requirement
bhanu951’s picture

bhanu951 commented

We have a test to check file names here we can utilise it for resolving this issue.

bhanu951’s picture

bhanu951 commented
Related issues: +#2465029: Ensure that PHPUnit tests are in the correct namespace

Seems this issue can resolve few more old issues.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Rename test classes not ending with Test to end with Test » [PP-upstream] Rename test classes not ending with Test to end with Test
Status: Needs work » Postponed

Thanks @catch for the confirmation on the "base classes should always be abstract".

@Bhanu951: Not sure if we want to solve all the mentioned issues in one go and or in this one issue.

Let's try the get the proposed rule in PHPStan first and take it from there. Extending it later to deal with class names and Traits seems more manageable.

Postponing this issue on:

- Getting the proposed rule PR into mglaman/phpstan-drupal
- A release of mglaman/phpstan-drupal with that rule in it.

If that's done we can up the version of mglaman/phpstan-drupal in the root composer.json to that release and fix the found problems both in this issue.

catch’s picture

catch
he/him
Primary language English
commented

I'm not sure we should postpone getting 'ghost' tests to run on the phpstan rule, since every day they don't run is an additional chance we could commit something that will make them fail, so would be fine with reversing the order.

spokje’s picture

spokje
Primary language Dutch
commented

Fair point, although it looks like all of the tests are running with our without the "Test" prefix:

From QA run https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/86948/...

00:04:35.008 Drupal\Tests\Core\Theme\CoreThemesAutoloadedForTests           1 passes                                      
00:09:13.038 Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateTermNodeCompl   1 passes                                      
00:16:53.327 Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7TestWit   1 passes                                      
00:17:18.780 Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6TestWit   1 passes     
00:17:38.772 Drupal\FunctionalTests\Asset\AssetOptimizationTestUmami        1 passes                                       
00:17:39.009 Drupal\Tests\block_content\Functional\Update\BlockContentRem   1 passes             
00:21:08.832 Drupal\FunctionalTests\Installer\InstallerExistingConfigProf   1 passes           
00:21:09.731 Drupal\FunctionalTests\Installer\InstallerExistingConfigSync   1 passes                                                                                                                        
00:28:16.826 Drupal\Tests\help\Functional\Update\HelpTopicsMerge            1 passes                                      
00:28:16.941 Drupal\Tests\help\Functional\Update\HelpTopicsUninstall        1 passes      
00:21:35.225 Drupal\Tests\jsonapi\Functional\RestJsonApiUnsupported         1 passes                                                                      
00:21:35.592 Drupal\Tests\jsonapi\Functional\RestExportJsonApiUnsupported   1 passes                                      
00:23:26.689 Drupal\Tests\node\Functional\NodeAccessCacheabilityWithNodeG   1 passes         
00:28:34.944 Drupal\Tests\system\Functional\Update\UpdateDescriptionConfi   1 passes

Spokje closed merge request !4914

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.