-
Notifications
You must be signed in to change notification settings - Fork 48
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
PCHR-3221: Migrate BackstopJS test suite to headless Chrome + Puppeteer #2535
Conversation
…adless-chrome-backstopjs-puppetter
…topjs-puppetter PCHR-3221: Migrate BackstopJS test suite to Puppeteer
fb97ce6
to
a8f7aed
Compare
@@ -23,6 +23,6 @@ module.exports = modal.extend({ | |||
* to wait a couple of seconds for it to "stabilize" before taking the screenshot | |||
*/ | |||
async waitForReady () { | |||
await this.puppet.waitFor(2000); | |||
await this.puppet.waitFor(4000); |
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.
Why cant we use a selector here?
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's another of BackstopJS quirks, it's really not related to any particular content in the page
@@ -10,7 +10,8 @@ | |||
"label": "SSP / Manager Leave / Legend expanded", | |||
"url": "{{siteUrl}}/manager-leave#/manager-leave/calendar", | |||
"onReadyScript": "ssp/manager-leave/calendar-legend-expanded.js", | |||
"user": "civihr_manager" | |||
"user": "civihr_manager", | |||
"requireSameDimensions": false |
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 why we need to add this? Why exactly the dimensions were different in the first place.
Please add a block in the doc section.
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'll add it, but the short answer to "why" is "i don't know" unfortunately. For those couple scenarios backstopjs sometimes, at random, takes screenshots with different dimensions (i think you also saw it first-hand when testing), so we have to tell it to ignore them during the comparison, otherwise we'll end up with false positives
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.
Added a new "Avoid dimensions-based false positives" section in the PR description
*/ | ||
async waitForReady () { | ||
await this.puppet.waitFor('.ct-filter-date', { visible: true }); | ||
await this.puppet.waitFor('.ct-table-documents [href^="/civicrm/contact/view"]', { visible: 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.
I was checking the UI, this selector is present twice. So It might be due to one of the elements being visible before the other. Could you check by waiting for individual items like
.ct-table-documents td:nth-child(2) [href^="/civicrm/contact/view"
and
.ct-table-documents td:nth-child(3) [href^="/civicrm/contact/view"
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's not important if there is one or multiple selectors (it really depends on the data in your local site), as long as one of element that matches that selector is visible, then backstop gets the green light
Update
With #2565 now merged in this branch, any information related to Chromy in the PR documentation should be considered obsolete
Overview
Our current BackstopJS setup uses PhantomJS as headless browser and CasperJS as library to control the browser.
Given that Google released a year ago headless Chrome, that development and support for PhantomJS had been halted, and that BackstopJS had been supporting Chromy since v3.0, it only made sense to migrate our setup to headless Chrome + Chromy.
Couple of notes:
Although BackstopJS supports parallel capturing with Chromy, the results are unfortunately not reliable (see Not really reliable results using engine Chromy garris/BackstopJS#696), so we're still stuck for the time being with capturing only one screenshot at a time
This Chromy set up is temporary, because BackstopJS's support for Puppeteer is underway. Given that this is Google's official control library for Chrome, given that the API is much more powerful and comprehensive, that the documentation is in a much better state, and that the performance is better, we will switch to Puppeteer as soon as its integration in BackstopJS is released in a stable version.
I've actually spent a couple of days to perform the migration from Chromy to Puppeteer in a separate branch, but the integration is still not stable and leads to unreliable results (see here, but I also got problems with random timeouts and inconsistent screenshots, even with one capture at a time), so I've parked it for now and will resume it once things are more stable.
Technical Details
Migrating from CasperJS to Chromy
CasperJS's api is, from what I could see, more comprehensive and well documented than Chromy's api (make sense, given the former has been around for much longer), so it took a while and a bit of trial and error to perform the migration from one to another.
Things worth mentioning:
When I started the migration, there were no equivalents of waitUntilVisible(), waitWhileVisible() in Chromy. After a bit of back-and-forth with the library's author here, he gently gifted me with v5.11.0 which does offer a
waitUntilVisible()
method.Unfortunately the library is still lacking a
waitWhileVisible()
method (I tried to implement it myself but got stuck), so I had to resort to writing a manual version for it, which is basically doing the opposite check of whatchromy.visible
does under the hoodThere is no clickLabel() in Chromy, so I wrote a manual version
also note that in Chromy you pass arguments to the
evaluate()
function in an array, not as a list of individual arguments as in CasperJS (or Puppeteer)I didn't polish any workaround that i've implemented (like the ones above), given that we will soon switch to Puppeteer anyway
Using shrinkwrap to make BackstopJS use latest version of Chromy
As mention above, the Chromy library had been bumped to v0.5.11 to include a
waitUntilVisible()
method, but BackstopJS still defines (at the time of this writing) v0.5.7 in its dependencies, meaning the new method would be unaccessible from the engine scripts.Defining Chromy v.0.5.11 in our package.json file would not solve the issue (when backstop does
require('chromy')
it gets it from its own node_modules/ folder), so I had resorted to use the shrinkwrap command, which converts the package-lock.json file to a npm-shrinkwrap.json file that can be edited, allowing to override any package's dependency with our own.What I did was then simply to add
chromy@0.5.11
in our package.json and remove anychromy
reference to the BackstopJS dependencies, so that when backstop requireschromy
, it will find it in hrcore's node_modules/ folder, and not in its ownLog in via cookies
In the PhantomJS+CasperJS setup, BackstopJS would log via a script that would go to the /welcome-page url (after eventually logging out) and "manually" enter the login credentials for the given user, stored in a site-config.json file.
This approach had two major drawbacks:
So an alternative approach that involves cookies had been developed:
backstopjs
task is called, for each of the 4 major users (admin, civihr_admin, civihr_manager, civihr_staff), rundrush uli
.--name
parametercookies/${userName}.json
fileuser
property. If nouser
is defined, it will be assumed that the user is civihr_adminbackstopjs
gulp task, for each scenario define thecookiePath
property based on the user associated to the scenariocookiePath
is used by theloadCookies.js
script, called byonBefore.js
(both provided by default by BackstopJS), which is going to set the cookies via ChromyThis whole things works also in parallel, given that two scenarios that use different users simply need to read a different .json file, without interfering with each other.
Fetch contact ids via drupal usernames
Some scenarios require BackstopJS to go the the page for a specific user with a specific CiviCRM contact id (for example the contact summary page of the "admin" user). Given that the id of a specific user might change from one installation to another, the ids in the scenarios' urls were kept as placeholders
only to be replaced later with the real ids on run-time.
The real ids were originally obtained via the civi api, by querying all the users with an email address beginning with "civihr_", given that the 3 basic users (civihr_admin, civihr_staff, civihr_manager) all had emails starting with "civihr_ (civihr_admin[at]email.something.com, etc)
This means though that those users can't have different emails in different installations, which unfortunately is not the case. So in case those users had different emails, they would not be found and the BackstopJS test suite could not be run.
The solution for this was to ignore the email and get the civi id via the drupal user names, given that those will always be the same (admin, civihr_admin, civihr_manager, civihr_staff). This is done in the following way:
drush user-information
to fetch the info of the 4 users, in a JSON formatuid
) and we map them to the usernameUFMatch
api entity to get the contacts associated to thoseuid
scontact_id
) and we map them to the usernameGo directly to the Job Contract / Job Roles / Documents / Tasks tab
Before, when testing any of those tabs ^, the script would simply first go to the Contact Summary page, wait for it to be loaded, then click on the tab, wait for the content to be loaded, and finally start capturing the screenshot
Given that we can provide, as part of the /civicrm/contact/view url, a
selectedChild
param that would switch automatically to the given tab, the approach above would waste a lot of precious time.So all the scenarios for those tabs had been improved, and now they go directly to the intended tab, making those scenarios much faster to run
Basic "show" scripts for contact summary and documents page
The scenarios for capturing the contact summary and documents page "as is" didn't have any onReadyScript assigned, meaning that BackstopJS would not wait for anything in particular (either some amount of time or a specific selector) before capturing a screenshot, which lead to inconsistent results
Adding a very simply
show.js
script for each of them solved the issue, leading to consistent resultsRemoving code warnings before capturing
We can have code warnings in our dev environments, both on admin and ssp
These warnings are sometimes displayed and sometimes not, meaning that they could lead to some false positive diffs.
The
page.init()
method now takes care of removing them before BackstopJS captures the screenshot(the code is not well optimised because the moment i tried to encapsulate the logic in some separate function Chromy would start act funny, so after I while I gave up and left it like this)
Removed unnecessary IIFEs
In basically every page object file, we had this thing going on with
module.exports
and IIFEwhich really served no practical purpose. I removed all of those instances
Scenarios clean up
The scenario files had been cleaned up, both in terms of how the scenarios were aggregated and in how many files and how those files were called (you can see the difference before and after) and most importantly in terms of labels consistency
Before
After
Avoid dimensions-based false positives
For some reasons, and only for specific scenarios (namely: "SSP / Manager Leave / Legend expanded" and "SSP / Onboarding Wizard / Dependants"), BackstopJS might end up taking screenshots of different sizes. BackstopJS by default not only checks difference in the content of the screenshots, but also in their dimensions, meaning that those scenarios will end up failing the comparison even if their content had not changed
Unfortunately I could not figure out why this weird behaviour happens in the first place, so I ended up using the
"requireSameDimensions"
property by setting it tofalse
on those scenarios, so that only the content, and not the size, of the screenshots are going to be factored in the comparison