Font Library: add e2e tests for Upload tab #59316
Use font-families endpoint to check installed fonts
- Loading branch information
commit 287252a802d75d6861ae00160ccf367f37165127
There are no files selected for viewing
This file contains hidden or 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
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.
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.
What condition is applying here in the UI to cause you to need such a long timeout?
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.
It seems to take quite a while to fetch the font family post data, it looks like it's the
useEntityRecordscall here that takes a while to resolve. There doesn't seem to be any errors, just that it takes longer than the default timeout set in Playwight to resolve.I'm currently looking at adding logic that would wait for this to resolve before running any of these tests. This should remove the need for specific timeout flags, but I'm assuming realistically it's still going to be a slower-than-average suite of tests, which I'm not a fan of adding.
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.
In which case I would be in favour of adding perhaps 1 or 2 high level "smoke tests". We might not catch every single bug but it might be enough to guard regressions around critical functionality (e.g. whether or not we display the
Uploadtab to users for example).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.
What's the minimum set of tests we could introduce? Or are we already at that point?
Uh oh!
There was an error while loading. Please reload this page.
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 been thinking this too, I'll update the PR so the tests don't include uploading a font. We can always come back to that later if necessary. Also, I learned how to upload files in Playwright, so that's something 😄
I like the idea of testing whether the Upload tab should appear or not based on permissions, so perhaps these are enough for now. It doesn't just test the permissions but also whether the font face posts have been initialised/fetched correctly, as the tab will only show once the posts have been fetched.
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.
These were covered previously but I've just removed them to simplify these tests, but happy to add them back! The main issue I'm seeing is that resolving the font family posts takes a few extra seconds, so this means we'd be adding some potentially slow e2e tests. I'm wondering if there's something we can improve here in the order in which the font posts are requested, i.e. perhaps the request could start at the same time as the modal opening, rather than after the modal has opened.
Maybe we should test whether the Font Library UI is disabled via the editor settings rather than checking permissions. Does that sound worthy of a test?
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.
Yes, I think it would be.
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.
Not sure if this is a good idea, but it should be possible to preload the font-families API response using a filter, in the same way that essential routes that are used when the editor first loads are preloaded, by outputting the response JSON into the html markup on the page when it loads.
But I don't think this would make sense normally, where the font library may or may not be opened. It would only be for the test environment--hence not being sure if it's a good idea to "fork" the test environment like this. But it may be worth it--the only change is where the response data comes from, the actual API response should be exactly the same either way.
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.
Can we just eliminate the default font collection from the tests? Unregister that one and register another, much smaller one that won't require a json file to resolve at all. We'll probably wind up with a few different font collection scenarios to manufacture and test anyway.
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.
There are a few instances in the e2e tests where we wait for either certain blocks to load in or specific endpoints to resolve before moving on, so I think we can use a similar method here.
The default font collection isn't loaded until the Google Fonts acceptance notice has been clicked, which hasn't been tested so far, so the whole collection hasn't been resolved as part of these tests, only the tab.
It's the initial post request that seems to take a while, but I'm not sure what part of it is taking a while or if it's a sign that performance needs to be improved somewhere. I usually find it easy to replicate in a Playground instance too - the initial request takes a noticeable 5-8 seconds. I'm not sure if addressing this is an urgent priority, and initially, perhaps we could look at improving the UX around it. Currently, there is a spinner that appears during the post request and then disappears once resolved. If there are no custom fonts, it's not clear what the spinner is indicating.
Also apologies as I think I'm deviating a bit here 😅 I plan to finish up the minimal tests in this PR and maybe we should open a separate issue to discuss performance improvements.