Added option cliExitOnEngineError to bubble up if engine has an error… #1127

mummybot · 2019-12-04T21:47:21Z

Exposing engine errors
By default if there is an error in the engine e.g. missing selector, it won't exit BackstopJS. This is normally fine as the screenshots will pick up any unexpected differences. If you want to terminate BackstopJS on engine errors set the option flag:

"cliExitOnEngineError": true

mummybot · 2019-12-08T19:34:39Z

Hi @garris, sorry to push but do you think you will merge this PR at some point? Our pipeline is currently being held up by this, and if it is not going to be merged I need to find another way to progress the pipeline (probably by having Jenkins checking the errors folder).

garris · 2019-12-09T00:44:47Z

Hi there @mummybot -- apologies for the delay.

I believe backstop is correctly reporting errors in cases where something in a scenario setup goes wrong.

Can you help me understand your situation better so we can figure out the right approach?

mummybot · 2019-12-09T01:41:37Z

Hi, thanks for the reply. It does indeed report errors fine, at both back stop and underlying engine issues. However we're finding that when running in a Jenkins job, which only errors if an exit code of 1 is returned, when running on reference, if there is an engine error such as missing selector, then the backstop job completes with success, albeit there are then error images in the reference folder. We need the ability to fail the backstop job if there are engine errors, so as to fail our build pipeline. A specific case, but hey ho.

mummybot · 2019-12-09T08:36:05Z

I've uploaded an example of what I have solved:

In examples/responsiveDemo
A failed selector in the engine but BackstopJS exits successfully executes with error code 0.

SCENARIO > comparePage
SCENARIO > comparePage
BackstopException: comparePage on tablet: TimeoutError: waiting for selector "#missing" failed: timeout 30000ms exceeded
BackstopException: comparePage on phone: TimeoutError: waiting for selector "#missing" failed: timeout 30000ms exceeded

Run `$ backstop test` to generate diff report.

      COMMAND | Command "reference" successfully executed in [33.308s]
responsiveDemo $

In examples/responsiveDemoFixed
Failed selector in the engine causes an exit code error of 1 in BackstopJS with cliExitOnEngineFail flag set.

BackstopException: comparePage on tablet: TimeoutError: waiting for selector "#missing" failed: timeout 30000ms exceeded
BackstopException: comparePage on phone: TimeoutError: waiting for selector "#missing" failed: timeout 30000ms exceeded
      COMMAND | Command "reference" ended with an error after [31.506s]
      COMMAND | [object Object]
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] dev-backstop-reference: `node ../../cli/index.js reference`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] dev-backstop-reference script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/francis/.npm/_logs/2019-12-09T08_23_40_348Z-debug.log

garris · 2019-12-09T08:44:13Z

Ok. Got it. I understand your thinking here -- but may I ask that we not short-circuit the bitmap generation here. The errors that occur during this flow should still propagate up. Would it be possible for you to handle this the same way that test handles it? (Only without actually comparing files). You can see how this is done in core/util/compare/index.js -- The code is unfortunately really undocumented -- could use some inline comments.

Please let me know if that would be possible. Thanks!

mummybot · 2019-12-09T08:49:58Z

I'm in London, so will look at this my tonight after work. If I figure out what you are asking, would you still want it as an optional condition, or would you want that as the default functionality?

garris · 2019-12-09T15:24:57Z

Sounds good. This would be an improvement which I don't expect would break existing implementations -- so I am thinking default functionality.

mummybot · 2019-12-09T23:05:33Z

I added another command engineErrors to be run after the reference command completes. It wasn't required for 'test' or its subsequent commands as they would error already. What do you think?

garris

Ok, good start. Almost there. Please implement this as an imported utility script as opposed to a new command. See comments -- hopefully this will make sense. Thanks!

garris · 2019-12-10T05:30:11Z

core/command/index.js

