Tests: introduce namespacing for the test classes

[jrf]

Introducing namespaces in the production code for WordPress Core is a hot topic, so I very purposely do NOT want to touch that in this ticket.

However, for the test suite, which doesn't get shipped with the WordPress production code, it's a whole other matter.

Benefits

Using namespaces in the test suite provides us with the following benefits:

1. If used consistently and providing they follow a set pattern (more about this below), they will make it very easy for contributors to find the test files/classes they are looking for.
2. It will allow for shorter file and test class names, while those will still be descriptive.
3. And... it will allow for mocking PHP native functions by declaring a namespaced version of that same function in the test class.
4. It will also allow more easily for multiple test classes to be created to test one particular feature/function, which the current naming scheme does not allow for. This will allow for tests for the same functionality, but which need different fixtures (setup/teardown) to be encapsulated in their own test classes.

Caveats:

As the WordPress Core test suite is used not only by Core, but also by plugins and themes for integration tests, the test class namespacing should be reserved for actual test classes and - for now - not be applied to test utility classes / Abstract base test classes (i.e. the tests/phpunit/includes directory should NOT be touched for now).

Proposed pattern

The current directory structure for tests is, to put it mildly, confusing and inconsistent.

To solve that, I would like to propose the following pattern:

• File paths: tests/phpunit/tests/wp-[includes|admin]/[SubFolder/]*Class_Under_Test/FunctionUnderTest[OptionalSubsetIndicator]Test.php
• Namespace: WordPress\Tests\WP_[Includes|Admin]\[SubFolder\]*Class_Under_Test
• Classname: FunctionUnderTest[OptionalSubsetIndicator]Test

For WP Core files which only contain functions outside of a class structure, the following pattern is proposed:

• File paths: tests/phpunit/tests/wp-[includes|admin]/[SubFolder/]*Functions_FileName/FunctionUnderTest[OptionalSubsetIndicator]Test.php
• Namespace: WordPress\Tests\WP_[Includes|Admin]\[SubFolder\]*Functions_FileName
• Classname: FunctionUnderTest[OptionalSubsetIndicator]Test

The pattern I'm proposing does imply a re-organisation of the test suite directory and file structure, but that IMO is for the better.

It also follows a PSR4-like pattern which will be more intuitive for new contributors to work with, as well as follow the PHPUnit recommended test class name pattern with having the Test as the end of the class name.

This will also allow for using PSR-4 autoloading for the tests classes and adding the autoload-dev directive to the composer.json file.

Planning

This should be regarded as a small project and not all renaming needs to be done at the same time.

New tests should start following the above proposed pattern as soon as consensus has been reached about this proposal.
Existing tests can be gradually switched over to the new pattern over time.

Additional tasks associated with this project

• [ ] Updating the contributors handbook for Core.
• [ ] Verify that the WordPressCS sniffs will validate this pattern correctly.
• [ ] Write a Make post about the decision once consensus has been reached.

[jrf]

Note: this proposal to follow PSR4 for the tests becomes all the more relevant once the test suite needs to start supporting PHPUnit 9.x.

PHPUnit 9.1.0 deprecated support for test file names which do not match the class name of the test class within.

It also deprecates the use of multiple (test) classes within a file, though I don't think that's used in WP.

Ref: ​https://github.com/sebastianbergmann/phpunit/pull/4109/

[jrf]

I've opened ​https://github.com/WordPress/WordPress-Coding-Standards/issues/1995 to adjust the WordPress Coding Standards for the above.

[jrf]

One thing which isn't addressed yet in the above proposal, but which we should probably have a think about: what to do with test specific "fixtures", mocks and doubles ?

What I mean by this, are additional files which may contain a supporting class or just some text, which are needed for a particular test.

Where should those live in the directory structure ?

If we look at other large projects, there are often three different structures to be seen:

