-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Testing: Experiment with Puppeteer for E2E testing #5618
Conversation
Thanks for the detailed write up and reference links 👍 No objections from me, but I'd like to pint out what has been raised previously:
p.s. I liked seeing shiny new thing Jest Puppeteer linked which I've been watching, in 12 days it's garnered 500+ stars, which is not any significant metric other than a lot of people are expressing interest in Jest and Puppeteer in a very short time frame... |
Ultimately a main consideration for me is the developer experience in authoring test cases. I want to write more E2E tests, but each time I have put effort toward it, it's consistently resulted in nothing but headaches. If a tool like Puppeteer enables this to be a painless and consistent experience, but means we're restricted to testing Chrome, I'm fine to make that compromise. I really don't have much experience with Selenium, so I'll not speak to its role in this conversation, and how its experience aligns with my observations. |
Agreed that Cypress is not great for handling selection and text interactions in general and that's very important for our project. I think the other parts where Cypress falls short are less important though. The alternative proposed until now was Selenium and my experience with it until now was not great for several reasons::
So, yeah we need an alternative here and ideally, we would have the best of both worlds. So: Do you know how Puppeteer handles the timeouts and the async behaviors? Do you have to tweak those separately and think about those in each test or are there generic ways to handle those without questioning the efficiency of these timeouts on every run? The loss of the Cypress UI is unfortunate but it's not a blocker for me if Puppeteer tests prove to be stable enought and easy to write without the need to think about async timeouts... These questions are hard to answer without actually trying it for some time, so I'd be fine keeping both for a release or two and trying to add more tests and see how it goes. |
Thanks for the feedback @youknowriad . I think it's generally agreed that Cypress has some nice conveniences which smooth over some of the rough points which would be surfaced by a "closer to the metal" approach like that of Puppeteer. To your specific points:
To Selenium vs. Puppeteer: Again, I'm not familiar with the former enough to be able to comment beyond statements which may be attributed as vanity ("new hotness"). Again noted in the original comment, it does appear Google is quite invested in their headless browser and the underlying Chrome DevTools Protocol used by both Puppeteer and Node.js' native inspector, which may speak to its long-term stability. In doing some brief research to gauge sentiments, the general consensus is they seek to achieve the same goal, with Selenium being the more mature and broadly-supporting option, yet a higher-level abstraction and suffering in heaviness/slowness/"quirks" for it. I'll reiterate that my main objective is to make authoring E2E tests less painful and unpredictable, and am personally okay to make some sacrifices (browser compatibility, more difficult testing for less-common behaviors) in the pursuit of this goal. |
For moving forward, I'd be okay to run in parallel, though 200MB of dependencies between the two is a pretty steep penalty to hold for very long. Alternatively, I had thought to consider rewriting existing tests with Puppeteer to see if it surfaced any potential pain points along the way. The single test implemented here was a simple test to demonstrate one which has proven challenging to implement with Cypress. |
That sounds like a good idea to me, if you want to split work, I'd be happy to help. |
@aduth I made a small update in 3dda486
Right now you can launch them using |
Updates are good 👍 I figured we'd want to restructure the files. The original implementation was meant mainly for demo-ing purposes only. |
test/e2e/specs/adding-blocks.test.js
Outdated
// Assertions | ||
const textEditorContent = await page.$eval( '.editor-post-text-editor', ( element ) => element.value ); | ||
|
||
expect( textEditorContent ).toEqual( [ |
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.
This looks like a great fit for snapshot testing (if it is configured).
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 would be the matter of changing to toMatchSnapshot
, yeah? I agree this would be a good use-case.
Some issues were observed with Puppeteer's I think we'll either need to:
|
@@ -144,13 +128,13 @@ | |||
"fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate", | |||
"package-plugin": "./bin/build-plugin-zip.sh", | |||
"pot-to-php": "./bin/pot-to-php.js", | |||
"test-unit": "wp-scripts test-unit-js", | |||
"test-unit": "wp-scripts test-unit-js --config test/unit/jest.config.json", |
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'm not certain what's the difference between wp-scripts test-unit-js
and just jest
. Both seems to work which makes me wonder why we need this script at all cc @gziolo
6e75f05
to
327ef3d
Compare
Ok! so I went ahead and extract the jest config into two separate config files, and replace Cypress entirely. Still seeing some inconsistent failures, let's how this goes in CI. |
test/e2e/.eslintrc.js
Outdated
@@ -4,11 +4,10 @@ module.exports = { | |||
'../../eslint/config.js', | |||
], | |||
env: { | |||
mocha: true, | |||
jest: true |
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.
Since this extends our base configuration, we shouldn't need to specify env
at all now I think.
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 tried but it didn't work :)
test/e2e/specs/a11y.test.js
Outdated
import '../support/bootstrap'; | ||
import { newPost } from '../support/utils'; | ||
|
||
// Tests |
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 don't know that these specific comments add much value.
test/e2e/specs/adding-blocks.test.js
Outdated
// Assertions | ||
const textEditorContent = await page.$eval( '.editor-post-text-editor', ( element ) => element.value ); | ||
|
||
expect( textEditorContent ).toEqual( [ |
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 would be the matter of changing to toMatchSnapshot
, yeah? I agree this would be a good use-case.
test/e2e/support/bootstrap.js
Outdated
@@ -0,0 +1,16 @@ | |||
/* eslint-env jest */ |
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 don't think we should need either this or the following line anymore, since these are now incorporated into e2e/.eslintrc.js
package.json
Outdated
@@ -87,6 +86,7 @@ | |||
"phpegjs": "1.0.0-beta7", | |||
"postcss-loader": "2.0.6", | |||
"prismjs": "1.6.0", | |||
"puppeteer": "1.1.1", |
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's been a 1.2.0 release since the PR was first opened. We should probably plan to upgrade.
So the tests are passing even in CI, with the last commit I'm trying to tweak the timeouts and also isolate the tests (a new browser page per test) to increase their stability. I think we could start thinking about merging this PR :) |
b68f195
to
e35fbfa
Compare
I'd appreciate a second look here. I'm looking forward adding more e2e tests :) |
@@ -92,4 +92,13 @@ module.exports = { | |||
}, | |||
], | |||
}, | |||
overrides: [ |
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 removed the other Eslint config and made it work with overrides
:)
.eslintrc.js
Outdated
@@ -11,7 +11,7 @@ const { version } = require( './package' ); | |||
/** | |||
* Regular expression string matching a SemVer string with equal major/minor to | |||
* the current package version. Used in identifying deprecations. | |||
* | |||
*n |
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.
oups :)
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.
Removed :)
{ | ||
"rootDir": "../../", | ||
"preset": "@wordpress/jest-preset-default", | ||
"setupFiles": [], |
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 think support/bootstrap
should go in here or we should use the existing jest-puppeteer
preset: https://www.npmjs.com/package/jest-puppeteer. See also: https://facebook.github.io/jest/docs/en/puppeteer.html#use-puppeteer-preset which informs about the outdated preset.
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 tried the bootstrap here but it didn't work, I can't remember the reason. I think it's related to beforeAll
not yet defined or something like that. Any idea?
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 other examples, it goes to the globalSetup
, but they also do some optimizations to share a connection to the browser between tests. We should look into it 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.
We probably shouldn't use this preset as setupFiles
only appends new files, it doesn't allow to override it. Sometimes it's good but sometimes bad ...
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'm keen on using the jest-puppeteer
preset but it is pretty new and shiny.
Currently an issue argos-ci/jest-puppeteer#17 "Jest's native expect functions are broken by puppeteer-expect" would be a blocker for us
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.
@ntwb, another blocker is that whatever they suggest in the example repository recommended by Jest docs, it doesn't work stable with the tests we have :(
It looks like what is proposed in this PR is the way to go.
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 didn't look that close on the logic of the tests but this looks like a good start. We need to look into Jest docs and improve the integration with puppeteer as explained in here: https://facebook.github.io/jest/docs/en/puppeteer.html and in the example repository linked there.
import path from 'path'; | ||
import url from 'url'; | ||
|
||
const BASE_URL = 'http://localhost:8888'; |
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 was possible to provide a different value for BASE_URL
, USERNAME
and PASSWORD
. We should look into it.
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.
See: cypress_base_url=http://my-custom-basee-url cypress_username=myusername cypress_password=mypassword npm run test-e2e
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, this was available by default in cypress, we need to do it ourselves in the puppeteer setup but not that hard I think.
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, shouldn't be an issue. Can be done 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.
Could just be some simple env variables:
const { E2E_BASE_URL = 'http://localhost:8888' } = process.env;
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 was this idea someone shared, that we might want to introduce wordpress
section inside package.json
to provide a way to override some variables.
{
"wordpress": {
"baseUrl": "http://localhost:8888"
}
}
@@ -0,0 +1,69 @@ | |||
import path from 'path'; |
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.
The comment for dependencies is missing.
@@ -0,0 +1,12 @@ | |||
import puppeteer from 'puppeteer'; |
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.
The comment for dependencies is missing.
I tried a few different setups to move I often see an error like this, which might be an existing issue but hidden somehow:
|
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 tested it locally, all looks good. I don't have any quick solutions how to improve the config part of it. I'm planning to take a look at it later and extract that to @WordPress/packages
. Not a blocker at all. Awesome work on this PR you both @aduth + @youknowriad.
This pull request seeks to experiment to see how Google Puppeteer might fair in serving as our E2E testing solution, and whether it fills any of the gaps and frustrations with Cypress, our current solution.
What's wrong with Cypress?
I've wanted to like Cypress, and it does have a nice interface, but writing tests has been a constant source of headaches for me. I've attributed this to a few main causes:
cy.should
.should( 'have.css' )
evaluates with jQuery's.css
, which assumes prior knowledge of jQuery.then
, but this is not (always?) necessary. Despite the language having evolved to support inline asynchronous code viaasync
/await
, Cypress uses its own alternativeWhy Puppeteer?
Google's Puppeteer, and the Chrome headless effort it is built upon, currently maintains quite a bit of momentum behind it, and supports nearly all of the same functionality of Cypress without the sugar, incompatibilities, and inconsistencies.
With Puppeteer, utility functions can be defined as plain functions. We can expose Puppeteer's
page
orbrowser
instances as a global to allow for more convenient access. Most of its methods return promises, and as such work well withasync
/await
syntax.The download size is a fair bit smaller, closer to 75MB on Mac, instead of Cypress' 115MB.
Another interesting consideration is that Puppeteer / Chrome headless may eventually replace the need for JSDOM. There are many instances of test code which is made uglier by the need to accommodate JSDOM behaviors.
Findings
Working with Puppeteer, there feels like less sugar. This is obvious in that:
slowMo
)One frustration I found, mostly in trying to port JSDOM usage, is that the testing environment and the Chrome window are insulated from one another, and communicate via a bridge. This can be seen in its various evaluate functions (e.g.
page.evaluate
), where the insulation prevents use of variables scoped within the tests, but not within the Chrome window. Since objects must be serialized over the bridge, it is difficult to pass complex objects back and forth (e.g. functions, DOM elements). I don't know that this is really any different than with Cypress, since we've not done much passing back-and-forth (most relevant in how it could be used as a replacement for JSDOM).In all, I have good confidence that Puppeteer has a strong future ahead of it. Cypress gets a lot of things right (e.g. nice UX), but in many ways it tries too hard and suffers for it.
Related Resources