-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fonts Library: Test improvements #53702
Fonts Library: Test improvements #53702
Conversation
Splits the tests into 1 test file per public method.
17bdf16
to
b7ad62f
Compare
For tracking purposes, marking this PR as part of the fonts management feature that will be merged into Core for WP 6.4. |
b7ad62f
to
bd91b81
Compare
Hey @costdev, as a follow-up to our discussion of the Font Library PR, this PR splits and improves its tests, i.e. 1 test file per public method. It's following the work you and I have been doing in Core. What do you think? Have time for a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping! 🙂 Just a few of suggestions and a question about possible additional tests in future.
phpunit/fonts-library/wpFonFamilyUtils/getFilenameFromFontFace-test.php
Outdated
Show resolved
Hide resolved
phpunit/fonts-library/wpFonFamilyUtils/getFilenameFromFontFace-test.php
Outdated
Show resolved
Hide resolved
'txt' => array( '/temp/test.txt' ), | ||
'zip' => array( '/temp/lato.zip' ), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be additional tests (not necessarily in this PR) to ensure if an extender filters the supported MIME types, that ::has_font_mime_type()
takes this into consideration? I imagine this is already handled in the implementation, but might it be worth protecting this functionality with a test or two for when something is added/removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the list of valid mime types is not filterable. ::has_font_mime_type()
checks the mime via wp_check_filetype()
by passing the allowed types via a hardcoded list in WP_Fonts_Library::ALLOWED_FONT_MIME_TYPES
. There are no filters in the process.
phpunit/fonts-library/wpFonFamilyUtils/getFilenameFromFontFace-test.php
Outdated
Show resolved
Hide resolved
Props @costdev Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
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/fonts-library/wpFontFamily/__construct-test.php ❔ phpunit/fonts-library/wpFontFamily/base.php ❔ phpunit/fonts-library/wpFontFamily/getData-test.php ❔ phpunit/fonts-library/wpFontFamily/getDataAsJson-test.php ❔ phpunit/fonts-library/wpFontFamily/getFontPost-test.php ❔ phpunit/fonts-library/wpFontFamily/hasFontFaces-test.php ❔ phpunit/fonts-library/wpFontFamily/install-test.php ❔ phpunit/fonts-library/wpFontFamily/uninstall-test.php ❔ phpunit/fonts-library/wpFontFamilyUtils/getFilenameFromFontFace-test.php ❔ phpunit/fonts-library/wpFontFamilyUtils/hasFontMimeType-test.php ❔ phpunit/fonts-library/wpFontsLibrary/getFontsDir-test.php ❔ phpunit/fonts-library/wpFontsLibrary/setUploadDir-test.php ❔ phpunit/fonts-library/wpFontFamilyUtils/mergeFontsData-test.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @hellofromtonya! This looks good to me now 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the tests and couldn't find any issues.
Since this PR aims to refactor existing test classes, I'm not expecting any potential issues.
✅ LGTM.
Thank you @costdev and @anton-vlasenko! Merging it now. |
Part of #52698 tracking issue.
What?
For Core readiness and parity, this PR improves the tests by:
Why?
To prepare the tests for merging into Core.
How?
Each Fonts Library PHP class is split into separate test classes, one per public method.
WP_Font_Family
WP_Font_Family_Utils
WP_Fonts_Library
WP_REST_Fonts_Library_Controller
is not included in this PR as it needs different considerations to align with the other REST API tests in Core. A follow-up PR will handle it.Testing Instructions
PHPUnit tests and all CI jobs should still pass ✅