• One "fixtures" directory at test top-level and sub-directories within that directory which follow the path to the test which the fixture applies to.
• One fixtures directory at test top-level and some form of organisation within, but not necessarily one which follows the test directory structure. Think: organisation by test groups for instance.
• A fixtures subdirectory within the (sub)directory a test lives in with the fixtures for the tests in that directory. In practices that would mean that the fixtures would be close to the tests they apply to and easy to find, but also that there will be a multitude of fixtures directories.

At this time, I'm not going to propose a direction yet. I'm just posting this here as a reminder that we need to think about this and should probably discuss this/take a decision about this before the reorganization starts.

[azaozz]

As the WordPress Core test suite is used not only by Core, but also by plugins and themes for integration tests, the test class namespacing should be reserved for ...

Yeah, I'm not really sure introducing "partial" namespacing will have any benefits. As far as I understand it will create more of a "messy" situation where some names can be repeated and other cannot. This doesn't seem helpful to anybody.

Looking at the benefits of namespacing ​as mentioned in the PHP manual, they aren't very appealing in many cases:

1. Name collisions between code you create, and internal PHP classes/functions/constants or third-party classes/functions/constants.
2. Ability to alias (or shorten) Extra_Long_Names designed to alleviate the first problem, improving readability of source code.

So, at the basic level namespaces are good when:

1. You want to name your function or class the same as an existing built-in function or class.
2. You want to avoid using longer, more descriptive names.

Both of these cases would signify a bad naming practice by promoting confusing, sloppy names and would make the code less self-documenting.

Looking at the "benefits" listed in this ticket description:

1. This seems to be a non-issue. Finding a class or a function works very well in all IDEs and IDE type text editors I've seen. Most of them would also auto-complete the name for you.
2. "Shorted file names" are not better! :) That's a disadvantage if anything.
3. Has there ever been a need to mock PHP built-in functions in "local" code? What would that accomplish?
4. This is a non-issue too. It will probably be easier to read and understand if the multiple test classes are named accordingly to what they do, i.e. self-documenting, descriptive names not short, non-descriptive, repeating names.

In that terms I don't think introducing partial namespaces in the WP PHP tests is a good idea.

[hellofromTonya]

Moving to the next cycle, though that milestone isn't yet available. For now, moving to Future Release.

[hellofromTonya]

Hey 6.0 is now available for selection. Moving this into the 6.0 cycle.

[costdev]

Per the discussion in the bug scrub, I'm moving this ticket to Future Release.

[markjaquith]

This seems like a good proposal which would help organize the tests and make it easier to find what you're looking for.

[azaozz]

May be missing something but it seems the proposed "File paths" suggest that each class should be split in up to 50 separate files, one per method. Is that correct?

If yes, could you please explain what are the benefits of having such "file and directory layout" with several thousands of files instead of 100-200? Seems this would trigger a lot of needless disk i/o and slow down running of the tests as most of the files will have to be loaded every time the tests are run.

Also seems this is not recommended (compatible?) by ​PSR4. It states that:

A fully qualified class name has the following form:
\<NamespaceName>(\<SubNamespaceNames>)*\<ClassName>

(where "The terminating class name corresponds to a file name ending in .php.").

In any case, thinking that an example of the proposed namespaces, and list of the file paths and file names would be great to have. For example: what would https://core.trac.wordpress.org/browser/tags/6.2.2/tests/phpunit/tests/blocks/register.php look like if this proposal is implemented?

### Tests/admin/wpListTable: create a testcase for tests for WP_List_Table

Most tests test testing the WP_List_Table class will need the same set up/tear down fixtures, so moving those fixtures from the actual test class to a separate abstract TestCase class will allow for expanding the tests more easily.

While doing this, we ran the tests in isolation using the --filter CLI argument and discovered that there was a previously unrecognized presumption in the fixtures that the $hook_suffix global would always be set.

This presumption proved incorrect as was demonstrated by the Undefined global variable $hook_suffix warning being thrown when running the tests.