@@ -20,7 +20,8 @@ var commandNames = [
  'test',
  'approve',
  'version',
-  'stop'
+  'stop',
+  'engineErrors'


I think you should be able to just add this as a regular module (from /core/util). We should avoid creating this as a command since this would never be run alone.

garris · 2019-12-10T05:30:39Z

core/command/reference.js

@@ -5,6 +5,7 @@ const { shouldRunDocker, runDocker } = require('../util/runDocker');

 module.exports = {
  execute: function (config) {
+    const executeCommand = require('./index');


Yup, agreed

garris · 2019-12-10T05:31:25Z

core/command/reference.js

@@ -22,6 +23,7 @@ module.exports = {
        return createBitmaps(config, true);
      }).then(function () {
        console.log('\nRun `$ backstop test` to generate diff report.\n');
+        return executeCommand('_engineErrors', config);


you should be able to just run a plain old module. like here... https://github.com/garris/BackstopJS/blob/master/core/command/test.js#L10

garris · 2019-12-10T05:37:00Z

core/command/engineErrors.js

@@ -0,0 +1,13 @@
+module.exports = {
+  execute: function (config) {


You can just export the anon function here. Skip the execute property and the object all together.

mummybot · 2019-12-10T08:44:15Z

Added test for engineErrors

garris

Beautiful! Thank you.

mummybot · 2019-12-10T17:39:17Z

Can you let me know here when you bump the minor version number so I can pull it in? And thanks for your comments, very helpful. BackstopJS is a great library!

garris · 2019-12-10T17:43:14Z

published to [email protected]

npm install backstopjs@canary

Please validate when you get a chance -- thanks!

mummybot · 2019-12-10T19:17:49Z

Awesome, I'll try it tomorrow, finished for the night.

mummybot · 2019-12-11T13:15:39Z

Worked a treat, thank you very much!

mummybot force-pushed the cli-exit-on-engine-fail branch from 25985c4 to e8b6fd5 Compare December 9, 2019 22:58

Added engineError command for failing on reference

4b485e3

mummybot force-pushed the cli-exit-on-engine-fail branch from e8b6fd5 to 4b485e3 Compare December 9, 2019 23:02

garris requested changes Dec 10, 2019

View reviewed changes

Francis Saul added 2 commits December 10, 2019 07:11

reference engineError command => util

01a4b7f

core/util/engineErrors test

6d36b4a

garris approved these changes Dec 10, 2019

View reviewed changes

garris merged commit bc6cc0a into garris:master Dec 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option cliExitOnEngineError to bubble up if engine has an error… #1127

Added option cliExitOnEngineError to bubble up if engine has an error… #1127

mummybot commented Dec 4, 2019

mummybot commented Dec 8, 2019

garris commented Dec 9, 2019 •

edited

Loading

mummybot commented Dec 9, 2019

mummybot commented Dec 9, 2019

garris commented Dec 9, 2019

mummybot commented Dec 9, 2019

garris commented Dec 9, 2019

mummybot commented Dec 9, 2019

garris left a comment

garris Dec 10, 2019

mummybot Dec 10, 2019

garris Dec 10, 2019

mummybot Dec 10, 2019

garris Dec 10, 2019

mummybot Dec 10, 2019

garris Dec 10, 2019

mummybot Dec 10, 2019

mummybot commented Dec 10, 2019

garris left a comment

mummybot commented Dec 10, 2019

garris commented Dec 10, 2019

mummybot commented Dec 10, 2019

mummybot commented Dec 11, 2019

		@@ -0,0 +1,13 @@
		module.exports = {
		execute: function (config) {

Added option cliExitOnEngineError to bubble up if engine has an error… #1127

Added option cliExitOnEngineError to bubble up if engine has an error… #1127

Conversation

mummybot commented Dec 4, 2019

mummybot commented Dec 8, 2019

garris commented Dec 9, 2019 • edited Loading

mummybot commented Dec 9, 2019

mummybot commented Dec 9, 2019

garris commented Dec 9, 2019

mummybot commented Dec 9, 2019

garris commented Dec 9, 2019

mummybot commented Dec 9, 2019

garris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mummybot commented Dec 10, 2019

garris left a comment

Choose a reason for hiding this comment

mummybot commented Dec 10, 2019

garris commented Dec 10, 2019

mummybot commented Dec 10, 2019

mummybot commented Dec 11, 2019

garris commented Dec 9, 2019 •

edited

Loading