Missing @covers in the comment blocks in PHPUnit tests

[pbearne]

PHPUnit has a comment block notation @cover and @uses that we are not using so we can create coverage report to see how much of WordPress has tests

​https://thephp.cc/news/2014/03/phpunit-4-0-code-coverage-improvements
​http://blog.teamlazerbeez.com/2009/08/18/phpunit-annotations/#covers
​https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.covers

@covers tells PHPUnit that this test what the test function is testing

    /**
     * @covers Class_Name::function_name
     */

@uses tells PHPunit that the test is using another function to help test the @covers function but this test is not testing it so it should ignore it for the covers report

     /**
     * @covers Class_Name::function_name
     * @uses   Other function
     */

note: if the @ covers function that is not in a class and in global namespace then the notation is like this

     /**
     * @covers ::function_name
     */

This ticket is place holder for how and what the changes need to be so that we can encourage the fixing of this.

[jnylen0]

While I am definitely a proponent of thorough unit tests, I am -1 on adding annotations for code coverage, and in general I think the @covers annotation is not a good idea. I will try to explain why.

Someone will have to update every single test to indicate which functions are covered. This will be a manual process requiring an enormous amount of effort, not only now, but in perpetuity. I can't speak for everyone but I personally don't want to do this in my patches.

It is virtually guaranteed that tests will not be consistently updated with @covers annotations in the future. (It's hard enough to ensure that tests are written in the first place.) Even if we do a huge effort now to add them, the coverage reports will become essentially useless in the future due to attrition.

Per-function coverage is not a very useful thing to report on. Consider: @covers wp_update_post. This is a very long function with a lot of complicated logic. Line-by-line coverage reporting is far more useful.

I don't want to read lots of lines of @covers and other annotations in the test code. I think we already have too many of these annotations and they are just visual noise, because the information about which functions are called is already present in the test code itself.

I would be far more interested in an effort to provide automated code coverage reports on a line-by-line basis, perhaps with results split by WordPress component or something like that. I do think this would be really valuable, and once it's set up, there's far less ongoing maintenance required.

[pbearne]

Hi @jnylen0

How would we go about providing automated code coverage reports on a line-by-line basis? This would good but can it be done?

A good test will only be testing one function so I expect to see just one @covers per test.

We went through the code base and add comments to actions and filters this a smaller task and we already have volunteers to do the heavy lifting who just waiting for the OK to get started

Paul

[jnylen0]

Most of ​https://phpunit.de/manual/current/en/code-coverage-analysis.html is dedicated to setting up automated code coverage.

Reading closer, the @covers annotation is a supplement to this automated coverage, and I don't think you can generate a report from manual annotations only.

@jorbin has already set this up: ​https://daily.jorb.in/2015/10/automated-code-coverage-of-wordpress-core-php-code/

The basic way to run it is as follows, but I get a mysterious error when I do so:

$ phpunit --coverage-html coverage-dir
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.15 by Sebastian Bergmann and contributors.

Cannot modify header information - headers already sent by (output started at /home/james/wp/wordpress-develop-official/tests/phpunit/includes/bootstrap.php:62)

[pbearne]

So the end point is to get coverage reports so if this work then we are good

The error is due to print statements e.g. "Installing..." there is a bug ticket for that we need away to turn it off for coverage reports.

[tloureiro]

Hey, @jnylen0 what version of phpunit are you using?
I got the same bug in version 5.1.3. Downloading and using the phpunit 5.7 version .phar worked for me. The report was generated successfully and it looks like this:
​https://ibb.co/e4UNAk

Also, I couldn't see any results for code coverage in @jorbin's page (​https://codecov.io/gh/aaronjorbin/wordpress.codecov)

[jrf]

I'd highly recommend for this ticket to be revisited for WP 5.6.

As explained in more detail in #50902, there are significant changes in PHP 8.0 which we will only be able to detect breakage from via unit tests.

Knowing which code is not, or insufficiently, covered by tests will allow a more focused approach for expanding the test suite to ensure compatibility with PHP 8.0.

---

Regarding the use of the @covers tags, reading the above discussion there seems to be some confusion about what that tag does and why it is so extremely useful (and important!), so let me try and explain it:

Let's take a unit test which is set up to test function foo(). Function foo() calls the functions bar() and baz() under the hood as part of its logic.

Now when running a code-coverage report, you will see that code coverage is recorded for function foo(), bar() and baz(), while in reality we were only testing function foo().
This is called incidental code coverage - covering code in functions/classes/methods which are not explicitly being tested. It is basically a false positive for which code is covered by a test.

With incidental code coverage, it is easy to think your code base has a high test coverage, while in reality there are no dedicated tests for a lot of the code.

In comes the @covers tag. The @covers tag ensures that only the code from the functions/methods/classes listed in @covers tag will be regarded as being covered by a specific test.
So, when we add a @covers ::foo tag to the test from the example and run code coverage again, we will see that code coverage has only been recorded for the code from the foo() function and that the bar() and baz() functions have 0% code coverage.

Once @covers tags have been added to all tests, the next step would be to add the forceCoversAnnotation=true setting in the phpunit.xml.dist configuration file. With that setting turned on, code coverage will only be recorded if a test has a @covers annotation. This effectively prevents incidental code coverage from entering the code coverage reports as only the code covered by tests with a @covers tag will be included.

Also note: The tests don't all have to have individual @covers tags. If a test class, through a variety of tests, covers all aspect of one particular function, a single @covers tag for that function in the test class docblock will suffice. Same if a test class covers every aspect of a class. A single @covers tag referencing the covered class in the test class docblock is enough in that case.

For more information, have a look at: ​https://phpunit.readthedocs.io/en/7.5/annotations.html#covers, ​https://phpunit.readthedocs.io/en/7.5/code-coverage-analysis.html#specifying-covered-code-parts and ​https://phpunit.readthedocs.io/en/7.5/risky-tests.html#risky-tests-unintentionally-covered-code

[jrf]

Oh and regarding the @uses tag - I would leave that out, using that tag in combination with the beStrictAboutCoversAnnotation=true setting will mark a test as "Risky" if code is executed during the test which is not annotated via @covers or @uses tags.

That would make the maintainance of the tags very fiddly, while adding little additional value for this test code base.

[pbearne]

Are we go on for lots of little patches or should this get done on github and get pushed one big patch?

Glad to see this getting picked up

Paul

[jrf]

Replying to pbearne:

Are we go on for lots of little patches or should this get done on github and get pushed one big patch?

@pbearne Well, it is a lot of work and to do this right, every tests without the @covers tag will need to "studied" to see what the test coverage intention was when the test was added, possibly tracing this back to the patch/ticket via which the test was added etc. For some this will be simple (only one WP method/function is being called), for some not so much.

With that in mind, I think starting with small patches, something like a file (if complex) or directory (if not that complex) at a time, would probably be the way forward.

Once the number of @covers tags has increased significantly, we could consider one final patch to finish it off.

I'm working on setting up a PHPUnit QA standard for PHP_CodeSniffer which could help detect missing @covers tags as well as incorrect ones. Unfortunately that isn't public yet, though the ​YoastCS PHP_CodeSniffer standard contains some preliminary versions of sniffs, but those are in part too specificly attuned to their codebases to be used for WP Core for now.

All the same, if I adjust those slightly and do a scan, I currently see there are (at least) 3298 test methods which don't have a @covers tag either at the method or the class level and 191 @covers tags for which the () at the end should be removed. That last bit can be auto-fixed, so I'll upload a patch for that in a minute.

[jrf]

Clean up some existing @covers tags. The () at the end is unnecessary and disregarded by PHPUnit, so can be removed.

[SergeyBiryukov]

In 48858:

Tests: Clean up some existing @covers tags.

The () at the end is unnecessary and disregarded by PHPUnit, so can be removed.

Props jrf.
See #39265.

[pbearne]

So let's have patch per folder and one for the root
this will 51 patches this will be a good way to split the task up
I am working with a group at codeable.io to do this a project :-)

if we get this done quickly should add the @uses or not?

[SergeyBiryukov]

Replying to pbearne:

if we get this done quickly should add the @uses or not?

Per comment:8, I think @uses should not be added.

Per the ​documentation standards:

Note: This tag has been used in the past, but should no longer be used.

[pbearne]

Covers add to Functions folder

[patopaiar]

Adds @covers to folders starting with d: date, db and dependencies

[TimothyBlynJacobs]

@jrf Do you have suggestions on how @covers should be used with the REST API endpoint tests? We are generally doing more “end-to-end” like tests. In other words actually creating a request object and dispatching it to the server instead of calling the controller methods directly.

[patopaiar]

Adds @covers to folders starting with e: editor, error-protection, export, external-http

[patopaiar]

Adds @covers to folders starting with g: general

[sephsekla]

Adds @covers to folders starting with L: link, load

[patopaiar]

Adds @covers to folders starting with h: hooks, http

[SergeyBiryukov]

In 49006:

Tests: Add missing @covers tags for files in phpunit/tests/functions/.

Props pbearne, jrf.
See #39265.

[pbearne]

Question: should we add a note if the test doesn't cover a WP function?

eg the feeds test looks at content in the feed, not a function :-)