This warning has been fixed now in the new Admin_WpListTable_TestCase by adding some additional defensive coding and making sure that the global state is always brought back to its original state, including if that original state means that the $hook_suffix global variable was not set.

### Tests/admin/wpListTable: split off tests for the get_column_info() method to separate test class

This will allow the test class to be compatible with PHPUnit 11 using the new Admin_WpListTable_TestCase class as introduced in the previous commit.

Includes removing method level @covers tags in favour of class level @covers tags to allow for supporting PHPUnit 11 attributes instead of annotations.

### Tests/admin/wpListTable: split off tests for the get_views_links() method to separate test class

This will allow the test class to be compatible with PHPUnit 11 using the new Admin_WpListTable_TestCase class as introduced in the previous commit.

Includes removing method level @covers tags in favour of class level @covers tags to allow for supporting PHPUnit 11 attributes instead of annotations.

### Tests/admin/wpListTable: rename the original test class to be more descriptive for the remaining tests

The remaining tests in the original test class all cover the magic methods and those are very closely related, so it is perfectly fine for those tests to be grouped together in one test class.

This commit, however, does rename the test class to use the new naming conventions, both for the class name as well as the file name.
It also includes removing method level @covers tags in favour of class level @covers tags to allow for supporting PHPUnit 11 attributes instead of annotations.

This will allow the test class to be compatible with PHPUnit 11.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Unlinked contributors: poena, afercia

Unlinked contributors: poena, afercia

## Tests/admin/wpPluginInstallListTable: rename the original test class to be more descriptive and adjust covers notaiton.

This will allow the test class to be compatible with PHPUnit 11.

Includes removing method level @covers tags in favour of class level @covers tags to allow for supporting PHPUnit 11 attributes instead of annotations.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

## Tests/admin/wpPluginsListTable: split the original test class and adjust covers notaitons.

This will allow the test class to be compatible with PHPUnit 11.

Includes removing method level ​https://github.com/Covers tags in favour of class level ​https://github.com/Covers tags to allow for supporting PHPUnit 11 attributes instead of annotations.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

## Tests/admin/ExportWp: rename the original test class.

This will allow the test class to be compatible with PHPUnit 11.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Rename the test class Tests_Admin_wpPostCommentsListTable to Admin_WpPostCommentsListTable_GetViews_Test so that the name includes both the class and the method that the test covers.

Moves @covers to the class level.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Tests/admin/wpMediaListTable:

The tests for WP_Media_List_Table use the same set up, tear down and helper methods. By moving them to a new abstract TestCase class, Admin_WpMediaListTable_TestCase, they can be reused by multiple tests.

The test for prepare_items is moved to a new test class; Admin_WpMediaListTable_PrepareItems_Test and the @covers is moved to the class level.
This will allow the test class to be compatible with PHPUnit 11 using the new Admin_WpListMediaTable_TestCase class.

Tests_Admin_wpMediaListTable is renamed to Admin_WpMediaListTable_GetRowActions_Test to reflect that the remaining test is for _get_row_actions. The @covers is moved to the class level.

Trac ticket:
https://core.trac.wordpress.org/ticket/53010

…ive and move @covers

Renames IncludesComment to include the path, file name, and function name: Admin_Includes_Comment_CommentExists_Test

Removes duplicate @covers and moves @covers to the test class level.

Tests/Admin/IncludesComment: Rename the test file to be more descriptive and move @covers.

Renames IncludesComment to include the path, file name, and function name: Admin_Includes_Comment_CommentExists_Test

Removes duplicate @covers and moves @covers to the test class level.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Test/Admin/IncludesFile: Split the test class into one file and test class for get_home_path and one for download_url.
The test for get_home_path is moved to Admin_Includes_File_GetHomePath_Test.
The file- and class name for the tests for download_url is renamed from IncludesFile to Admin_Includes_File_DownloadUrl_Test.

