#57099 closed enhancement (fixed)
Fix Test Class Header Tags
Reported by: | wojtekn | Owned by: | 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)
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
#2
follow-ups:
↓ 3
↓ 5
@
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
@
2 years ago
I have checked the files, and everything looks nice. The current comments make more sense.
#5
in reply to:
↑ 2
@
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.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
21 months ago
#9
@
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
@
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
@
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
@
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:
↓ 21
@
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
This ticket was mentioned in Slack in #core by costdev. View the logs.
21 months ago
@SergeyBiryukov commented on PR #3614:
21 months ago
#20
Thanks for the PR! Merged in r55337.
#21
in reply to:
↑ 14
@
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.
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