Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Feb 1, 2024

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Jan 31, 2024

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?

Testing Instructions

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

@hellofromtonya hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 463307f to 7d2d826 Compare January 31, 2024 14:36
@hellofromtonya 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 WordPress deleted a comment from github-actions bot Jan 31, 2024
@WordPress WordPress deleted a comment from github-actions bot Jan 31, 2024
Copy link

github-actions bot commented Jan 31, 2024

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

@hellofromtonya 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
@hellofromtonya 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
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.
@hellofromtonya hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 4317550 to 1c9f8fa Compare February 1, 2024 15:49
@hellofromtonya hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 1c9f8fa to d7bacf4 Compare February 1, 2024 20:42
Copy link

github-actions bot commented Feb 1, 2024

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.
@hellofromtonya hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from d7bacf4 to 7226ae8 Compare February 1, 2024 21:18
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 );
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 hellofromtonya force-pushed the fix/font-family-tests-per-core-coding-standards branch from 7226ae8 to d002d7b Compare February 1, 2024 21:32
@hellofromtonya hellofromtonya marked this pull request as ready for review February 1, 2024 21:33
@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Feb 1, 2024

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
Copy link
Contributor Author

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

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

Props creativecoder.
Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicks have been corrected, this look ready to me.

@hellofromtonya hellofromtonya enabled auto-merge (squash) February 1, 2024 23:18
@hellofromtonya hellofromtonya merged commit 4177183 into trunk Feb 1, 2024
61 of 62 checks passed
@hellofromtonya hellofromtonya deleted the fix/font-family-tests-per-core-coding-standards branch February 1, 2024 23:43
@github-actions github-actions bot added this to the Gutenberg 17.7 milestone Feb 1, 2024
@getdave
Copy link
Contributor

getdave commented Feb 2, 2024

Nice work folks 👏

*/
class Tests_Font_Family_Backwards_Compatibility extends WP_UnitTestCase {
private $post_ids_to_delete = array();
Copy link
Contributor

@anton-vlasenko anton-vlasenko Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants