Use a consistent order of annotations in the test suite

[SergeyBiryukov]

WordPress core has an ​established DocBlock format for inline documentation:

/**
 * Summary.
 *
 * Description.
 *
 * @since x.x.x
 *
 * @see Function/method/class relied on
 * @link URL
 *
 * @global type $varname Description.
 * @global type $varname Description.
 *
 * @param type $var Description.
 * @param type $var Optional. Description. Default.
 * @return type Description.
 */

This is more or less consistently applied in core, which is helpful for reusing this template for newly added functions without the guesswork of where to put each particular tag.

Unit tests also use some of these tags:

• @since
• @see
• @global
• @param
• @return (for tests with dependencies)

as well as some ​test-specific annotations:

• ​`@ticket`
• ​`@group`
• ​`@covers`
• ​`@depends`
• ​`@requires`
• ​`@dataProvider`
• @expectedDeprecated
• @expectedIncorrectUsage

However, the order of these annotations differs in various test classes and can be almost random even in test methods of the same class. These inconsistencies increase cognitive load when writing new tests or reviewing test PRs to bring them in line with existing tests.

I would like to propose a DocBlock template that can be consistently applied across the test suite. Something like:

/**
 * Summary.
 *
 * Description.
 *
 * @since x.x.x
 * @ticket 12345
 *
 * @group group_name_1
 * @group group_name_2
 *
 * @covers function_name_1
 * @covers function_name_2
 *
 * @requires function function_name
 *
 * @expectedDeprecated
 * @expectedIncorrectUsage
 *
 * @see Function/method/class relied on
 * @link URL
 *
 * @depends test_name
 * @dataProvider data_provider_name
 *
 * @global type $varname Description.
 * @global type $varname Description.
 *
 * @param type $var Description.
 * @param type $var Optional. Description. Default.
 * @return type Description.
 */

Notes:

• All of these annotations are optional and may not be present on a particular test, so most of the time the DocBlock would be much shorter. But if they are present, the order should be consistent across the test suite.
• @since and @ticket are grouped together because they are both related to when a test was introduced.
• @group and @covers are separated into their own sections for better readability when a test belongs to multiple groups and/or covers multiple functions.
• @depends and @dataProvider are grouped together and moved closer to globals and parameters, because they are both related to passing data to the test. When reviewing the current usage of @depends in the test suite, I found some instances that don't pass any data but seem to (incorrectly) assume that this annotation defines the order in which the tests are executed. That can be addressed separately.

Any thoughts on using this DocBlock format or any suggestions for improving it are welcome.

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

[davidbaumwald]

Thank you for flagging this. It would be nice to update the handbook with this new standard also.

[jrf]

Thanks for the ping @SergeyBiryukov !

These inconsistencies increase cognitive load when writing new tests or reviewing test PRs to bring them in line with existing tests.

I fully agree and applaud this initiative.

Once the format has been settled upon, please consider sending in a PR to the ​Coding Standards handbook repo to add this format to the ​PHP Documentation Standards.

Not sure if you already have, but it might be worth it to have a look through the draft ​PSR5 and ​PSR19 guidelines to see if the format as currently proposed is at least in line with those. This is, of course, not a "must-have", but it would be a "nice-to-have" to not increase the barrier of entry for contributing to WP.

As for feedback on the proposed format:

• I would move the @see and @link tags higher up - just below (or even above) the @ticket and @since tags as this information will normally be related to the situation being tested as described in the description.
• Along the same lines, I would reverse the order between the @covers block and the @group block as the information in the @covers tag is about what is being tested, while the information in the @group tag is about potentially selectively running the tests.
• @group could even potentially be moved below @requires.

As discussed in another ticket previously, the @global tag should be considered discouraged as it doesn't actually add any functional value and the same information will be contained within the test (and API documentation won't be generated for tests anyhow and modern generation tools don't need the @global tag anymore either).

Along the same lines, aside from some very specific exceptions - the groups being run separately in CI -, IMO the @group tag also should be considered discouraged as the ​`--filter` CLI argument of PHPUnit has come a long way and is generally much more versatile than using @group.

For documenting the format, it might be a good idea to also mention the ​`@coversNothing` tag in the @covers block as this is a tag which will at times be used.

Additional PHPUnit specific tag to consider adding to the format: ​`@doesNotPerformAssertions`.

For the record, also note that the @ticket tag is actually an alias for the PHPUnit @group tag.

Ref: ​https://phpunit.readthedocs.io/en/9.5/annotations.html

---

As a final note: once I have some breathing room (may still be a while), I intend to greatly improve the WordPress-Docs standard and verifying a standardized tag order would definitely be one of the things I'd like to add to WPCS.

[dingo_d]

I love seeing more structure in the inline docs, kudos for creating this ticket to bring more consistency across the codebase!

