Make WordPress Core

Opened 3 years ago

Last modified 4 days ago

#53010 assigned task (blessed)

Tests: introduce namespacing for the test classes

Reported by: jrf's profile jrf Owned by: hellofromtonya's profile hellofromTonya
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: 2nd-opinion has-patch has-unit-tests
Focuses: docs, coding-standards Cc:

Description (last modified by 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.

Change History (24)

#1 @jrf
3 years ago

  • Description modified (diff)

#2 @jrf
3 years ago

  • Description modified (diff)

#3 @johnbillion
3 years ago

  • Version trunk deleted

This ticket was mentioned in Slack in #core by ayesh. View the logs.


3 years ago

#5 @jrf
3 years ago

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/

#6 @jrf
3 years ago

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

#7 @hellofromTonya
3 years ago

  • Milestone changed from Awaiting Review to 5.9
  • Owner set to hellofromTonya
  • Status changed from new to assigned

#8 @jrf
3 years ago

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.

#9 @azaozz
3 years ago

  • Keywords 2nd-opinion added

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.

Last edited 3 years ago by azaozz (previous) (diff)

#10 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to Future Release

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

#11 @hellofromTonya
2 years ago

  • Milestone changed from Future Release to 6.0

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

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by costdev. View the logs.


23 months ago

#14 @costdev
23 months ago

  • Milestone changed from 6.0 to Future Release

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

Last edited 23 months ago by costdev (previous) (diff)

#15 @markjaquith
20 months ago

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

#16 @azaozz
9 months ago

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?

Last edited 9 months ago by azaozz (previous) (diff)

This ticket was mentioned in PR #6235 on WordPress/wordpress-develop by @jrf.


12 days ago
#17

  • Keywords has-patch has-unit-tests added

### 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

@jrf commented on PR #6235:


12 days ago
#18

Unlinked contributors: poena, afercia

@jrf commented on PR #6235:


12 days ago
#19

Unlinked contributors: poena, afercia

This ticket was mentioned in PR #6272 on WordPress/wordpress-develop by @afercia.


5 days ago
#20

## 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

This ticket was mentioned in PR #6273 on WordPress/wordpress-develop by @afercia.


5 days ago
#21

## 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

This ticket was mentioned in PR #6274 on WordPress/wordpress-develop by @afercia.


5 days ago
#22

## 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

This ticket was mentioned in PR #6276 on WordPress/wordpress-develop by @poena.


4 days ago
#23

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

This ticket was mentioned in PR #6277 on WordPress/wordpress-develop by @poena.


4 days ago
#24

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

Note: See TracTickets for help on using tickets.