[Font Library] Update PHPUnit tests per Core coding standards and practices #58502

hellofromtonya · 2024-01-31T13:34:21Z

What?

Updates the Font Library's PHPUnit tests to comply with the WordPress Core Test coding standards and practices.

Why?

To prepare the test suite for Core sync / merge, as these changes will get flagged in review. By fixing them now, it can help to reduce the review time and change requests at Core merge time.

How?

Test class naming. Should follow this format:
- For a test covering the entire class: Tests_[GroupName]_[ClassName].
- For a test covering a class' method: Tests_[GroupName]_[ClassName]_[MethodName].
- For a function: Tests_[GroupName]_[FunctionName].
Test method naming.
Usage of TestCase fixture methods including the setup of factory instances.
Usage and structure of data provider for repetitive tests.
Hook tests.
Usage and structure of annotations.
Presence of assertion message for each assertion within a test method, when there's more than one assertion.
Replacement of assertEquals() with assertSame() (see Trac ticket 59655 for the ongoing effort in 6.5).
General formatting and documentation that typically gets flagged in Core merge review.

Testing Instructions

All PHPUnit tests and test code linting checks should still pass.

github-actions · 2024-01-31T14:39:22Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/tests/fonts/font-library/fontFamilyBackwardsCompatibility.php
❔ phpunit/tests/fonts/font-library/fontLibraryHooks.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/loadFromJson.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getMimeTypes.php
❔ phpunit/tests/fonts/font-library/wpFontUtils/formatFontFamily.php
❔ phpunit/tests/fonts/font-library/wpFontUtils/getFontFaceSlug.php
❔ phpunit/tests/fonts/font-library/wpRestFontCollectionsController.php
❔ phpunit/tests/fonts/font-library/wpRestFontFacesController.php
❔ phpunit/tests/fonts/font-library/wpRestFontFamiliesController.php
❔ phpunit/tests/fonts/font-library/wpFontsDir.php

Renamed test classes per WordPress Core's Test Classes coding standards: For a test covering the entire class: Tests_[GroupName]_[ClassName] For a test covering a class' method: Tests_[GroupName]_[ClassName]_[MethodName] For a function: Tests_[GroupName]_[FunctionName] See https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/#test-classes.

Test classes should not have a constructor. Instead, use the set_up_before_class() fixture method to perform tasks that pre-setup all tests within the test class.

This is an ongoing effort in Core. Reasoning is shared in the 6.5 Trac ticket: >The assertEquals() test method does not check that the types >of the expected and actual values match. This can hide subtle >bugs especially when the values are falsey. https://core.trac.wordpress.org/ticket/59655

Per the WordPress Core coding standards, test files should be named according to the function, class, or method under test within its test class. Function under test: wpFunctioName.php Class under test: wpClassName.php Method under test: wpClassName/methodName.php See https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/#naming-and-organization.

github-actions · 2024-02-01T20:42:52Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hellofromtonya <hellofromtonya@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: anton-vlasenko <antonvlasenko@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: ironprogrammer <ironprogrammer@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Updates: * Refactors repetitive tests into a dataProvider. * Data provider's docblock. * Adds a brief explanation to each dataset (helps for understanding when a dataset fails). * Adds each argument name with a dataset. * Documents each argument in the test method's docblock.

When there are more than 1 assertion in a test method, a failure message helps to identify which assertion failed. Once an assertion fails, any other assertions after it in the same test method will not run. Thus, the message helps to know which one of the assertions failed. This can help with debugging process.

Resets are things like deleting posts, removing a filter/action, etc. These need to occur _before_ an assertion. Why? If a reset is after an assertion and any of the assertions before it fail, the test method bails out and the reset does not happen. To ensure resets happen, they need to be located in either: * tear_down() fixture. * just after invoking the thing under test but before the assertion(s).

Per the PHPUnit handbook: assertSomething( expected, actual, message );

@group

All of the test classes now have these groups: * @group fonts * @group font-library These annotations allow for specific groups of tests to be run, which is helping in local development.

hellofromtonya · 2024-02-01T21:34:52Z

~~All tasks are now done. Waiting for all CI jobs to be happy. If any fail, I'll likely force push to keep the atomic commits. Please wait to review or contribute only all the CI jobs are 🟢~~

update: it's ready for review.

hellofromtonya · 2024-02-01T21:43:49Z

All PHP linters and unit tests are green 🟢 This PR is ready for ready.

creativecoder

Found a handful of minor nits (see comments), otherwise this looks good to me, tests are passing, and the reasoning in each commit is very clear. Thank you!

phpunit/tests/fonts/font-library/fontLibraryHooks.php

phpunit/tests/fonts/font-library/wpRestFontFamiliesController.php

* Add endstop . for consistency. * Single form of "exist". Props creativecoder.

creativecoder

Nitpicks have been corrected, this look ready to me.

getdave · 2024-02-02T09:02:59Z

Nice work folks 👏

anton-vlasenko · 2024-02-02T10:17:37Z