Duplicate the @covers tags are removed.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Renames IncludesUser to include the path, file name, and function name, to better indicate which function the test tests: Admin_Includes_User_WpIsAuthorizeApplicationPasswordRequestValid_Test

Moves @covers to the test class level.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Move the setup functions from Tests_Admin_WpAutomaticUpdater to a new abstract TestCase class, so that they can be used by multiple tests for the WpAutomaticUpdater class.

Splits the existing test class Tests_Admin_WpAutomaticUpdater into three classes. This will allow these test classes to be compatible with PHPUnit 11.

• class Tests_Admin_WpAutomaticUpdater is renamed to Admin_WpAutomaticUpdater_SendPluginThemeEmail_Test and covers WP_Automatic_Updater::send_plugin_theme_email

• Tests that covers WP_Automatic_Updater::is_allowed_dir are moved to a new test class: Admin_WpAutomaticUpdater_IsAllowedDir_Test

• The test that covers WP_Automatic_Updater::is_vcs_checkout is moved to a new test class: Admin_WpAutomaticUpdater_IsVcsCheckout_Test

• Duplicate @covers are removed, the remaining @covers are moved to the test class level.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

This PR splits the existing test class wpPrivacyRequestsTable into two, and creates a new TestCase.
The purpose is to make the test classes compatible with PHPUnit 11.

Move tear down and helper functions from wpPrivacyRequestsTable to a new abstract TestCase so that they can be reused in multiple tests.

Split off the test for WP_Privacy_Requests_Table::get_views into a separate test class,
Admin_WpPrivacyRequestsTable_GetViews_Test.