And test_mysqli_flush_sync this test the SQL functions

PS

this the list of updates promised by the codeable team
Chase Gruszewski : rest-api
Paul Bearne : all the f,root
Hudson Hudson: All the As
Jonathan Bossenger: All the Bs
Tamara M: All the Ds - Es - Gs - Hs
Joe B-R: All the Ls

[pbearne]

all the files in the root of test

[pbearne]

Got the root files done now just few folder left

[jrf]

Fix newly introduced incorrect @covers tags. When global functions are covered, they need to be prefixed with :: (double colon) to distinguish them from class name. See: ​https://phpunit.readthedocs.io/en/7.0/annotations.html#covers

[jrf]

I've done an initial review of the submitted patches adding new covers tags for commit readiness.
I have NOT verified whether the added tags are correct. I trust this has been thoroughly fetted by the person who created the patch. I've only left a remark on those here and there when what I saw triggered an instance response.

Thank you all for stepping up and creating these patches! <3 This is a huge amount of work you've all done and getting these patches committed will be a great step forward!

Please find my feedback below. Line numbers I refer to are generally the "new" line number. File names as per the patch.

functions_folder.patch

@pbearne

• @covers syntax is correct.
• In a number of places superfluous empty docblocks lines are introduced at the start or end of a docblock. These need to be removed. Examples:
  • tests/phpunit/tests/functions/allowedProtocols.php line 19.
  • tests/phpunit/tests/functions/canonicalCharset.php all except the last one.
  • tests/phpunit/tests/functions/wpAuthCheck.php line 31.
