Make WordPress Core

Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#57099 closed enhancement (fixed)

Fix Test Class Header Tags

Reported by: wojtekn's profile wojtekn Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: minor Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

The class header of the Tests_Functions_DoEnclose class contains duplicate information (check @since).

As it was proposed by antonvlasenko, it makes sense to merge these two docblocks into one and remove the duplicate tags (i.e., @since).

Why?

  • Having two docblocks doesn’t make sense to me if a file contains only 1 test class. However, it can be justified if a file contains multiple test classes (which is not the case here).
  • Out of 804 PHP files in the tests/phpunit folder, 497 files have 1 docblock, and 307 files have both file and class docblocks. So, having a single docblock per test class/file is more common.

Attachments (1)

57099.diff (9.5 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (22)

This ticket was mentioned in PR #3614 on WordPress/wordpress-develop by @wojtekn.


2 years ago
#1

  • Keywords has-patch has-unit-tests added

In this PR, I propose merging file-level docblock with class-level docblock to avoid repeating information.

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

@SergeyBiryukov
2 years ago

#2 follow-ups: @SergeyBiryukov
2 years ago

  • Component changed from General to Build/Test Tools
  • Milestone changed from Awaiting Review to 6.2
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Hi there, welcome to WordPress Trac! Thanks for the ticket.

It looks like the file-level DocBlock was created per the File Headers section of the documentation standards:

The file header DocBlock is used to give an overview of what is contained in the file.

Whenever possible, all WordPress files should contain a header DocBlock, regardless of the file’s contents – this includes files containing classes.

Looking at the WordPress core, most class files do indeed contain both the file-level DocBlock and the class DocBlock.

That said, I agree it makes less sense for unit test classes if not applied consistently, and the duplicate tags look a bit confusing.

If we do want to merge these DocBlocks in unit tests, 57099.diff includes a few more files where this can be applied.

Related changesets: [43183], [43291], [43292], [43499], [43568], [44502], [44535], [44628], [44786], [44824], [44906], [44909], [46175].

#3 in reply to: ↑ 2 @arafatjamil01
2 years ago

I have checked the files, and everything looks nice. The current comments make more sense.

#4 @wojtekn
23 months ago

@SergeyBiryukov thanks, I've applied this patch and updated the PR.

#5 in reply to: ↑ 2 @hztyfoon
22 months ago

Hey there, wojtekn 🤗
welcome to WordPress Trac! Thanks for the ticket.

Though the file-level DocBlock was probably created as per this as sergey pointed out here:

It looks like the file-level DocBlock was created per the File Headers section of the documentation standards:

The file header DocBlock is used to give an overview of what is contained in the file.

Whenever possible, all WordPress files should contain a header DocBlock, regardless of the file’s contents – this includes files containing classes.

But, I totally agree with sergey on the duplicate tags looking confusing.
Also this standard is not consistently used with all unit test classes.

That said, I agree it makes less sense for unit test classes if not applied consistently, and the duplicate tags look a bit confusing.

So,
Looks to me, this ticket is good to move forward with the proposed changes in 57099.diff which seems to be also applied in the PR now.

@hztyfoon commented on PR #3614:


22 months ago
#6

Hello @wojtekn , Your PR looks good.
Looks to me, this 👇 one can also be included with the changes.

https://github.com/WordPress/wordpress-develop/blob/0cb8475c0d07d23893b1d73d755eda5f12024585/tests/phpunit/tests/admin/wpCommunityEvents.php#L2-L18

@wojtekn commented on PR #3614:


22 months ago
#7

Thanks, @hz-tyfoon!. I've included this change in the PR.

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


21 months ago

#9 @mukesh27
21 months ago

This ticket was discussed in the recent bug scrub.

@SergeyBiryukov do you have capacity to review attached PR?

Props to @costdev

#10 @jamilbd07
21 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/3614.diff

Environment

  • OS: macOS 13.1
  • Web Server: nginx/1.23.3
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Browser: Chrome 109
  • Theme: Twenty Twenty-Three
  • Active Plugins:
    • No plugins active

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

  • Applied the patch and found fixed in 16 files. However, Me and @robinwpdeveloper checked together and found few more files where two docblocks exists. I think a new ticket can create for other double docblocks fix.

#11 @robinwpdeveloper
21 months ago

This ticket addresses doc block changes only.
I have searched with * @since and found few more files are missing the change in the header (before class).

List:
wordpress-develop/tests/phpunit/tests/blocks/context.php
wordpress-develop/tests/phpunit/tests/blocks/editor.php
wordpress-develop/tests/phpunit/tests/blocks/wpBlock.php
wordpress-develop/tests/phpunit/tests/blocks/wpBlockList.php
wordpress-develop/tests/phpunit/tests/blocks/wpBlockParser.php
wordpress-develop/tests/phpunit/tests/blocks/wpBlockType.php
wordpress-develop/tests/phpunit/tests/blocks/wpBlockTypeRegistry.php
wordpress-develop/tests/phpunit/tests/comment/isAvatarCommentType.php
wordpress-develop/tests/phpunit/tests/feed/wpSimplePieFile.php
wordpress-develop/tests/phpunit/tests/functions/anonymization.php
wordpress-develop/tests/phpunit/tests/link/getThePrivacyPolicyLink.php
wordpress-develop/tests/phpunit/tests/load/wpDebugMode.php
wordpress-develop/tests/phpunit/tests/rest-api/rest-block-renderer-controller.php
wordpress-develop/tests/phpunit/tests/rest-api/rest-block-type-controller.php
wordpress-develop/tests/phpunit/tests/rest-api/rest-blocks-controller.php
wordpress-develop/tests/phpunit/tests/rest-api/rest-widget-types-controller.php
wordpress-develop/tests/phpunit/tests/rest-api/rest-widgets-controller.php
wordpress-develop/tests/phpunit/tests/rest-api/wpRestBlockPatternCategoriesController.php
wordpress-develop/tests/phpunit/tests/rest-api/wpRestBlockPatternsController.php
wordpress-develop/tests/phpunit/tests/rest-api/wpRestEditSiteExportController.php
wordpress-develop/tests/phpunit/tests/rest-api/wpRestUrlDetailsController.php
wordpress-develop/tests/phpunit/tests/url/getPrivacyPolicyUrl.php
wordpress-develop/tests/phpunit/tests/user/retrievePassword.php

#12 @robinwpdeveloper
21 months ago

Since the occurrence number is high (already fixed in 16 files) we can go with it and create separate ticket for the remaining fixes if @SergeyBiryukov wants.

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


21 months ago

#14 follow-up: @costdev
21 months ago

  • Keywords commit added

This ticket was discussed during the bug scrub. I'm adding for commit consideration for the completed work and for thoughts on whether the remaining files can be addressed in this ticket later in the cycle, or in a separate ticket in a future release.

Additional props: @mukesh27

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


21 months ago

#16 @costdev
21 months ago

  • Type changed from enhancement to task (blessed)

#17 @costdev
21 months ago

  • Type changed from task (blessed) to enhancement

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


21 months ago

#19 @SergeyBiryukov
21 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55337:

Tests: Merge file-level and class-level DocBlocks in various unit test files.

Per the documentation standards, whenever possible, all WordPress files should contain a header DocBlock, regardless of the file’s contents – this includes files containing classes.

However, this recommendation makes less sense for unit test classes if not applied consistently, and the duplicate tags cause some confusion.

This commit aims to reduce confusion and avoid repeating information by combining the DocBlocks.

Follow-up to [40607], [43183], [43291], [43292], [43499], [43568], [44502], [44535], [44628], [44786], [44824], [44906], [44909], [46175].

Props wojtekn, antonvlasenko, arafatjamil01, hztyfoon, mukesh27, costdev, jamilbd07, robinwpdeveloper, SergeyBiryukov.
Fixes #57099.

@SergeyBiryukov commented on PR #3614:


21 months ago
#20

Thanks for the PR! Merged in r55337.

#21 in reply to: ↑ 14 @fuadragib
21 months ago

Replying to costdev:

This ticket was discussed during the bug scrub. I'm adding for commit consideration for the completed work and for thoughts on whether the remaining files can be addressed in this ticket later in the cycle, or in a separate ticket in a future release.

Additional props: @mukesh27

I created this ticket to address the remaining files mentioned by @robinwpdeveloper. Feel free to check it.

Note: See TracTickets for help on using tickets.