Correct the covers for `WP_Privacy_Requests_Table::get_views:
Previously @covers WP_Privacy_Requests_List_Table::get_views.
Updated @covers WP_Privacy_Requests_Table::get_views.
The previous was possibly a typo.

Rename the remaining test Tests_Admin_wpPrivacyRequestsTable and test file to Admin_WpPrivacyRequestsTable_PrepareItems_Test, to clarify which method that is being tested. Move the covers to the class level.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Rename the test class Tests_Admin_wpUsersListTable and the file to Admin_WpUsersListTable_GetViews_Test to clarify which method is being tested.

Moves the @covers to the class level.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

Trac ticket:
https://core.trac.wordpress.org/ticket/53010

The test class Tests_Admin_WpUpgrader (in tests/phpunit/tests/admin/wpUpgrader.php) contained tests for several methods.

This PR splits the test class into a new test class for each method, to indicate which methods are being tested, and to improve compatibility with PHPUnit 11.

Each of the split test classes has one covers annotation at the class level.

A new abstract TestCase is created, so that the set up and tear down functionality for WP_Upgrader can be reused in multiple tests. The name if the test case is Admin_WpUpgrader_TestCase

…ss to be more descriptive and adjust covers annotations.

Trac ticket: https://core.trac.wordpress.org/ticket/53010

[jrf]

Related: #62004

Note: while yes/no namespacing is still up for debate, the rest of the proposals in this ticket MUST be addressed to allow the test suite to update to PHPUnit 11/12, so this ticket is no longer just a suggestion. It is a requirement.

[jrf]

I've opened up a ticket in WPCS for this now too: ​https://github.com/WordPress/WordPress-Coding-Standards/issues/2484

[hellofromTonya]

Replying to jrf:

Related: #62004

Note: while yes/no namespacing is still up for debate, the rest of the proposals in this ticket MUST be addressed to allow the test suite to update to PHPUnit 11/12, so this ticket is no longer just a suggestion. It is a requirement.

This ticket is now part of #62004. The renaming and splitting tasks are now a requirement and prerequisite:

• [ ] Split the tests into 1 test class per each function or method. See the explanation here.
• [ ] Rename the test class to remove Tests_ prefix and add _Test suffix. See the explanation here.
• [ ] Rename each test class file to exactly match the class name. See the explanation here.

As all of the test classes will need to be touched/changed/split, what is proposed in this ticket now aligns well with these needs as @jrf explains:

As all test classes will need to be touched/changed/split to address problem 4 anyway, I would strongly recommend for the new test classes to be made fully compatible with the PHPUnit naming conventions.

This will prevent having to re-do this exercise yet again in the future if the bypass for the first rule would be made impossible.

The PRs already attached to this ticket are good examples of these tasks.

[pbearne]

point me :-)

[hellofromTonya]

Hey @azaozz, fast-forwarding to today, this proposal is now part of #62004 "Test suite: update the tests for PHPUnit 10/11 and get ready for PHPUnit 12".

As I pointed out in comment:37, the renaming and splitting are now requirements.

Replying to azaozz:

May be missing something but it seems the proposed "File paths" suggest that each class should be split in up to 50 separate files, one per method. Is that correct?

If yes, could you please explain what are the benefits of having such "file and directory layout" with several thousands of files instead of 100-200? Seems this would trigger a lot of needless disk i/o and slow down running of the tests as most of the files will have to be loaded every time the tests are run.

Let's start with the splitting of tests into 1 function / method per test class / file.

Test coverage changes in PHPUnit are now the primary driver for splitting the tests. The full explanation is found here.

tl;dr

we don't just need to split the test classes up by class, in actual fact, we'll need to split them up to individual test classes for each method in a class to still be able to measure code coverage correctly.

What are the test coverage changes driving this effort?
The full explanation is found here.

Replying to azaozz:

Also seems this is not recommended (compatible?) by ​PSR4. It states that:

A fully qualified class name has the following form:
\<NamespaceName>(\<SubNamespaceNames>)*\<ClassName>

(where "The terminating class name corresponds to a file name ending in .php.").

Fast-forwarding to today, the recommendation now is to fully compatible with PHPUnit naming conventions, which are "comply largely with PSR4 file name regulations".

Why is renaming a requirement?

As explained here, tests will not run on PHPUnit 11+ if they do not comply with the naming conventions:

1. Class name [SomethingUnderTest]Test - take note of the Test suffix.
2. The file name for the test class MUST match the class name EXACTLY in a case-sensitive manner.

An example:
Consider the test class Tests_Compat_ArrayKeyLast which is located in a file named called tests/compat/arrayKeyLast.php.

What needs to change?

1. This class will need to be renamed to Compat_ArrayKeyLast_Test.
2. The file will need to be renamed to tests/compat/Compat_ArrayKeyLast_Test.php.

Does that make sense?

[pbearne]

Hi All

So we don't have to touch the files twice as we spit them
Can we please agree on the namespacing

Paul

[hellofromTonya]

Scope of Work that can start now

The required changes for #62004 are:

1. For tests that include more than one function or method, split each function and method into a separate test class and file.
2. Rename each test class.
3. Rename each test file to exactly match the test class name.

Contributors:
Work can (and should) start on doing only this part of the scope. Currently, there are PRs/patches you can use as examples, such as ​https://github.com/WordPress/wordpress-develop/pull/6235.

Scope of Work that is still being discussed

The following parts of the proposal are not required for PHPUnit 11+, but are ideal to address now to avoid touching the files more than once:

• Namespace.
• Reorganize the test suite directory and file structure.

Currently, there's not yet consensus on these remaining parts.

Contributors:
Please don't yet make these changes in the PRs submitted.

Committers

Ideally, consensus is reached on the remaining parts of this proposal before the commits are done. Why? To avoid touching the files more than once and to minimize disruptions to existing patches/PRs.

Let's work towards getting consensus on whether to move or not with the remaining parts of the proposal (i.e. namespace and directory reorganization).

[pbearne]

I worked it out I got it wrong

I made a script to do the renaming
I have not done any spiting yet

Can you confirm the example renames is right?

Paul

Created new PHPUnit test classes for get_avatar, get_avatar_data, and get_avatar_url functions. These changes improve test organization and maintainability by grouping related tests together. Also, renamed avatar.php to Functions_GetAvatarUrl_Test.php to better reflect its content.

[pbearne]

Hi All

I have started to commit to the ​https://github.com/pbearne/wordpress-develop/tree/rename-test

I used a script to rename all the tests with a single @covers in it, renaming the file and class to match the @covers value.

There are about 500 files in the set :-)

I am working through to QA and fix/spilt as needed, like the first 3 files.

Can someone confirm that the naming is correct?

[jrf]

@pbearne To be blunt: every automated patch for this ticket is automatically disqualified.

The whole point of this exercise in the context of #62004 is to have a human who understands the task at hand:

• Verify that any existing @covers tags are actually correct.
• Decide based on those @covers tags if and if so, how to split the test class into multiple classes. Depending on the test class, this could end up being a split into dozens of new classes, which will each need careful review of what set up/tear down they need, what test case they should extend etc.
• Make sure the @covers tags are no longer at method level, but at class level.
• Decide what the right class name should be based on what is actually being tested.
• If there are no @covers tags or only partial @covers tags (for some test methods in a class, not for others), figure out what is really being tested (and no, that can't be done based on what function/methods are called in the test method, there are a lot more things at play - this needs examining the actual code coverage reports and understanding the impact of setup/teardown on those, it needs history tracing via git blame etc).

Honestly, if I thought, for even a moment, that any form of automation for this task would be helpful, I would have written a sniff with an auto-fixer for this. The fact that I didn't, should tell you enough.

In other words, the renaming is the least of our problems and should only be done as an after-thought of the test class splitting. Not the other way around.

[azaozz]

Replying to hellofromTonya:

As I pointed out in comment:37, the renaming and splitting are now requirements.
...
For tests that include more than one function or method, split each function and method into a separate test class and file.
...
Test coverage changes in PHPUnit are now the primary driver for splitting the tests.

So basically according to PHPUnit there can only be one test case per class if test coverage is to be tracked (​example)? Do I understand this correctly? If yes, this seems soooo inefficient? Is PHPUnit trying to be as slow as possible and wear out the hard disks of the computers it is running on, lol?

Joking of course, but thought PHPUnit was supposed to be ​PSR-4 compliant, right? Limiting classes to only one method is not compliant.

I'm also starting to think whether this splitting is worth it. Seems like a pretty bad degradation. Perhaps can settle down for partial/somewhat undefined coverage if that means tests will run faster and cause less problems?

the recommendation now is to fully compatible with PHPUnit naming conventions

Sure, makes sense.

The following parts of the proposal are not required for PHPUnit 11+, but are ideal to address now to avoid touching the files more than once:

• Namespace.
• Reorganize the test suite directory and file structure.

I don't really care about namespacing. Don't think it would fix or improve anything, just some overhead and very unlikely but nevertheless possible back-compat issues? But if that's your personal preference, may as well add them.

If adding namespaces, imho changing the directory structure won't break anything more :)

Frankly I'm starting to wonder if WP should just fork PHPUnit? Perhaps other older PHP projects would be happy to use/help maintain a fork? As far as I see there are always problems when it needs to be updated, and WP has to "jump through hoops" to be able to use it. Seems PHPUnit doesn't care much about its users and what are the use cases/existing code for it :(

[pbearne]

@hellofromTonya
The script is just to help with a few automation and to make sure I get the steps right.
I check every file and run the test on every.

I am confused as the what I should set he first part of the name to

Is the folder the test is in?
or the file the function is in

That said, we can now add namespace.
Could we create folders/namespace where the filename is the top folder e.g./wp_includes/functions?
And only add a prefix if the function is in a class?

I notice that the pull requests in this ticket are for a single function is this how you what the requests.
Or will you take a pull with 400 - 500 files/changes to get use started?

Paul