phpunit/tests/fonts/font-library/fontFamilyBackwardsCompatibility.php

 */
 class Tests_Font_Family_Backwards_Compatibility extends WP_UnitTestCase {
+	private $post_ids_to_delete = array();


I can say that this is a redundant initialization of the post_ids_to_delete property since it gets initialized in the set_up() method anyway.
However, this could also be considered good coding practice to always initialize variables before using them.
Nonetheless, this falls into the category of an ultra-nitpick and personal preference; in my opinion, it doesn't justify creating a new pull request to address it. The rest of the code looks good to me. Great work.

Note that the backwards compatibility code for Font Families shouldn't go into Core. This is only to handle the refactor in the latest Gutenberg release, to keep installed fonts working for sites that were already using the Font Library feature from Gutenberg prior to 17.6.

I should have added a @core-merge comment in this test, as well, sorry about that.

In that case should we break this test into its own file?

The top of this file includes the @core-merge "do not merge" comment referenced above, so I believe this can be marked resolved?

Yes, I think you're right. As long as gutenberg_convert_legacy_font_family_format continues to run for anyone who may have installed fonts previous to Gutenberg 17.6, this should be fine--and it looks like the function will run from this file without issue.

I also think it makes sense to delete this code after WP 6.5 is released--anticipating that almost all who were using the font library before would have upgraded Gutenberg and no longer need the legacy wp_font_family migration.

hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 463307f to 7d2d826 Compare January 31, 2024 14:36

hellofromtonya added [Type] Code Quality Issues or PRs that relate to code quality [Feature] Font Library [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jan 31, 2024

WordPress deleted a comment from github-actions bot Jan 31, 2024

hellofromtonya removed the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jan 31, 2024

anton-vlasenko self-requested a review January 31, 2024 18:19

hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch 3 times, most recently from 77e0b22 to 4317550 Compare February 1, 2024 15:48

hellofromtonya added 5 commits February 1, 2024 09:49

Update covers tag

cdffdc8

Use set_up_before_class() instead of __construct().

5507b12

Test classes should not have a constructor. Instead, use the set_up_before_class() fixture method to perform tasks that pre-setup all tests within the test class.

hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 4317550 to 1c9f8fa Compare February 1, 2024 15:49

creativecoder mentioned this pull request Feb 1, 2024

Font Library: Add sanitize from schema util #58571

Merged

hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 1c9f8fa to d7bacf4 Compare February 1, 2024 20:42

hellofromtonya added 2 commits February 1, 2024 15:17

hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from d7bacf4 to 7226ae8 Compare February 1, 2024 21:18

hellofromtonya added 3 commits February 1, 2024 15:32

Put expected as first arg in each assert.

4fe255e

Per the PHPUnit handbook: assertSomething( expected, actual, message );

Add fonts and font-library group annotation.

d002d7b

All of the test classes now have these groups: * @group fonts * @group font-library These annotations allow for specific groups of tests to be run, which is helping in local development.

hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 7226ae8 to d002d7b Compare February 1, 2024 21:32

hellofromtonya marked this pull request as ready for review February 1, 2024 21:33

creativecoder reviewed Feb 1, 2024

View reviewed changes

Messages: add endstop; single "exist"

022869a

* Add endstop . for consistency. * Single form of "exist". Props creativecoder.

creativecoder approved these changes Feb 1, 2024

View reviewed changes

hellofromtonya enabled auto-merge (squash) February 1, 2024 23:18

hellofromtonya merged commit 4177183 into trunk Feb 1, 2024
61 of 62 checks passed

hellofromtonya deleted the fix/font-family-tests-per-core-coding-standards branch February 1, 2024 23:43

github-actions bot added this to the Gutenberg 17.7 milestone Feb 1, 2024

anton-vlasenko reviewed Feb 2, 2024

View reviewed changes

getdave mentioned this pull request Feb 2, 2024

Font Library: WordPress 6.5 Required Tasks #55277

Closed

25 tasks

anton-vlasenko mentioned this pull request Feb 2, 2024

Fix font library unit tests nit-picks [round 2] #58612

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Font Library] Update PHPUnit tests per Core coding standards and practices #58502

[Font Library] Update PHPUnit tests per Core coding standards and practices #58502

hellofromtonya commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

hellofromtonya commented Feb 1, 2024 •

edited

Loading

hellofromtonya commented Feb 1, 2024

creativecoder left a comment

creativecoder left a comment

getdave commented Feb 2, 2024

anton-vlasenko Feb 2, 2024 •

edited

Loading

creativecoder Feb 2, 2024

getdave Feb 2, 2024

ironprogrammer Feb 21, 2024

creativecoder Feb 22, 2024

[Font Library] Update PHPUnit tests per Core coding standards and practices #58502

[Font Library] Update PHPUnit tests per Core coding standards and practices #58502

Conversation

hellofromtonya commented Jan 31, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jan 31, 2024 • edited Loading

github-actions bot commented Feb 1, 2024 • edited Loading

hellofromtonya commented Feb 1, 2024 • edited Loading

hellofromtonya commented Feb 1, 2024

creativecoder left a comment

Choose a reason for hiding this comment

creativecoder left a comment

Choose a reason for hiding this comment

getdave commented Feb 2, 2024

anton-vlasenko Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

creativecoder Feb 2, 2024

Choose a reason for hiding this comment

getdave Feb 2, 2024

Choose a reason for hiding this comment

ironprogrammer Feb 21, 2024

Choose a reason for hiding this comment

creativecoder Feb 22, 2024

Choose a reason for hiding this comment

hellofromtonya commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

hellofromtonya commented Feb 1, 2024 •

edited

Loading

anton-vlasenko Feb 2, 2024 •

edited

Loading