The onActionDone method should take TestRunErrorFormattableAdapter instance instead of Error (closes #4867)

src/reporter/index.js

@@ -2,7 +2,6 @@ import { find, sortBy, union } from 'lodash';
 import { writable as isWritableStream } from 'is-stream';
 import ReporterPluginHost from './plugin-host';
 import formatCommand from './command/format-command';
-import TestCafeErrorList from '../errors/error-list';
 
 export default class Reporter {
     constructor (plugin, task, outStream, name) {
@@ -135,14 +134,11 @@ export default class Reporter {
         reportItem.pendingTestRunDonePromise.resolve();
     }
 
-    _prepareReportTestActionEventArgs ({ command, result, testRun, errors }) {
+    _prepareReportTestActionEventArgs ({ command, result, testRun, err }) {
         const args = {};
 
-        if (errors) {
-            errors = errors instanceof TestCafeErrorList ? errors.items : [errors];
-
-            args.errors = errors;
-        }
+        if (err)
+            args.err = err;
 
         return Object.assign(args, {
             testRunId: testRun.id,
@@ -218,9 +214,9 @@ export default class Reporter {
             }
         });
 
-        task.on('test-action-done', async ({ apiActionName, command, result, testRun, errors }) => {
+        task.on('test-action-done', async ({ apiActionName, command, result, testRun, err }) => {
             if (this.plugin.reportTestActionDone) {
-                const args = this._prepareReportTestActionEventArgs({ command, result, testRun, errors });
+                const args = this._prepareReportTestActionEventArgs({ command, result, testRun, err });
 
                 await this.plugin.reportTestActionDone(apiActionName, args);
             }

src/test-run/index.js

@@ -42,6 +42,7 @@ import {
 } from './commands/utils';
 
 import { TEST_RUN_ERRORS } from '../errors/types';
+import processTestFnError from '../errors/process-test-fn-error';
 
 const lazyRequire                 = require('import-lazy')(require);
 const SessionController           = lazyRequire('./session-controller');
@@ -294,16 +295,15 @@ export default class TestRun extends AsyncEventEmitter {
             await fn(this);
         }
         catch (err) {
-            let screenshotPath = null;
+            await this._makeScreenshotOnFail();
 
-            const { screenshots } = this.opts;
+            this.addError(err);
 
-            if (screenshots && screenshots.takeOnFails)
-                screenshotPath = await this.executeCommand(new browserManipulationCommands.TakeScreenshotOnFailCommand());
-
-            this.addError(err, screenshotPath);
             return false;
         }
+        finally {
+            this.errScreenshotPath = null;
+        }
 
         return !this._addPendingPageErrorIfAny();
     }
@@ -378,19 +378,19 @@ export default class TestRun extends AsyncEventEmitter {
         return false;
     }
 
-    _createErrorAdapter (err, screenshotPath) {
+    _createErrorAdapter (err) {
         return new TestRunErrorFormattableAdapter(err, {
             userAgent:      this.browserConnection.userAgent,
-            screenshotPath: screenshotPath || '',
+            screenshotPath: this.errScreenshotPath || '',
             testRunPhase:   this.phase
         });
     }
 
-    addError (err, screenshotPath) {
+    addError (err) {
         const errList = err instanceof TestCafeErrorList ? err.items : [err];
 
         errList.forEach(item => {
-            const adapter = this._createErrorAdapter(item, screenshotPath);
+            const adapter = this._createErrorAdapter(item);
 
             this.errs.push(adapter);
         });
@@ -617,8 +617,9 @@ export default class TestRun extends AsyncEventEmitter {
     }
 
     async executeAction (actionName, command, callsite) {
-        let error  = null;
-        let result = null;
+        let errorAdapter = null;
+        let error        = null;
+        let result       = null;
 
         await this.emitActionStart(actionName, command);
 
@@ -627,9 +628,18 @@ export default class TestRun extends AsyncEventEmitter {
         }
         catch (err) {
             error = err;
+
+            // NOTE: check if error is TestCafeErrorList is specific for the `useRole` action
+            // if error is TestCafeErrorList we do not need to create an adapter,
+            // since error is already was processed in role initializer
+            if (!(err instanceof TestCafeErrorList)) {
+                await this._makeScreenshotOnFail();
+
+                errorAdapter = this._createErrorAdapter(processTestFnError(err));
+            }
         }
 
-        await this.emitActionDone(actionName, command, result, error);
+        await this.emitActionDone(actionName, command, result, errorAdapter);
 
         if (error)
             throw error;
@@ -709,6 +719,13 @@ export default class TestRun extends AsyncEventEmitter {
         return Promise.reject(err);
     }
 
+    async _makeScreenshotOnFail () {
+        const { screenshots } = this.opts;
+
+        if (!this.errScreenshotPath && screenshots && screenshots.takeOnFails)
+            this.errScreenshotPath = await this.executeCommand(new browserManipulationCommands.TakeScreenshotOnFailCommand());
+    }
+
     _decorateWithFlag (fn, flagName, value) {
         return async () => {
             this[flagName] = value;
@@ -844,9 +861,9 @@ export default class TestRun extends AsyncEventEmitter {
             await this.emit('action-start', { command, apiActionName });
     }
 
-    async emitActionDone (apiActionName, command, result, errors) {
+    async emitActionDone (apiActionName, command, result, err) {
         if (!this.preventEmitActionEvents)
-            await this.emit('action-done', { command, apiActionName, result, errors });
+            await this.emit('action-done', { command, apiActionName, result, err });
     }
 }
 

test/functional/fixtures/reporter/reporter.js

@@ -48,7 +48,7 @@ function generateReporter (log, options = {}) {
                 log.push(item);
             },
 
-            async reportTestActionDone (name, { command, test, fixture, errors }) {
+            async reportTestActionDone (name, { command, test, fixture, err }) {
                 if (!emitOnDone)
                     return;
 
@@ -57,8 +57,8 @@ function generateReporter (log, options = {}) {
 
                 const item = { name, action: 'done', command };
 
-                if (errors && errors.length)
-                    item.errors = errors.map(err => err.code);
+                if (err)
+                    item.err = err.code;
 
                 if (includeTestInfo) {
                     if (test.id) {

test/functional/fixtures/reporter/test.js

@@ -308,7 +308,7 @@ describe('Reporter', () => {
                                 options:  new ClickOptions(),
                                 selector: 'Selector(\'#non-existing-target\')'
                             },
-                            errors: ['E24']
+                            err: 'E24'
                         }
                     ]);
                 });
@@ -541,7 +541,7 @@ describe('Reporter', () => {
                                 selector: 'Selector(\'#non-existing-element\')',
                                 type:     'click'
                             },
-                            errors: ['E24']
+                            err: 'E24'
                         },
                         {
                             name:    'useRole',
@@ -553,8 +553,7 @@ describe('Reporter', () => {
                                     phase:     'initialized'
                                 },
                                 type: 'useRole'
-                            },
-                            errors: ['E24']
+                            }
                         }
                     ]);
                 });
@@ -675,5 +674,40 @@ describe('Reporter', () => {
         });
     });
 
+    describe('Screenshot errors', () => {
+        it('Screenshot on action error', () => {
+            let testDoneErrors     = null;
+            const actionDoneErrors = [];
+
+            function screenshotReporter () {
+                return {
+                    async reportTestActionDone (name, { err }) {
+                        actionDoneErrors.push(err);
+                    },
+                    async reportTaskStart () {
+                    },
+                    async reportFixtureStart () {
+                    },
+                    async reportTestDone (name, testRunInfo) {
+                        testDoneErrors = testRunInfo.errs;
+                    },
+                    async reportTaskDone () {
+                    }
+                };
+            }
 
+            return runTests('testcafe-fixtures/index-test.js', 'Screenshot on action error', {
+                only:               'chrome',
+                reporter:           screenshotReporter,
+                screenshotsOnFails: true
+            })
+                .then(() => {
+                    expect(actionDoneErrors[0]).is.undefined;
+                    expect(actionDoneErrors[1].code).eql('E24');
+                    expect(testDoneErrors.map(err => err.code)).eql(['E24', 'E8']);
+                    expect(testDoneErrors[0].screenshotPath).is.not.empty;
+                    expect(testDoneErrors[0].screenshotPath).eql(testDoneErrors[1].screenshotPath);
+                });
+        });
+    });
 });

test/functional/fixtures/reporter/testcafe-fixtures/index-test.js

@@ -63,3 +63,9 @@ test('Client Function', async () => {
 test('Eval', async t => {
     await t.eval(() => document.getElementById('#target'));
 });
+
+test('Screenshot on action error', async t => {
+    t.hover('body');
+
+    await t.click('#unexisting-element');
+});

test/server/test-controller-events-test.js

@@ -1,11 +1,12 @@
-const expect            = require('chai').expect;
-const AsyncEventEmitter = require('../../lib/utils/async-event-emitter');
-const TestRun           = require('../../lib/test-run');
-const TestController    = require('../../lib/api/test-controller');
-const Task              = require('../../lib/runner/task');
-const BrowserJob        = require('../../lib/runner/browser-job');
-const Reporter          = require('../../lib/reporter');
-const { Role }          = require('../../lib/api/exportable-lib');
+const expect                         = require('chai').expect;
+const AsyncEventEmitter              = require('../../lib/utils/async-event-emitter');
+const TestRun                        = require('../../lib/test-run');
+const TestController                 = require('../../lib/api/test-controller');
+const Task                           = require('../../lib/runner/task');
+const BrowserJob                     = require('../../lib/runner/browser-job');
+const Reporter                       = require('../../lib/reporter');
+const { Role }                       = require('../../lib/api/exportable-lib');
+const TestRunErrorFormattableAdapter = require('../../lib/errors/test-run/formattable-adapter');
 
 class TestRunMock extends TestRun {
     constructor () {
@@ -175,10 +176,12 @@ describe('TestController action events', () => {
 
     it('Error action', () => {
         let actionResult = null;
+        let errorAdapter = null;
 
         initializeReporter({
-            async reportTestActionDone (name, { command, errors }) {
-                actionResult = { name, command: command.type, err: errors[0].message };
+            async reportTestActionDone (name, { command, err }) {
+                errorAdapter = err;
+                actionResult = { name, command: command.type, err: err.errMsg };
             }
         });
 
@@ -191,8 +194,9 @@ describe('TestController action events', () => {
                 throw new Error();
             })
             .catch(err => {
+                expect(errorAdapter).instanceOf(TestRunErrorFormattableAdapter);
                 expect(err.message).eql('test error');
-                expect(actionResult).eql({ name: 'click', command: 'click', err: 'test error' });
+                expect(actionResult).eql({ name: 'click', command: 'click', err: 'Error: test error' });
             });
     });
 });