garris / BackstopJS

Catch CSS curve balls.
http://backstopjs.org
MIT License
6.73k stars 602 forks source link

[New Feature Request] Retry Function #761

Open Kenith opened 6 years ago

Kenith commented 6 years ago

Hello Garris,

We were wondering that is it possible to add retry feature if any critical error during the test. With the retry feature, we consider it could improve the backstopjs stability significantly.

Thank you, Ken

Kenith commented 6 years ago

+1

garris commented 6 years ago

What would the retry feature do? How would it work?

Kenith commented 6 years ago

Here are the stories. Really appreciate for your HOT help in advance.

AC-1 GIVEN I am backstop customer THEN I could be able to set the retry times in backstop.json AND Default retry times is 1

AC-2 GIVEN I am backstop customer WHEN I have set the retry times in backstop AND Some scenarios encounter errors (such as timeout error) during "backstop reference", THEN Backstop will re-capture the pages for these scenarios based on the retry numbers

AC-3 GIVEN I am backstop customer WHEN I have set the retry times in backstop AND Some scenarios encounter errors (such as timeout error) during "backstop test", THEN Backstop will re-capture the pages for these scenarios based on the retry numbers

AC-4 GIVEN I am backstop customer WHEN I have set the retry times in backstop AND Some scenarios encounter mis-match error during the comparing process of "backstop test" THEN Backstop will re-capture and re-compare the pages for these scenarios based on the retry numbers

AkA84 commented 6 years ago

+1 i think this would be really great for improving backstop's reliability @garris , especially because right now the parallel capturing for us simply doesn't work as it throws errors for scenarios that do not give any problem when single-capturing (see here).

I would say that having backstopjs retry-ing on error to make sure that those failures are really there would be a great fallback mechanism that could allow us to hopefully finally make use of parallel capturing

igorpavlov-zz commented 6 years ago

+1 I think this is a great feature!

deb1990 commented 6 years ago

+1 this would improve the reliability given the parallel capturing throws random errors.

garris commented 6 years ago

FWIW: the current version using the puppeteer engine resolves the parallel capture issue. That is available now.

AkA84 commented 6 years ago

@garris unfortunately it doesn't seem to be the case for me (backstopjs@3.2.16, puppeteer@1.4.0), not sure what others are reporting (was thinking about opening a separate issue about it)

I get random timeout and script errors whenever i switch to parallel capture on scripts that have a 99% success rate when doing single capture. The other 1% is again random errors that seem happen for no discernible reason even on single capture (talking about even scenarios that just require to go to a url without having to run any script on them).

That's why I would consider this a huge improvement, it would provide a safety mechanism to avoid very annoying false positives whenever you get these errors that seems you have really no control over

garris commented 6 years ago

@AkA84 -- Just to sure, can you confirm you have set "engine": "puppeteer" in your config? At work we have an overly thick web client and middle tier which we run locally while developing. When I run the chromy engine "engine": "chromy" in parallel on a test file with say 100 tests I would consistently see a few dropped scenarios. However, I can run a similar workload on a very thin client and get no dropped scenarios. When we moved to puppeteer this problem disappeared entirely -- we are now set "asyncCaptureLimit": 20. We found this to be the case across multiple testing environments here. So, this obviously points me back to the BackstopJS/Chromy integration -- or possibly backstopTools.js which runs in the client. Or maybe we have either a memory issue or a networking issue which occurs when the client is stressed.

I completely understand this retry request from a practical view. But I am also conflicted about this feature -- I feel it would be better/correct to put energy into understanding why parallel capture fails but sequential capture doesn't. Is it a state contamination issue? Is it a memory leak? Is it an OS resource? Is it network or server throttling? I would rather put the energy into understanding the root cause and create a more stable system.

I really don't want to add a feature to compensate for the fact that the backstop is broken.

OTOH -- If the retry feature was purely a request to compensate for a client app deficiency that would feel a little bit better -- especially for prod testing where we actually are do have legit variance. In our case we have backstop on a cronjob which runs tests every 5 minutes. If three in a row fail then our release engineer gets a notification.

reneolivo commented 6 years ago

:+1: for the feature request

garris commented 6 years ago

Ok. The crowd has spoken. I am happy to bring in the PR if someone puts one up.

Cheers.

AkA84 commented 6 years ago

@AkA84 -- Just to sure, can you confirm you have set "engine": "puppeteer" in your config?

@garris Yes absolutely, we've been using Puppeteer for a while now. Although I think it's important to point out that these reliability issues begun after we switched to Headless Chrome (first with Chromy and now with Puppeteer), while with PhantomJS we never encountered them.

I completely understand this retry request from a practical view. But I am also conflicted about this feature [...] I really don't want to add a feature to compensate for the fact that the backstop is broken.

Yes I completely understand you. I'm far from wanting to promote a hit-it-with-a-wrench-until-it-works kind of approach!

This to me isn't to be intended as a way to dismiss performance/reliability issues with the tool itself, but as i said it's a fallback mechanism stemmed from a pragmatic approach: with a tool that depends on so many different variables, things can go wrong for many different reasons, some of which can be outside of the user's control. It is pretty bad to see a ~20min test run fail because of some random timeout error that never happened before and that doesn't seem to be due to problems in the script or the app/site being tested, especially if this is done in the context of CI.

Just to be sure I tried parallel capturing again after your reply, and unfortunately I still see random errors popping up here and there, in every run :( Also I want to stress again that we see these random errors also in single capture, although much more rarely than when using parallel capture.

Finally I thought that maybe this is a problem that we have only with our app, so i set up a simple test with these scenarios

{
  "label": "Google 1",
  "url": "https://google.com"
},
{
  "label": "Google / Type 1",
  "url": "https://google.com",
  "onReadyScript": "type.js"
},
// ...
{
  "label": "Google 99",
  "url": "https://google.com"
},
{
  "label": "Google / Type 99",
  "url": "https://google.com",
  "onReadyScript": "type.js"
}

and first ran reference with no parallel capturing, then ran test with "asyncCaptureLimit": 7. In the first case it all went smoothly, in the second case I got errors on dozens of scenarios. I got errors also with "asyncCaptureLimit": 2,, although not as many (please let me know if you need some details about my set up)

Thanks as usual!

cactusa commented 6 years ago

My personal opinion is that this feature might be useful, but it's trying to solve a problem by avoiding it. Furthermore, if you are using deployment tools such a Jenkins you can set up your retry functionality there.

garris commented 6 years ago

@aka84 — what happens when you test a simple local page? (no JS)

What is your os & & node versions?

AkA84 commented 6 years ago

Furthermore, if you are using deployment tools such a Jenkins you can set up your retry functionality there.

@cactusa Unfortunately it's not really the same: you would end up retrying an entire 20-mins test suite just because of a single scenario that randomly errored out. This feature instead would retry only that scenario for an X (configurable) amount of times before declaring that there are indeed errors and failing the run

garris commented 6 years ago

Also, what is your hardware profile?

https://github.com/garris/BackstopJS/issues/770#issuecomment-390197942

AkA84 commented 6 years ago

@garris following this conversation i did yet again more troubleshooting and it seems (but i'm not 100% sure yet) that most (i'm not sure yet if it's all) of these random errors stem from two reasons (one is very specific to our app, the other might be of interest to you)

  1. in some scenarios the browser is being redirected to the dashboard of the CMS that our app is based on, which is fetching a remote feed to display in one of the widgets. By using debugWindow: true i could see that randomly that remote request fails (probably because of the parallel requests?) and the CMS displays a JS alerts which prevents the browser to go further, thus timing out
  2. we are relying heavily on page objects in our engine scripts: the page objects are simple Objects, not Classes that get instantiated. For that reason it seems that then all the multiple parallel Chromium instances end up using, at the same time, the same page object when running their scenarios, which leads to all sorts of random errors. When I trimmed our test suite down to only scenarios that used different page objects (basically one page object would be used only once in the entire suite), the random errors disappeared.

I'm still investigating it, but thought fair to let you know what I got so far!

(btw: I'm on macOs High Sierra 10.13.4, node v8.9.4)

Kenith commented 6 years ago

Here is my expertise to stable the backstop. Hope could be the help to you all.

For more full samples, please refer to: https://github.com/Kenith/ta-visual-lib/tree/master/Sample/Backstop

- Chromy Engine

  1. The engineOptions in backstop.json should like this, especially the timeout, which should set to 300000 (if not, when any timeout, the backstop won't capture image). Set asyncCaptureLimit to 2 if still not stable
"engine": "chromy", 
  "engineOptions": {
    "waitTimeout": 300000,
    "gotoTimeout": 300000,
    "chromeFlags": [
      "--disable-gpu", 
      "--force-device-scale-factor=1", 
      "--disable-infobars=true",
      "--hide-scrollbars"
    ]
  }, 
  "asyncCaptureLimit": 2, 

- Puppeteer Engine

  1. Suggest the following settings in backstop.json.
"engine": "puppeteer", 
  "engineOptions": {
    "ignoreHTTPSErrors": true,
    "slowMo": 500,
    "args": [
      "--no-sandbox",
      "--disable-setuid-sandbox",
      "--disable-gpu", 
      "--force-device-scale-factor=1", 
      "--disable-infobars=true",
      "--hide-scrollbars"
    ]
  }, 
  1. Add the timeout in puppet/onBefore.js to make sure Puppeteer has enough time to wait the page finish load.

await page.setDefaultNavigationTimeout(300000);

- PhantomJS

  1. Set the following settings in backstop.json
"engine": "phantomjs",
  "casperFlags": [
    "--ignore-ssl-errors=true", 
    "--ssl-protocol=any",
    "--engine=phantomjs"
  ], 
  1. Add following settings in casper/onBefore.js to avoid the PhantomJS hang issue. If not set this, the phantomjs has high possibilities to hang there forever.
casper.page.settings.resourceTimeout = 300000;
  casper.options.waitTimeout = 300000;
  casper.page.onResourceTimeout = function() {
    console.log("Phantomjs failed to donwload the resource in 5 minutes..."); 
    casper.bypass(1);
  };
mike-malandra commented 6 years ago

I have also found Chromy to be unreliable when being ran in docker, even running with asyncCaptureLimit set to 1. I simply made the switch to puppeteer and so far have had much better results.

For whoever is building this PR, can we have it default to not retrying? I'm actually against using the retry option since there is potential for it to mask legitimate issues (webserver, js issues, etc).

hiroshi commented 5 years ago

I wrote a quick hackish script that called backstop_retry.js for this purpose. It retries only once for each timed out scenario. It is ugly, but it may be better than manually rerun CI.

#!/usr/bin/env node
# Name: backstop_retry.js
# Usage example: backstopjs --config=backstop.json || backstop_retry.js backstop.json

const { execSync } = require('child_process')
const fs = require('fs')
const path = require('path')

const configJsonPath = process.argv[2]
const configJson = JSON.parse(fs.readFileSync(configJsonPath, 'utf8'))
const reportConfigPath = path.join(process.cwd(), configJson.paths.html_report, 'config.js')

global.report = function (config) {
  // Fail fast if no timeout failure found
  config.tests.forEach((test) => {
    if (test.status === 'fail' && !(test.pair.engineErrorMsg && test.pair.engineErrorMsg.match(/Navigation Timeout Exceeded: \d+ms exceeded/))) {
      process.exit(1)
    }
  })
  // Retry each timeout
  config.tests.forEach((test) => {
    if (test.status === 'fail') {
      const label = test.pair.label
      console.log(`\nRetrying a failed scenario label="${label}"...\n`)
      const cmd = `backstop test --config=${configJsonPath} --filter="${label}"`
      try {
        execSync(cmd, { stdio: 'inherit' })
      } catch (err) {
        process.exit(1)
      }
    }
  })
}

require(reportConfigPath)
TomdeHaan commented 4 years ago

@hiroshi where in backstop do I define when this is triggered?

hiroshi commented 4 years ago

where in backstop do I define when this is triggered?

Sorry, but I failed to understand your question.

For usage, see the third line of my snippet. backstopjs --config=backstop.json || backstop_retry.js backstop.json

TomdeHaan commented 4 years ago

where in backstop do I define when this is triggered?

Sorry, but I failed to understand your question.

For usage, see the third line of my snippet. backstopjs --config=backstop.json || backstop_retry.js backstop.json

You gave the correct answer :D And that's what I write in my jenkins file?

hitode909 commented 3 years ago

I made a command line tool named backstop-retry-failed-scenarios. This tool retries failed scenarios, and merge results to one result. Our team are using this tools for 9 months, and it works fine.

sreeram1985 commented 1 year ago

Hi @garris can the tool created by @hitode909 be made a part of BackstopJS please. This is very useful tool https://github.com/garris/BackstopJS/issues/761#issuecomment-800743374.