Font Library: Add upload font test #60221
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
|
Size Change: +1.37 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
getdave
reviewed
Mar 27, 2024
getdave
reviewed
Mar 27, 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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
mikachan
added
[Type] Automated Testing
This was referenced Mar 27, 2024
getdave
reviewed
Mar 27, 2024
getdave
reviewed
Mar 27, 2024
getdave
reviewed
Mar 27, 2024
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
getdave
reviewed
Mar 27, 2024
|
I've pushed the following changes to this branch
|
|
Just noting that I've made a couple of small tweaks:
|
peterwilsoncc
previously requested changes
Mar 27, 2024
Comment on lines
+31
to
+33
| if ( is_dir( $font_path ) ) { | ||
| rmdir( $font_path ); | ||
| } |
There was a problem hiding this comment.
In some development environments the content and uploads folder is shared between the test suite and the developer's manual testing so deleting the directory will fail as it will contain files.
If the files are deleted beforehand then the manual testing environment will be busted.
For the test suite I suggest filtering the folder and cleaning it up afterwards. You can probably use wp_generate_uuid4() to generate the folder name.
There was a problem hiding this comment.
Good point, that part is brittle. I've modified the plugin to use a randomly generated directory for fonts during the duration of the test and to cleanup on plugin activation and deactivation. Let me know what you think!
There was a problem hiding this comment.
Thanks Grant, that looks good. Is there an e2e tear down at the end that could be used to delete the temporary folder? It's no big deal if there isn't.
There was a problem hiding this comment.
I think that because of the way we're testing here with a plugin to handle the server-side logic, the tear-down happens on deactivation on the plugin, i.e. on line 19 we deactivate the plugin after all tests:
test.afterAll( async ( { requestUtils } ) => {
await requestUtils.deactivatePlugin(
'gutenberg-test-delete-installed-fonts'
);
} );
And the plugin handles the deletion of fonts and folders on deactivation:
register_deactivation_hook( __FILE__, 'gutenberg_delete_installed_fonts' );
…in activation and deactivation
|
Reviewing this now |
getdave
reviewed
Mar 28, 2024
There was a problem hiding this comment.
Great work here 👏
This provides a good level of coverage for the features and confidence for the fixes we shipped in WP 6.5 RC4.
Longer term we can look at other ways of providing coverage and perhaps reduce the reliance on complex tests such as this one.
Some small tweaks might improve this.
getdave
dismissed
peterwilsoncc’s stale review
This PR needs to be merged prior to RC4 code freeze and contributor is not available. Please see https://wordpress.slack.com/archives/C065MBW03EH/p1711624856021779
getdave
approved these changes
Mar 28, 2024
This was referenced Mar 28, 2024
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
* Add upload font test * Use Exo 2 woff as test font * Test uploading multiple fonts * Update test/e2e/specs/site-editor/font-library.spec.js Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update test/e2e/specs/site-editor/font-library.spec.js Co-authored-by: Dave Smith <getdavemail@gmail.com> * Add test for CSS rules * Rename multiple fonts test case * Add e2e plugin to delete fonts and the fonts directory * Use delete fonts plugin in e2e tests and check for all uploaded font variations * Fix coding standards * Remove timeouts * Remove step to go back to the Library tab * Remove isVisible check, covered with click action * Remove visible check for another element that is clicked * Modify e2e fonts plugin to use a random directory and cleanup on plugin activation and deactivation * Fix lint * Move font management tests into separate describe block --------- Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Grant Kinney <hi@grant.mk> Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Updates the Font Library e2e tests to include a test for uploading a font. A continuation of the efforts made in #59316.
Includes test coverage for:
How?
Adds new test case for "should allow user to upload multiple local font files" and "should allow user to delete installed fonts". Also updates all
getByRolecalls to use actual strings from the labels, as per this comment.Testing Instructions
Run the following command to run these tests locally and ensure they pass:
npm run test:e2e:playwright -- test/e2e/specs/site-editor/font-library.spec.js