I have one question about checking docblocks.

Are we checking inline docblocks for the structure/order? And do we intend to do so using tools like PHPCS?

I also agree with Juliette that we should see if we can align our docblocks with the official proposals for the docblocks in the PHP community. Even though those are still drafts (should also keep an eye on PER standards if they will add inline docs to their list).

[desrosj]

Beta 1 is in a few hours. This one is strictly test and doc related, so it can remain in 6.1 as long as someone still plans on wrapping this up in the next few weeks.

@SergeyBiryukov are you still anticipating that you'll be able to work on this one?

[SergeyBiryukov]

Let's move to 6.2 for now, this still needs some time to address the feedback from comment:4 and apply the changes to the existing test classes.

[peterwilsoncc]

@SergeyBiryukov @jrf @dingo_d I agree this is a great idea, is it possible to put together a proposal to post on make/core?

It looks like there is still some discussion about order to be finalised, so having that wrapped up would be lovely.

[costdev]

This ticket was discussed in the recent bug scrub. As this ticket can be committed at any time during the cycle, and has support from Core developers involved in the discussion, we agreed to change this ticket's type to Task (blessed).

[desrosj]

This one still needs a little bit of discussion. Though it's test related and can go in after RC, I think it makes more sense to let move to 6.4 since there has been no movement during the 6.3 cycle.

[flixos90]

I would like to bring attention to this ticket as the lack of documentation on WordPress core's requirements for test annotations has been problematic in the past couple months, requiring further iterations on otherwise completed PRs, based on something that is not documented and therefore feels arbitrary.

I would like to better understand where this ticket is at and what the next steps are. The format that @SergeyBiryukov outlined in the ticket description makes total sense to me, so I wonder what are we looking for: Are we still discussing whether that's the right format? Are we waiting to write a proposal? Update the documentation? Update all existing tests?

From my perspective, the thing that is lacking here right now is documentation. Given that we already would like new PRs to provide more appropriate annotations on tests, we should document whatever is currently expected - even if the ticket still remains open for further discussion or work (documentation can still be updated again later if needed).

​https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/#annotations currently doesn't mention any of these requirements, and I think we cannot expect PR authors to be aware of or follow "unwritten" rules.

[costdev]

@flixos90 From my perspective, the thing that is lacking here right now is documentation. Given that we already would like new PRs to provide more appropriate annotations on tests, we should document whatever is currently expected - even if the ticket still remains open for further discussion or work (documentation can still be updated again later if needed).

+1

@peterwilsoncc I agree this is a great idea, is it possible to put together a proposal to post on make/core?

Is it possible that we could decide this within this ticket? I don't think this is something that is likely to garner much feedback or many iterations via a Make/Core post beyond what's already been shared in this ticket.

Having something in the handbook sooner rather than later to point contributors to will greatly help encourage good and consistent test documentation practices. Until now, it's either been a case of not mentioning a consistent order/certain annotations at all, or in an effort to have something in place, linking to this ticket as a useful reference.

[jrf]

the lack of documentation on WordPress core's requirements for test annotations has been problematic in the past couple months, requiring further iterations on otherwise completed PRs

@flixos90 why was this problematic ? why did this cause iterations on PRs ?
As there is no standard, there is nothing to enforce, other than the committers personal preference (at this time).

what the next steps are

As formalizing an order requires a change to the ​coding standards handbook, the next step would be a proposal via a Make post.

Having said that, I don't think we're ready for that yet as there was never any response to the feedback already given in this ticket. I believe those remarks should be addressed first.

Is it possible that we could decide this within this ticket? I don't think this is something that is likely to garner much feedback or many iterations via a Make/Core post beyond what's already been shared in this ticket.

@costdev No, it can't. It has been made very clear to me and others at various points in time, that no matter how innocent the change, coding standards changes MUST go via a proposal on Make.
I see no reason why this proposal would be exempt from that rule.
If there's not much feedback/push-back, all the better as that means that the proposal can move forward reasonably quickly after the post has been published.

Process-wise, in my view, the steps are:

• Get some form of agreement on the format to propose from the current stakeholders (people involved in this ticket).
• Write & publish a Make post proposing an addition/update to the Coding Standards handbook.
• If approved/there are no objections: submit a PR to the Coding Standards handbook repo & get that merged.
• Update the code samples in the test handbook where necessary to comply with the new rules + add links to the Coding Standards which apply (possibly more updates are needed, this would need to be evaluated at that time).
• Open a feature request ticket in the WordPressCS repo for enforcement of the new rules (may be a while before that gets addressed).
• Green light for updating existing tests.

[peterwilsoncc]

As RC1 is happening in a few hours, I'll move this off the milestone.

@jrf's comment above details the steps required to make this change. tl;dr: those of us on this ticket need to decide on an order and propose it on make/core before proceeding.