Make WordPress Core

Opened 3 years ago

Last modified 6 months 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
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 (16)

#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
2 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
2 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
2 years ago

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

#8 @jrf
2 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
2 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 2 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.


22 months ago

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


20 months ago

#14 @costdev
20 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 20 months ago by costdev (previous) (diff)

#15 @markjaquith
16 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
6 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 6 months ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.