Font Library: add e2e tests for Upload tab #59316
Conversation
mikachan
added
[Type] Automated Testing
mikachan
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
mikachan
requested review from
getdave,
matiasbenedetto,
pbking and
creativecoder
|
An interesting method to avoid lack of user switching in Playwright utils! |
|
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. |
| if ( | ||
| await page | ||
| .getByRole( 'button', { name: /commissioner/i } ) | ||
| .isVisible( { timeout: 40000 } ) | ||
| ) { | ||
| await page | ||
| .getByRole( 'button', { name: /commissioner/i } ) | ||
| .click(); | ||
| await page.getByRole( 'button', { name: /delete/i } ).click(); | ||
| await page.getByRole( 'button', { name: /delete/i } ).click(); | ||
| } |
There was a problem hiding this comment.
Can this be achievable programmatically before the test is run?
There was a problem hiding this comment.
Do you mean by doing something like clearing the installed fonts via the REST API, rather than via the UI in the test?
| test( 'should not display the "Upload" tab', async ( { page } ) => { | ||
| await expect( | ||
| page.getByRole( 'tab', { name: /upload/i } ) | ||
| ).toBeHidden( { timeout: 40000 } ); |
There was a problem hiding this comment.
We should probably comment to explain the need for the timeout.
There was a problem hiding this comment.
Added a couple of comments here: 8e3f7bf
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 8e3f7bf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8021449846
|
creativecoder
changed the base branch from
update/font-upload-ui-permissions
to
update/font-library-ui-permissions
|
I'm not fully convinced customized font post permissions deserve e2e tests (coming from the philosophy that e2e tests are expensive and generally saved for the most important flows). Is there a component level test we could do here instead that would be lighter weight and test that the components contain the correct logic based on the results of |
|
e2e tests are far less expensive than they were previously, however I take your point. The testing instructions on the previous PR had a lot of edge cases and potential setup instructions to validate the behaviour. I felt that when things get to that level of granularity, having a few tests to ensure there are no regressions is easier than trying to manually validate behaviour. If the scope of the permissions has now lowered then it may be worth reducing the level of detail and just smoke testing the high level of functionality. You are all best placed to make a call on this - after all you are likely to be involved in resolving any future regressions 😅 |
…dd-e2e-tests-for-font-perms # Conflicts: # packages/edit-site/src/components/global-styles/font-library-modal/index.js # packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
mikachan
changed the title
I also understand where you're coming from, but we're also lacking quite a few scenarios in the e2e tests for the Font Library generally and I think testing uploading a local font from the UI is probably a worthy e2e test to include, as it's encompassing many parts of the feature in a small number of tests. I hear you about them potentially being quite expensive though, I'm a little concerned about the timeouts I've had to add for these tests. It seems that the Font Library takes a while to resolve in the Playwright environment. |
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
In my experience these typically indicate something isn't right in the underlying code under test 😬 |
| } ) => { | ||
| await expect( | ||
| page.getByRole( 'tab', { name: 'Upload' } ) | ||
| ).toBeVisible( { timeout: 40000 } ); |
There was a problem hiding this comment.
What condition is applying here in the UI to cause you to need such a long timeout?
There was a problem hiding this comment.
It seems to take quite a while to fetch the font family post data, it looks like it's the useEntityRecords call 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.
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 Upload tab to users for example).
There was a problem hiding this comment.
What's the minimum set of tests we could introduce? Or are we already at that point?
There was a problem hiding this comment.
In which case I would be in favour of adding perhaps 1 or 2 high level "smoke tests".
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 😄
What's the minimum set of tests we could introduce? Or are we already at that point?
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.
An installed and activated font is shown correctly in the list of Font Library fonts
The Google Fonts collection has a tab shown in the Font Library modal
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.
But using custom permissions seems like a rare use case (I'd guess a small fraction of a percent of sites), so still not sure it's worth spending an e2e test on.
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.
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?
Yes, I think it would be.
There was a problem hiding this comment.
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.
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.
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.
Not sure if this is a good idea, but it should be possible to preload the font-families API response using a filter
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.
Can we just eliminate the default font collection from the tests?
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.
mikachan
changed the title
|
Closing this in favour of #60221. Thanks for all the input on this one 🙇 |
What?
Adds some e2e tests to cover the changes made in #59332.
How?
Tests the following scenarios:
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