[wp-trac] [WordPress Trac] #56070: Use a consistent order of annotations in the test suite

[WordPress Trac]

#56070: Use a consistent order of annotations in the test suite
--------------------------------------+---------------------
 Reporter:  SergeyBiryukov            |       Owner:  (none)
     Type:  enhancement               |      Status:  new
 Priority:  normal                    |   Milestone:  6.1
Component:  Build/Test Tools          |     Version:
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+---------------------

Old description:

> WordPress core has an [https://developer.wordpress.org/coding-standards
> /inline-documentation-standards/php/#docblock-formatting 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 [https://make.wordpress.org/core/handbook/testing
> /automated-testing/writing-phpunit-tests/#annotations 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.

New description:

 WordPress core has an [https://developer.wordpress.org/coding-standards
 /inline-documentation-standards/php/#docblock-formatting 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 [https://make.wordpress.org/core/handbook/testing
 /automated-testing/writing-phpunit-tests/#annotations test-specific
 annotations]:
 * [https://phpunit.readthedocs.io/en/9.5/annotations.html#ticket
 `@ticket`]
 * [https://phpunit.readthedocs.io/en/9.5/annotations.html#group `@group`]
 * [https://phpunit.readthedocs.io/en/9.5/annotations.html#covers
 `@covers`]
 * [https://phpunit.readthedocs.io/en/9.5/annotations.html#depends
 `@depends`]
 * [https://phpunit.readthedocs.io/en/9.5/annotations.html#requires
 `@requires`]
 * [https://phpunit.readthedocs.io/en/9.5/annotations.html#dataprovider
 `@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.

--

Comment (by 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 [https://github.com/WordPress/wpcs-docs Coding Standards handbook
 repo] to add this format to the [https://developer.wordpress.org/coding-
 standards/inline-documentation-standards/php/ PHP Documentation
 Standards].

 Not sure if you already have, but it might be worth it to have a look
 through the draft [https://github.com/php-fig/fig-
 standards/blob/master/proposed/phpdoc.md PSR5] and [https://github.com
 /php-fig/fig-standards/blob/master/proposed/phpdoc-tags.md 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
 [https://phpunit.readthedocs.io/en/9.5/textui.html#textui-examples-filter-
 patterns `--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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56070#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform