WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 7 months ago

#35065 closed enhancement (reported-upstream)

Tests for docblock quality

Reported by: johnbillion Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch ongoing
Focuses: docs Cc:
PR Number:

Description

Attached is a patch which implements various tests for docblocks in PHP files. The patch is experimental and is intended only to determine whether unit tests for docblocks are a good idea or not.

Currently tested for each function/method:

  • Existence of docblock.
  • Non-emptiness of docblock.
  • Correct number of @param tags.

Currently tested for each parameter of a function/method:

  • Non-emptiness of description.
  • Name matches actual parameter name.
  • Various type hint validations (not so appropriate for core).
  • Existence or not of "Optional" string.
  • Existence or not of "Default" string.

Currently this only tests functions/classes which are loaded in the current scope. phpDocumentor's static reflection library allows static analysis of files instead of using PHP's built-in Reflection API, which would allow for all functions and classes to be tested.

cc @DrewAPicture

Attachments (3)

docblocks.diff (98.4 KB) - added by johnbillion 4 years ago.
docblocks.2.diff (7.1 KB) - added by johnbillion 4 years ago.
docblocks.3.diff (7.3 KB) - added by johnbillion 4 years ago.

Download all attachments as: .zip

Change History (17)

#1 @johnbillion
4 years ago

  • Keywords has-patch added

Usage:

  1. Adjust the $to_search, $files, $test_functions, and $test_classes variables in Tests_DocBlocks::dataReflectionTestFunctions() as appropriate.
  2. Run phpunit --filter Tests_DocBlocks.
  3. Observe many failures.

#2 @johnbillion
4 years ago

The phpDocumentor library that's used is PHP 5.3+, so these tests would need some logic so they only run on PHP 5.3+.

#3 follow-up: @joehoyle
4 years ago

Is this not the job of some kind of code sniffer rather than unit testing?

#4 in reply to: ↑ 3 ; follow-up: @johnbillion
4 years ago

Replying to joehoyle:

Is this not the job of some kind of code sniffer rather than unit testing?

Potentially, yes, but we don't have an infrastructure for this currently. WPCS could be used, but there's nothing in there for docblock quality and it would have to be run separately from the PHP rules in WPCS because WordPress itself (naturally) doesn't yet adhere to its standards.

#5 @tw2113
4 years ago

What about collecting data for a report regarding file location and function name to ease finding where one would need to work on?

#6 in reply to: ↑ 4 @jdgrimes
4 years ago

Replying to johnbillion:

Replying to joehoyle:

Is this not the job of some kind of code sniffer rather than unit testing?

Potentially, yes, but we don't have an infrastructure for this currently. WPCS could be used, but there's nothing in there for docblock quality and it would have to be run separately from the PHP rules in WPCS because WordPress itself (naturally) doesn't yet adhere to its standards.

This is actually incorrect. WPCS includes the WordPress-Docs ruleset for checking docblock quality (thanks to @GaryJ). So you can run phpcs --standard=WordPress-Docs and just check for docblock issues. It is possible that the rules won't match exactly what WordPress core expects, but PRs are welcome :-).

#7 @GaryJ
4 years ago

The rules that exist in WordPress-Docs were just the easy wins that utilised existing Sniffs that are part of existing standards that ship with PHPCS (i.e. Squiz and Generic) - see https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Docs/ruleset.xml .

This is in no-way comprehensive, and doesn't check for many WP idiosyncrasies. The ruleset.xml in fact is mainly made up of exclusions from those more typical existing standards, and would therefore need custom Sniffs to add those valid checks in.

#8 @johnbillion
4 years ago

docblocks.2.diff switches to static analysis instead of PHP's built-in Reflection, so classes and functions that are not loaded in the current scope can be tested too. Needs Composer to set up before running the tests:

  1. cd tests/phpunit
  2. composer install

See the $to_search and $files variables in the Tests_DocBlocks::dataReflectionTestFunctions() method for tweaks that can be made, and also the $ignore_empty static property on the class which silences errors caused by functions and methods which completely lack a docblock.

At this point I think it's clear that such tests don't need to be in core's tests, but having them allows the docs team to work through the issues that this patch does highlight.

See #32246 for ongoing results.

#9 @johnbillion
4 years ago

docblocks.3.diff corrects the composer.json file.

#10 @johnbillion
4 years ago

  • Keywords ongoing added

This ticket was mentioned in Slack in #docs by morganestes. View the logs.


3 years ago

#12 @morganestes
3 years ago

@johnbillion anything folks can do to help you on this one?

#13 @johnbillion
3 years ago

I have a standalone repo for this: https://github.com/johnbillion/php-docs-standards and I was planning a WordPress specific set of rules based on the above.

Seeing as the static analyses approach requires a Composer dependency, maybe we can close this ticket as maybelater, or we can switch to using WPCS for docblock quality sniffing.

#14 @johnbillion
7 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to reported-upstream
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.