E2E Tests: Verify the expected theme is active. #18699
Conversation
|
Hm, the thing with a check added in the global config like this is that we suddenly require Twenty Twenty for any tests regardless of their needs. I think there are legitimate cases where you’d want to run given test suites against a site with a different active theme. Sure, the global config may be bypassed, but how practical is that? This is why I was speculating in the other issue about a suite-specific theme constraint (and hence the importance of performance). |
We already do, implicitly. All of our tests expect the latest theme and our CI/CD pulls from |
| async function verifyActiveTheme() { | ||
| await page.goto( | ||
| createURL( '' ) | ||
| ); |
There was a problem hiding this comment.
What does this ensure, and can we achieve the same perhaps with a more semantic use of Puppeteer's API?
There was a problem hiding this comment.
It takes you to the home page in the front end of the site so that the theme's stylesheets will be loaded.
a more semantic use of Puppeteer's API?
What were you thinking?
There was a problem hiding this comment.
I was thrown off by my own expectation of not having to navigate anywhere. Indeed, with a one-off requirement specially made for Twenty Twenty it would be enough to look for #twentytwenty-block-editor-styles-css in the editor itself.
What you're doing is more generic, since the existence of #foo-style-css in the front end is guaranteed by WordPress itself, not the theme implementation. There's no such guarantee for the back end, alas, as editor styles are optional.
In short: disregard my comment about anything more semantic.
Yes, this means other projects using How do you feel about tracking down which test suites rely on a specific theme and adding |
942e5b1 to
e1f8a86
Compare
e1f8a86 to
e61b891
Compare
| export async function verifyActiveTheme( theme ) { | ||
| await page.goto( createURL( '' ) ); | ||
| if ( ( await page.$( `#${ theme }-style-css` ) ) === null ) { | ||
| process.exit( `Test suite must be run using the ${ theme } theme.` ); |
There was a problem hiding this comment.
We have to use process.exit, because of this bug in Jest.
e61b891 to
674a8a5
Compare
|
How feasible it would be to update the existing tests to be theme independent? Can we customize the theme by setting our own colors and fonts which override theme defaults? Alternatively, can we override CSS or element styles in places where it becomes theme-specific and fails tests? |
I think that is a much better approach. I tried it here: #18761, and it seems to work well. Should we close this and merge that? |
|
Yes, that sounds much better, and #18761 already looks promising. |
|
Closing this then. |
Description
This PR adds a check before all E2E tests are ran that verifies that the theme the tests expect is active.
Sometimes, contributors run tests locally with different themes and are confused as to why they fail. This check will surface those situations. See this comment for an example.
How has this been tested?
It was verified that running the E2E tests with a theme other than the currently expected one, twentytwenty, throws a descriptive error before all tests are run.
Checklist: