Skip to content
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

Merged
merged 16 commits into from
Mar 23, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 14, 2018

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:

  • Not emulating the DOM well enough
  • Not supporting desperately needed interactions
  • Inheriting too much of its API usage and sugar from jQuery
    • e.g. Often left confused how to use cy.should. should( 'have.css' ) evaluates with jQuery's .css, which assumes prior knowledge of jQuery
      • Aside: I used to know jQuery methods like the back of my hand, but as I've found myself using it less and less, my knowledge is waining, and this syntax is becoming less obvious
  • Language sugar is not obvious
    • Cypress's methods are asynchronous, but you would not know by looking at them. They can be chained with .then, but this is not (always?) necessary. Despite the language having evolved to support inline asynchronous code via async / await, Cypress uses its own alternative
    • Utility functions are defined as commands within Cypress on its global object
  • Built-in Chai + Sinon assertions
    • We use Jest elsewhere in the codebase. This means developers need to maintain knowledge of two separate assertion patterns for writing tests in the project.

Why 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 or browser instances as a global to allow for more convenient access. Most of its methods return promises, and as such work well with async / 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:

  • There's no nice UI like Cypress provides. I've found Cypress' UI can be helpful when stepping through a failing test case.
    • Edit: Puppeteer does include a section about debugging tips, including running in non-headless, and slowing down execution of commands (slowMo)
  • I'm not as often confused about the semantics for writing a test case.
  • Assertions aren't built-in, so we can write tests as we do elsewhere in the codebase with Jest assertions, lifecycle, etc. (less opinionated overall)
  • Utility functions are much easier to write and compose.

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

@aduth aduth added [Type] Question Questions about the design or development of the editor. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Mar 14, 2018
@ntwb
Copy link
Member

ntwb commented Mar 15, 2018

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:

  • This would limit testing to a single browser, Chrome, support is coming for Firefox in Cypress though progress appears to have stalled again, so /shrug.

  • Firefox now has a headless mode though unlike Puppeteer there is currently no native Node library and the recommended way is use Firefox headless via the selenium-webdriver

  • There is an existing PR add/selenium-tests: selenium tests that replicate the existing e2e tests #3664 to add selenium E2E tests by @EphoxJames with a couple of goals, firstly to support Firefox & Chrome E2E testing, and secondly keyboard testing where as already noted Cypress doesn't support right now

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...

@aduth
Copy link
Member Author

aduth commented Mar 15, 2018

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.

@youknowriad
Copy link
Contributor

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::

  • Async Cypress has built-in defaults for handling interaction timeouts, it waits for XHR requests on each call etc... It removes the need to think about those timeouts when writing tests

  • Debugging Very hard to debug, the UI provided by Cypress is really helpful while debugging tests. When working with Selenium, I found that you spend so many time debugging and trying to make the tests stable enough (no random results) that it makes you question the worthiness of these tests.

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.

@aduth
Copy link
Member Author

aduth commented Mar 19, 2018

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:

  • Async: For most behaviors, I expect the DOM reconciliation should be synchronous such that we shouldn't have any doubt whether an element exists. Where this may become an issue is where data may load in after-the-fact. I don't think these are very common, but there may be a few ways to address this in Puppeteer:
  • Debugging: Noted in an edit of the original comment, but Puppeteer does include a documentation section about debugging tips, including running in non-headless, and slowing down execution of commands (slowMo). In general, I have a feeling this won't be nearly as nice an experience as what Cypress provides. Long-term, I'm curious if this is a gap the ecosystem can fill with some layers to help step through failing tests. (Do such things exist for Selenium?)

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.

@aduth
Copy link
Member Author

aduth commented Mar 19, 2018

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.

@youknowriad
Copy link
Contributor

Alternatively, I had thought to consider rewriting existing tests with Puppeteer to see if it surfaced any potential pain points along the way.

That sounds like a good idea to me, if you want to split work, I'd be happy to help.

@aduth
Copy link
Member Author

aduth commented Mar 19, 2018

if you want to split work, I'd be happy to help.