• @covers tags need to be in docblock comments, i.e. the comment block needs to start with /**. In file tests/phpunit/tests/functions/wpListFilter.php, changes between line 42 and 162, this is not the case.

d_folders.patch

@patopaiar

• @covers syntax is correct.
• date/currentTime.php - indentation of the code on line 37 seems to have become borked.
• date/dateI18n.php - line 43 to 115 - need docblock format (see point 3 above) and alignment of the stars.
• date/dateI18n.php line 143, indentation of the comment starter is off.
• date/maybeDeclineDate.php - line 65 introduces a superfluous empty line.
• date/query.php :+1: on adding descriptions for the tests. The descriptions do still need a period . as "end of sentence" punctuation and most of these fixes all need docblock format + alignment of the stars.
• date/query.php line 310 + 313 introduce fixes to the actual test. This does not belong with this ticket and should be done in a separate patch, though the actual changes look good. :+1:
• date/query.php line 636, 647, 670, 768 - empty comment line at start of docblock needs to be removed.
• dependencies/scripts.php, line 684, 939 - something wonky is going on with the star indentation/alignment.

e_folders.patch

@patopaiar

• @covers syntax is correct.
• editor/wpEditors.php - these all need docblock format and the last one also removal of the stray empty line + alignment of the stars.
• error-protection/recovery-mode-cookie-service.php line 94, ReflectionMethod::invoke is a PHP native method and should not be included in the @covers tags.
• external-http/basic.php - needs docblock format, alignment of the stars and removal of a stray blank line above the new docblock (two empty lines at start of class instead of one).

g_folders.patch

@patopaiar

• This patch seems to miss the path to the files. This probably has to do with from which folder the patch was generated. Generally speaking it is always a good idea to generate patches from the project root so file references have the full relative path.
• @covers syntax is correct.
• document-title.php - these all need docblock format and alignment of the stars.
• paginateLinks.php - the new comment blockse all need docblock format.
• resourceHints.php, line 224, 239 - star alignment is off.
• template.php, line 418 introduces a stray blank line.

L_folders.patch

@sephsekla

• @covers syntax is correct.
• tests/phpunit/tests/link/adminUrl.php, tests/phpunit/tests/link/getAdjacentPost.php, tests/phpunit/tests/link/getAdjacentPostLink.php, tests/phpunit/tests/link/getDashboardUrl.php, tests/phpunit/tests/link/getPostCommentsFeedLink.php, tests/phpunit/tests/link/getPostTypeArchiveLink.php - if all tests cover the same function, the @covers tag at the class level is sufficient. Otherwise, the @covers tag at class level should be removed. Generally speaking, having @covers tags at test function level is better as it allows for more flexibility, so I'd recommend removing the one(s) at the class level.
• tests/phpunit/tests/link/getAdjacentPostLink.php, tests/phpunit/tests/link/getPostCommentsFeedLink.php, tests/phpunit/tests/link/getPreviewPostLink.php, tests/phpunit/tests/link/getPreviousCommentsLink.php - stray blank lines at the start of the docblocks need to be removed.

g_folders.patch

@patopaiar

• @covers syntax is correct.
• Same as with other patches, new comment blocks need to use docblock format and the stars need to be aligned (nearly all files).
• http/functions.php line 214 - star alignment is off.
• http/http.php line 218 - introduces a stray blank line.
• http/http.php line 274 - ReflectionClass::getConstants is a PHP native method and should not be included in the @covers tags.

root.patch

@pbearne

• This patch seems to contain unrelated changes to the package-lock.json file. Please remove these.
• @covers syntax is correct in nearly all cases.
• tests/phpunit/tests/actions.php, tests/phpunit/tests/auth.php, tests/phpunit/tests/avatar.php, tests/phpunit/tests/cache.php, tests/phpunit/tests/comment-submission.php, tests/phpunit/tests/comment.php etc - stray blank lines at the start of docblocks need to be removed.
• tests/phpunit/tests/adminbar.php, line 254 - 260 - the get_standard_admin_bar() method is not a test method, but a test helper. This should not have a @covers tag. Also: stray blank lines at the start of the docblock.
• tests/phpunit/tests/adminbar.php, line 373 - stray blank comment line between two @covers tags.
• tests/phpunit/tests/adminbar.php, line 750 - superfluous blank comment line (two instead of one).
• tests/phpunit/tests/auth.php, line 116 seems to change a test ?
• tests/phpunit/tests/auth.php, line 145-146 need to be removed (duplicate).
• tests/phpunit/tests/basic.php line 6 - there is a tag for that ;-) @coversNothing See: ​https://phpunit.readthedocs.io/en/7.0/annotations.html#coversnothing .
• tests/phpunit/tests/db.php line 238, 255, 793 introduce stray blank comment lines which should be removed.
• tests/phpunit/tests/db.php, line 1687 - missing :: before the covered function name.
• tests/phpunit/tests/db.php, line 1826 - 1828 seem incorrect.
• tests/phpunit/tests/filters.php, line 422 introduces a stray blank comment line.
• tests/phpunit/tests/link.php, line 131 seems to change a test ?
• tests/phpunit/tests/mail.php, line 454 - PHPMailer is not included in code coverage reporting, so this should probably be changed to @coversNothing.
• tests/phpunit/tests/media.php, line 522 seems to change a test and break it...
• tests/phpunit/tests/media.php, line 1541 seems to change a test and break it...
• tests/phpunit/tests/media.php, line 2658 removes the ticket number. This should be brought back or if the ticket number did not belong with the test, the whole line should be removed.
• tests/phpunit/tests/pluggable.php (both tests) - these look more like QA tests, than unit tests as they don't actually test the functioning of the functions. I think these tests would be better off being marked with @coversNothing.
• tests/phpunit/tests/rest-api.php, line 1021-1023 introduces a stray blank docblock.
• tests/phpunit/tests/user.php, line 201-202 - this method looks to test the magic __get(), __set(), __isset() and __unset() methods.
• tests/phpunit/tests/user.php, line 250, 265 - tests cannot cover properties. I believe both these tests are actually testing the __unset() method.
• tests/phpunit/tests/user.php, line 293, 307 - tests cannot cover properties.
• tests/phpunit/tests/widgets.php, line 45 seems to change a test and break it...

[SergeyBiryukov]

In 49305:

Tests: Correct newly introduced @covers tags.

When global functions are covered, they need to be prefixed with :: (double colon) to distinguish them from class name.

See ​https://phpunit.readthedocs.io/en/7.0/annotations.html#covers for more details.

Follow-up to [49000], [49171].

Props jrf.
See #39265.

[patopaiar]

@jrf I have made all the updates suggested in your notes - including creating a separate patch for the correction made to the unit test, and generating the patch with the missing path from the project root as suggested so that it does have the full file paths.

It's my first time contributing to core and I'm wondering what's the protocol to submit these revised versions of the patches?
Simply attach the updated versions here?
Should they keep their names or get new ones?
And lastly, should the patch with the corrected unit test be attached here on in a different ticket?

Thanks in advance for the guidance

[pbearne]

update functions folder

[sephsekla]

Adds @covers to folders starting with L: link, load (amended)

[pbearne]

updated root patch

[Hudson Atwell]

Adding @covers to Actions folder and Admin folder, Ajax and Attachment folders still need coverage. Hoping I did this well.

[Hudson Atwell]

After uploading actions+admin.diff I noticed everyone was submitting .patch files.

Since uploading, I've tried following the instructions in this article (​http://scribu.net/wordpress/svn-patches-from-git.html) to make the .diff file acceptable, by running this command: git diff --no-prefix > ~/actions+admin.diff.

If I run this command, will the .diff file be acceptable? Should I reupload the actions+admin.diff file after running this command?

[pbearne]

the formatting folder

[pbearne]

the L foders

[patopaiar]

Adds @covers to folders starting with o: oembed, option

[hellofromTonya]

Punting this ticket to 5.7 as today is 5.6 RC2.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[pbearne]

the M folders

[pbearne]

p folders

[SergeyBiryukov]

Replying to jrf:

I've done an initial review of the submitted patches adding new covers tags for commit readiness.

This is great, thank you!

functions_folder.patch

• @covers syntax is correct.
• In a number of places superfluous empty docblocks lines are introduced at the start or end of a docblock. These need to be removed. Examples:
  • tests/phpunit/tests/functions/allowedProtocols.php line 19.
  • tests/phpunit/tests/functions/canonicalCharset.php all except the last one.
  • tests/phpunit/tests/functions/wpAuthCheck.php line 31.
• @covers tags need to be in docblock comments, i.e. the comment block needs to start with /**. In file tests/phpunit/tests/functions/wpListFilter.php, changes between line 42 and 162, this is not the case.

Looks like this feedback was addressed in functions_folder.2.patch​, but it also seems that this patch is now redundant, as the phpunit/tests/functions/ folder was already addressed in [49006]. I've made a few adjustments in that commit to add @covers tags to the whole class instead of individual methods where appropriate, per this part of comment:7:

Also note: The tests don't all have to have individual @covers tags. If a test class, through a variety of tests, covers all aspect of one particular function, a single @covers tag for that function in the test class docblock will suffice. Same if a test class covers every aspect of a class. A single @covers tag referencing the covered class in the test class docblock is enough in that case.

@jrf Please let me know if [49006] looks correct or any follow-up changes are needed. Thanks!

Looking into other patches now :)

[SergeyBiryukov]

In 50276:

Tests: Correct the test for NOT BETWEEN comparison operator in WP_Date_Query.

Follow-up to [29793].

Props patopaiar, jrf.
See #39265, #51802.

[SergeyBiryukov]

In 50279:

Tests: Add missing @covers tags for files in phpunit/tests/date/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50286:

Tests: Add missing @covers tags for files in phpunit/tests/db/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50287:

Tests: Add missing @covers tags for files in phpunit/tests/dependencies/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50288:

Tests: Add missing @covers tags for files in phpunit/tests/editor/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50289:

Tests: Add missing @covers tags for files in phpunit/tests/error-protection/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50338:

Tests: Simplify @covers tags in editor/wpEditors.php using @coversDefaultClass annotation.

Follow-up to [50288].

See #39265.

[SergeyBiryukov]

In 50339:

Tests: Add missing @covers tags for files in phpunit/tests/general/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50341:

Tests: Add missing @covers tags for files in phpunit/tests/hooks/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50344:

Tests: Add missing @covers tags for files in phpunit/tests/http/.

Props patopaiar, jrf.
See #39265.

[SergeyBiryukov]

In 50454:

Tests: Add missing @covers tags for files in phpunit/tests/link/.

Props sephsekla, jrf.
See #39265.

[SergeyBiryukov]

In 50455:

Tests: Add missing @covers tags for files in phpunit/tests/load/.

Props sephsekla, jrf.
See #39265.

[jrf]

Fixes the syntax of some @covers tags

[jrf]

I've just uploaded a patch to fix some more @covers tags which are already in the code base, probably introduced some time over the last few months.

I've also done a review of the new patches which were uploaded, similar to my earlier review in https://core.trac.wordpress.org/ticket/39265#comment:24.

Thanks everyone for your perseverance and continued work on this ticket!

actions+admin.diff

@hudson-atwell

• Mostly looking good.
• .gitignore: that change does not belong in this patch. Excluded IDE specific files/directories should be done at the user level via .git/info/exclude, not at the project level.
• tests/phpunit/tests/admin/includesScreen.php: line 300 and below - the @covers tags refer to a non-existent class. The ​`get_current_screen()` method returns a WP_Screen object, so the method calls, like $screen->add_help_tab() call the method from the WP_Screen class. The @covers tags need to be fixed to reflect that, so @covers get_current_screen::add_help_tab should be: @covers WP_Screen::add_help_tab.
• tests/phpunit/tests/admin/includesTheme.php: line 179, same as above. The ​`wp_get_theme()` method retrieves a WP_Theme object, so the covers tag should be @covers WP_Theme::get_post_templates.
• tests/phpunit/tests/admin/includesListTable.php: this file seems to exist three times in the patch. The @covers class::WP_Posts_List_Table tags are incorrect, so the patches for this file will need to be combined to get the correct patch.

formatting.patch​

@pbearne

• @covers syntax is correct.
• tests/phpunit/tests/formatting/Autop.php: The @covers tags seem to refer to a non-existent function autop ? I think these should probably all be @covers ::wpautop. And if there are only tests for that one function in the file, adding just one @covers tag at the class docblock level instead of adding the tag to each individual method, is fine.
• tests/phpunit/tests/formatting/balanceTags.php (and most, though not all, of the other files too): similar to the previous comment - one @covers tag at the class docblock level would be sufficient here as all tests in this test class seem to be for the one WP function.
• tests/phpunit/tests/formatting/EscUrl.php, tests/phpunit/tests/formatting/EscXml.php - stray blank lines at the start of the docblocks need to be removed.
• tests/phpunit/tests/formatting/SanitizeTitleWithDashes.php: nice catch on the mistake in the test_strips_html() test.

I_folders.patch​

@pbearne

All looks good. No real remarks. Some tags could probably be done at class docblock level, but the patch will work as is.

o_folders.patch

@patopaiar

All looks good. There are some stray @uses tags which shouldn't really be needed, but don't do any harm either, so all good.

m-folders.patch

@pbearne

All looks good. Just one tiny nitpick:

• tests/phpunit/tests/multisite/networkQuery.php - indentation of the docblock is off round line 124

p-folders.patch

@pbearne

Mostly looking good. Just a few small remarks:

• tests/phpunit/tests/post/filtering.php: line 72 - blank line between docblock and function declaration which should be removed.
• tests/phpunit/tests/post/filtering.php: line 116 - stray blank docblock line which should be removed.
• tests/phpunit/tests/post/wpInsertPost.php: line 81 - I think the function call got accidentally removed here, which would break the test.
• tests/phpunit/tests/post/wpPublishPost.php: line 48, 68, 89, 124: there must be a blank docblock line between the function description and the first tag.

q-folder.patch

@pbearne

This patch needs a do-over.

The patch uses the @coversDefaultClass annotation at the class level, but there are no @covers tags for the individual tests. The @coversDefaultClass annotation is a way to prevent having to repeat the same class name over and over in the @covers tags used for the individual test methods in the test class, but should only be used in conjunction with @covers tags at the method level.
See the documentation for ​@coversDefaultClass.
I think @pbearne probably intended to use the @covers at class level for these tests and that would have been perfectly fine.

[pbearne]

fixed Q folder

[pbearne]

update formatting folder

[pbearne]

fixed formatting

[pbearne]

add this after post folder

[pbearne]

@jrf Add patches for comments
a couple of the patches are just the changed file(s) to will need to patch over the main folder

[pbearne]

do we have some you know the REST API to do the rest-api folder tests?

[SergeyBiryukov]

In 50537:

Tests: Correct some newly introduced @covers tags.

Follow-up to [50289], [50344].

Props jrf.
See #39265.

This a working branch and pull request for the adding of @covers to all the PHP test
This is not complete at this time
I have merged in all the SVN patches but I have to check that they are OK as I got errors in the merge

Trac ticket: #39265

This works as a better way to track the updates

the ongoing covers ticket
I am still rechecking all the files so expect up[date to the pull request

[pbearne]

all the test have covers apart from rest-api I need an API expert who understands which is the function that its been test via the call

Let's get merged!!!

​https://github.com/WordPress/wordpress-develop/pull/1255

@pbearne Awesome work!

Some questions:

• Have all remarks which I left on the previous PR #1221 been addressed ?
• Why are there changes to the grunt setup included in this PR ?
• Should the forceCoversAnnotation="true" attribute be added to the PHPUnit configuration files at this time ?

Note: there are two config files - the phpunit.xml.dist file in the project root and the multisite.xml file in the tests/phpunit directory. Both would need to be adjusted.

@pbearne Awesome work!

Some questions:

• Have all remarks which I left on the previous PR #1221 been addressed ?

Yes all done

• Why are there changes to the grunt setup included in this PR ?

Reverted not sure how they got in there

• Should the forceCoversAnnotation="true" attribute be added to the PHPUnit configuration files at this time ?

Would be nice I am trying to do the rest_api folder and will set the flag once done

Note: there are two config files - the phpunit.xml.dist file in the project root and the multisite.xml file in the tests/phpunit directory. Both would need to be adjusted.

Time for many eyes lets get this done

Paul

[desrosj]

Thanks for all of the work here, everyone! I am going to leave this in the 5.8 milestone past the beta 1 deadline (which is today). This only affects tests, so it can be committed during the beta cycle. There's not enough time before beta 1 for me to give this one more pre-commit review, but I'd like to get this in by end of week.

@SergeyBiryukov have you started reviewing yet? Happy to self assign, but don't want to double efforts.

Updated from master and accepted the code review changes
I totally expect that correction/change will be made to the covers overtime I really just what to 98%+ correct and all in :-)

[hellofromTonya]

Note for all:
​PR 1255 includes:

• all the remaining @covers except for the REST API (which needs discussion)
• adds forceCoversAnnotation="true" to the config

@SergeyBiryukov and @desrosj => This week I'm doing a deep dive code review of the PR. As each instance needs review to understand the intent of each test (i.e. what is the specific purpose of the test), it'll take some time and effort. Hoping to have it finished end of week.

Assigning ownership to me for this review process. Once done, I'll ping you both for final review and commit consideration.

[hellofromTonya]

In doing a deep and thorough review of PR 1255, it's huge with currently 570 files changed. Tackling one big commit will be too big of an ask of a core committer. To reduce the impact on core committers and thus get the changes committed, @pbearne and I discussed and decided to the following process:

• I'll do the thorough code review of a batch of files from PR 1255 which includes reasoning behind any changes
• If there are changes requested, Paul will update the branch
• Then one of us will create a "batch patch"
• Then the commit keyword will be added for a core committer to review and commit
• Once committed, the changes will merged into the PR's branch

The PR then is the source truth we'll use for tracking what remains. Batch-by-batch the PR will get thoroughly reviewed and committed.

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

Batch patch of thoroughly reviewed @covers changes from PR #1255 for the following tests:

• Tests_Actions => file tests/phpunit/tests/actions.php
• Tests_Actions_Callbacks => file tests/phpunit/tests/actions/callbacks.php
• Tests_Actions_Closures => file tests/phpunit/tests/actions/closures.php
• Tests_Admin_IncludesComments => file tests/phpunit/tests/admin/includesComment.php
• Test_WP_Community_Events => file tests/phpunit/tests/admin/includesCommunityEvents.php
• Tests_Admin_includesFile => file tests/phpunit/tests/admin/includesFile.php
• Tests_Admin_includesListTable => file tests/phpunit/tests/admin/includesListTable.php
• Tests_Admin_includesMisc => file tests/phpunit/tests/admin/includesMisc.php

See PR 1255 for the code review.

[hellofromTonya]

​PR 1388 is the first batch patch from ​PR 1255

Notes for core committer:

• Props: @pbearne who created PR 1255.
• I thoroughly reviewed these changes in PR 1255 ​here and ​here.
• Why batch? See comment 65.

Marking for commit.

[spacedmonkey]

Just a heads up, I have been supporting @pbearne with adding covers to REST API tests.

Here is the ​diff. These are the endpoints I have worked on. But the name of the methods maps to the method in the class pretty well, should be easy to do the rest.

[desrosj]

While I greatly appreciate all of the work going into this effort and the related PRs, there are simply too many other tickets that must be fixed and committed prior to 5.8 right now with time quickly running out. It's also become more clear that it's preferable to review and commit in batches that are still being created, and that the effort will stretch at least into 5.9.

With this in mind, I'm moving this to 5.8 to reflect that as much as possible, time should be spent on other tickets remaining in 5.8 for the time being.

If a committer has the time and wishes to focus on this between now and when 5.8 is branched, they're more than welcome to.

[hellofromTonya]

In 51702:

Tests: Add missing @covers tags for actions' tests.

Props pbearne, hudson-atwell, jrf, hellofromTonya.
See #39265.

[hellofromTonya]

In 51724:

Tests: Add missing @covers tags for Tests_Admin_IncludesComment.

Follow-up to [34456], [34460].

Props pbearne, jrf, hellofromTonya.
See #39265.

[hellofromTonya]

In 51725:

Tests: Add missing @covers tags for Tests_Admin_wpCommunityEvents.

Adds missing and moves placement of existing @covers tags for consistency.

Follow-up to [40607], [42726], [49145].

Props pbearne, jrf, hellofromTonya.
See #39265.

[hellofromTonya]

In 51852:

Build/Test Tools: Splits and improves compat tests.

Splits the tests in the tests/phpunit/tests/compat.php file up into individual test classes for each function being tested.

Improvements to individual test cases:

• Adds @covers tags.
• Adds visibility modifiers to all methods.
• Adds function availability test.
• Where relevant, fixes the assertion parameter order.
• Data provider:
  • Where relevant, reworks a test to use a data provider.
  • Where relevant, renames data provider methods to have a more obvious link to the test it applies to.
  • Makes the data provider more readable by adding keys within the data sets.
  • Moves the data provider below its associated tests.
  • Adds/removes data sets in data providers.
• Makes the actual test code more readable by using descriptive variables and multi-line function calls.
• Adds the $message parameter to all assertions when a test method contains more than one assertion.

Specifically for the _mb_substr() tests:

• Splits the test_mb_substr_phpcore() method into two test methods based on the PHP Core test files they are emulating.
• Makes the actual test code within the test_mb_substr_phpcore_basic() method more readable by using descriptive variables and multi-line function calls.
• Splits the data used for the second part of the test_mb_substr_phpcore() function, now test_mb_substr_phpcore_input_type_handling(), off into a separate data provider with named data sets.
• Removes duplicate data sets from the data_mb_substr_phpcore_input_type_handling().
  • Why? The PHP native tests test against upper/lowercase false, true, null and some other text string single quote/double quote variations. As things were, those differentiations had been undone when the coding standards were put in place, so in effect those weren't being tested anymore. And as this is userland code, there's no point in adding these differentiations back as they will be handled the same by PHP anyway (and that is safeguarded via the PHP native tests).
• Removes the "undefined variable" and "unset variable" test cases as, while those are relevant to the C code in which PHP is written, they are not relevant for testing userland code and will behave the same as the test passing null.

Follow-to [25002], [32364], [42228], [42343], [43034], [43036], [43220], [43571], [45607], [47122], [47198], [48937], [48996], [51415], [51563], [51594].

Props jrf, hellofromTonya.
See #39265, #53363.

[hellofromTonya]

In 51992:

Build/Test Tools: Add missing @covers tags for Tests_Admin_includesFile.

Follow-up to [25002], [42773].

Props pbearne, jrf, hellofromTonya.
See #39265.

[hellofromTonya]

In 51993:

Build/Test Tools: Add missing @covers tags for Tests_Admin_includesListTable.

Follow-up to [31730].

Props pbearne, jrf, hellofromTonya.
See #39265.

[hellofromTonya]

In 51994:

Build/Test Tools: Add missing @covers and visibility for Tests_Admin_includesMisc.

Adds the @covers tag to the test DocBlock.
Adds missing public visibility to the test method.

Follow-up to [25002].

Props pbearne, jrf, hellofromTonya.
See #39265.

Committed via changesets:
https://core.trac.wordpress.org/changeset/51702
https://core.trac.wordpress.org/changeset/51724
https://core.trac.wordpress.org/changeset/51725
https://core.trac.wordpress.org/changeset/51992
https://core.trac.wordpress.org/changeset/51993
https://core.trac.wordpress.org/changeset/51994

[hellofromTonya]

Removing the commit keyword as reviewed patches have been committed.

[SergeyBiryukov]

In 51998:

Tests: Correct @covers tags in WP_Comments_List_Table tests.

Follow-up to [51993], [51997].

See #39265.

[hellofromTonya]

5.9 is in Feature Freeze and focus is on defects. With less than a week until Beta 1, there's likely not time remaining in the 5.9 cycle to focus on reviewing and committing the remaining covers in the PR.

As tests can be committed at anytime into trunk, if someone has the time to get review and get this patch committed, please do so. Else, let's continue working on these annotations in 6.0.

[kirasong]

Wow, there's been some great work happening in this ticket!
We came across it during a ​6.0 bug scrub today.

What's left here, and what would be the best way for folks to help out?

[hellofromTonya]

With 6.0 RC1 tomorrow and work still ongoing in this ticket, moving it to 6.1.

[azaozz]

In 53561:

Build/Test Tools: Add missing @covers tags for the AJAX test group.

Props pbeane, hellofromTonya, antonvlasenko, ironprogrammer, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53562:

Build/Test Tools, Formatting group:

• Add and update @covers tags.
• Add and improve docs and inline comments.

Props pbeane, hellofromTonya, antonvlasenko, ironprogrammer, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53564:

Build/Test Tools: Add missing @covers tags and fix the docs for the Cron test group.

Props pbeane, hellofromTonya, antonvlasenko, ironprogrammer, SergeyBiryukov, costdev.
See #39265.

[SergeyBiryukov]

In 53565:

Tests: Correct the expected result for wp_ajax_replyto_comment() test with a draft post.

The "Error:" prefix was previously removed and accidentally re-added with @covers tags.

Follow-up to [53337], [53561].

See #39265.

[SergeyBiryukov]

In 53571:

Tests: Put @covers tags before @params in Ajax and Formatting groups.

This makes the placement more consistent with the rest of the test suite.

Follow-up to [53561], [53562].

See #39265.

[SergeyBiryukov]

In 53574:

Tests: Replace esc_url_raw() calls with sanitize_url().

Previously committed in [53455], appears to be accidentally reverted in [53562].

Follow-up to [51597], [53452], [53455], [53562].

See #39265, #55652.

[azaozz]

In 53680:

Build/Test Tools: Add and fix @covers tags to the attachments and block-supports test groups.

Props pbeane, hellofromTonya, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53681:

Tests: Remove duplicate testcase test_get_privacy_policy_url_should_return_empty_when_privacy_policy_page_not_set() from Tests_Url_GetPrivacyPolicyUrl.

See: #39265.

[azaozz]

In 53682:

Build/Test Tools: Add @covers tags to the url test group.

Props pbeane, hellofromTonya, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53683:

Build/Test Tools: Add @coversNothing tags to the tests for some content in documentation and settings files.

Props pbeane, hellofromTonya, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53684:

Build/Test Tools: Add @covers tags to the taxonomy and category test groups.

Props pbeane, hellofromTonya, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[SergeyBiryukov]

In 53686:

Tests: Separate the tests in basic.php for clarity.

There were two kinds of tests in this file:

• Tests for content of some files in the root directory:
  • license.txt
  • SECURITY.md
  • package.json
• Tests for some utility functions of the test framework itself:
  • strip_ws()
  • test_mask_input_value()

The latter are now moved to their own file, utils.php.

Follow-up to [22/tests], [81/tests], [103/tests], [25240], [26940], [28064], [28480], [28493], [28523], [28631], [42381], [47403], [53683].

See #39265, #55652.

[SergeyBiryukov]

In 53687:

Tests: Remove @uses tags from the test suite.

• These were not used consistently, with only four instances across all the tests.
• Using this tag in combination with the beStrictAboutCoversAnnotation="true" setting will mark a test as "Risky" if code is executed during the test which is not annotated via @covers or @uses tags. That would make the maintainance of the tags very fiddly, while adding little additional value for the test code base.

Follow-up to [32995], [39914], [42636], [53682].

Props jrf.
See #39265.

[azaozz]

In 53863:

Build/Test Tools: Add @covers tags to the comments tests.

Props pbearne, jrf, hellofromTonya, patopaiar, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53864:

Build/Test Tools: Add @covers tags to the import tests.

Props pbearne, jrf, hellofromTonya, patopaiar, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53865:

Build/Test Tools: Add @covers tags to the options tests.

Props pbearne, jrf, hellofromTonya, patopaiar, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53866:

Build/Test Tools: Add @covers tags to the l10n and i18n tests.

Props pbearne, jrf, hellofromTonya, patopaiar, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[azaozz]

In 53867:

Build/Test Tools: Update @covers tags for the load tests.

Props pbearne, jrf, hellofromTonya, patopaiar, ironprogrammer, antonvlasenko, SergeyBiryukov, costdev.
See #39265.

[SergeyBiryukov]

In 54049:

Tests: Remove @covers tags for native PHP functions in phpunit/tests/compat/.

As these are not user-defined functions, they cause notices when generating the code coverage report:

"@covers ::array_key_first" is invalid
"@covers ::array_key_last" is invalid
"@covers ::hash_hmac" is invalid
"@covers ::is_countable" is invalid
"@covers ::is_iterable" is invalid
"@covers ::mb_strlen" is invalid
"@covers ::mb_substr" is invalid
"@covers ::str_contains" is invalid
"@covers ::str_ends_with" is invalid
"@covers ::str_starts_with" is invalid

Follow-up to [51852], [52038], [52039], [52040].

See #39265, #55652.

[SergeyBiryukov]

In 54051:

Tests: Correct some @covers tags in wp_html_split() and wptexturize() tests.

As preg_split() is not a user-defined function, it causes notices when generating the code coverage report:

"@covers ::preg_split" is invalid

Instead, it appears that the intention was to test the performance of these WordPress functions:

• get_html_split_regex()
• _get_wptexturize_split_regex()
• _get_wptexturize_shortcode_regex()

Follow-up to [34761], [53562].

See #39265, #55652.

[SergeyBiryukov]

In 54060:

Tests: Add @coversNothing tag for PHP polyfill tests in phpunit/tests/compat/.

The @covers tags for these tests were previously removed to avoid notices when generating the code coverage report on PHP versions where these functions are natively available and not user-defined:

"@covers ::array_key_first" is invalid
"@covers ::array_key_last" is invalid
"@covers ::hash_hmac" is invalid
"@covers ::is_countable" is invalid
"@covers ::is_iterable" is invalid
"@covers ::mb_strlen" is invalid
"@covers ::mb_substr" is invalid
"@covers ::str_contains" is invalid
"@covers ::str_ends_with" is invalid
"@covers ::str_starts_with" is invalid

Explicitly including a @coversNothing annotation in this case appears to be a more appropriate option than not including any annotation at all.

Follow-up to [51852], [52038], [52039], [52040], [54049].

See #39265, #55652.

[SergeyBiryukov]

In 54064:

Tests: Restore @covers tags for PHP polyfill tests in phpunit/tests/compat/.

These tags were previously removed to avoid notices when generating the code coverage report on PHP versions where these functions are natively available and not user-defined:

"@covers ::array_key_first" is invalid
"@covers ::array_key_last" is invalid
"@covers ::hash_hmac" is invalid
"@covers ::is_countable" is invalid
"@covers ::is_iterable" is invalid
"@covers ::mb_strlen" is invalid
"@covers ::mb_substr" is invalid
"@covers ::str_contains" is invalid
"@covers ::str_ends_with" is invalid
"@covers ::str_starts_with" is invalid

It has been pointed out that those tests do cover the WP implementation of those functions and should be marked as such with a @covers tag. The reason PHPUnit displays notices about it, is that code coverage is only run on PHP 7.4 instead of multiple PHP versions.

For those PHP versions which don't natively contain the function, the WP polyfill is being tested and should be seen as covered by tests. The reason the tests are also run on PHP versions in which the function already exists in PHP natively, is to make sure that the polyfill test expectations line up with the PHP native behaviour, even though at that point, they are no longer testing the WP polyfill, but the PHP native function.

With the above in mind, while those PHPUnit notices add some noise to the code coverage report, in this case, they should be ignored and the @covers tags should be brought back.

As a potential future enhancement, the code coverage script could be updated to run against the highest and lowest supported PHP versions and with some variations of extensions enabled or disabled to ensure those tests actually test the polyfills.

Follow-up to [51852], [52038], [52039], [52040], [54049], [54060].

Props jrf.
See #39265, #55652.

[chaion07]

@pbearne thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received here's a summary:

1. Most of the test code does not use @cover or @uses notation yet.
2. The patch needs testing.
3. Adding needs-testing and removing has-unit-tests keyword.

Cheers!

Props: @priyomukul

[hellofromTonya]

This ticket has been a rolling blessed task over many major releases. In the spirit of ongoing improvement work (similar to #55646, #55647, etc.), I'm closing this ticket for the current cycle. I've opened a new ticket #56782 for the ongoing work through the next cycle, ie 6.2 cycle.

Thank you everyone for your contributions to improving coverage reporting!

@pbearne Can I recommend breaking this into smaller PR. Or at least classes should be in new PR. This PR is really hard to review and harder to commit / revert.