Sure, it would be good to validate feedback from my than just myself. Also cc @twsp who had encountered similar issues with Cypress keyboard behaviors in course of #5641.

@youknowriad
Copy link
Contributor

@aduth I made a small update in 3dda486

  • Extracted utils and globals to separate files
  • I'm currently importing them in each file but I think the bootstrap should be moved too a setupFiles jest config once we add the npm script for these e2e tests.

Right now you can launch them using npx jest test/e2e/specs --watch

@aduth
Copy link
Member Author

aduth commented Mar 20, 2018

Updates are good 👍 I figured we'd want to restructure the files. The original implementation was meant mainly for demo-ing purposes only.

// Assertions
const textEditorContent = await page.$eval( '.editor-post-text-editor', ( element ) => element.value );

expect( textEditorContent ).toEqual( [
Copy link
Member

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).

Copy link
Member Author

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.

@aduth
Copy link
Member Author

aduth commented Mar 20, 2018

Some issues were observed with Puppeteer's slowMo option. It turns out that this is an issue with our default Jest configuration to use fake timers, since it conflicts with the slowMo behavior which uses said timers for delays.

I think we'll either need to:

  • Reconsider whether fake timers are the best default, or if we should make them opt-in per test
  • Create a separate configuration for the E2E tests which use real timers

@@ -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",
Copy link
Contributor

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

@youknowriad
Copy link
Contributor

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.

@@ -4,11 +4,10 @@ module.exports = {
'../../eslint/config.js',
],
env: {
mocha: true,
jest: true
Copy link
Member Author

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.

Copy link
Contributor

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 :)

import '../support/bootstrap';
import { newPost } from '../support/utils';

// Tests
Copy link
Member Author

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.

// Assertions
const textEditorContent = await page.$eval( '.editor-post-text-editor', ( element ) => element.value );

expect( textEditorContent ).toEqual( [
Copy link
Member Author

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.

@@ -0,0 +1,16 @@
/* eslint-env jest */
Copy link
Member Author

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",
Copy link
Member Author

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.

@youknowriad
Copy link
Contributor

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 :)

@youknowriad
Copy link
Contributor

I'd appreciate a second look here. I'm looking forward adding more e2e tests :)

@youknowriad youknowriad self-assigned this Mar 22, 2018
@@ -92,4 +92,13 @@ module.exports = {
},
],
},
overrides: [
Copy link
Member

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oups :)

Copy link
Member

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": [],
Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Member

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 ...

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

@gziolo gziolo left a 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:https://localhost:8888';
Copy link
Member

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.

Copy link
Member

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:https://my-custom-basee-url cypress_username=myusername cypress_password=mypassword npm run test-e2e

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member Author

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:https://localhost:8888' } = process.env;

Copy link
Member

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:https://localhost:8888"
    }
}

@@ -0,0 +1,69 @@
import path from 'path';
Copy link
Member

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';
Copy link
Member

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.

@gziolo
Copy link
Member

gziolo commented Mar 22, 2018

I tried a few different setups to move bootstrap.js code to setupTestFrameworkScriptFile , or globalSetup & globalTeardown (as shown in https://github.com/xfumihiro/jest-puppeteer-example) but they aren't stable enough. At the moment it needs to be imported for every test suite, which isn't ideal.

I often see an error like this, which might be an existing issue but hidden somehow:

Expected value to equal:
StringContaining "is-multi-selected"
Received:
"editor-block-list__block"
at _callee2$ (/Users/gziolo/PhpstormProjects/vagrant-local/www/wordpress-default/public_html/wp-content/plugins/test/e2e/specs/multi-block-selection.test.js:36:26)

Copy link
Member

@gziolo gziolo left a 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.

@youknowriad youknowriad merged commit 4eed1e4 into master Mar 23, 2018
@youknowriad youknowriad deleted the try/puppeteer-e2e branch March 23, 2018 09:40
@ntwb ntwb mentioned this pull request Mar 29, 2018
3 tasks
@aduth aduth changed the title Try: Testing: Experiment with Puppeteer for E2E testing Testing: Experiment with Puppeteer for E2E testing Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants