fix 'incorrect screenshot path in report output ' (close #2726) (#2853)

* fix 'incorrect screenshot path in report output ' (close #2726)

* fix tests - 1

* fix test

* fix tests 3

* remove globby

package.json

@@ -131,6 +131,7 @@
     "broken-link-checker": "^0.7.0",
     "browserstack-connector": "^0.1.5",
     "caller": "^1.0.1",
+    "chai-string": "^1.5.0",
     "connect": "^3.4.0",
     "cross-spawn": "^4.0.0",
     "dom-walk": "^0.1.1",

src/client/automation/playback/rclick.js

@@ -5,16 +5,13 @@ import { focusAndSetSelection, focusByRelatedElement } from '../utils/utils';
 import cursor from '../cursor';
 import nextTick from '../utils/next-tick';
 
-var Promise = hammerhead.Promise;
+const Promise = hammerhead.Promise;
 
-var extend         = hammerhead.utils.extend;
-var browserUtils   = hammerhead.utils.browser;
-var eventSimulator = hammerhead.eventSandbox.eventSimulator;
-
-var domUtils   = testCafeCore.domUtils;
-var eventUtils = testCafeCore.eventUtils;
-var delay      = testCafeCore.delay;
+const extend         = hammerhead.utils.extend;
+const browserUtils   = hammerhead.utils.browser;
+const eventSimulator = hammerhead.eventSandbox.eventSimulator;
 
+const { domUtils, eventUtils, delay } = testCafeCore;
 
 export default class RClickAutomation extends VisibleElementAutomation {
     constructor (element, clickOptions) {
@@ -46,10 +43,10 @@ export default class RClickAutomation extends VisibleElementAutomation {
         // NOTE: If a target element is a contentEditable element, we need to call focusAndSetSelection directly for
         // this element. Otherwise, if the element obtained by elementFromPoint is a child of the contentEditable
         // element, a selection position may be calculated incorrectly (by using the caretPos option).
-        var elementForFocus = domUtils.isContentEditableElement(this.element) ? this.element : eventArgs.element;
+        const elementForFocus = domUtils.isContentEditableElement(this.element) ? this.element : eventArgs.element;
 
         // NOTE: IE doesn't perform focus if active element has been changed while executing mousedown
-        var simulateFocus = !browserUtils.isIE || this.eventState.activeElementBeforeMouseDown === domUtils.getActiveElement();
+        const simulateFocus = !browserUtils.isIE || this.eventState.activeElementBeforeMouseDown === domUtils.getActiveElement();
 
         return focusAndSetSelection(elementForFocus, simulateFocus, this.caretPos)
             .then(() => nextTick());
@@ -74,7 +71,7 @@ export default class RClickAutomation extends VisibleElementAutomation {
     }
 
     run (useStrictElementCheck) {
-        var eventArgs = null;
+        let eventArgs = null;
 
         return this
             ._ensureElement(useStrictElementCheck)

src/client/automation/utils/get-key-code.js

@@ -1,12 +1,11 @@
 import { KEY_MAPS } from '../deps/testcafe-core';
 import isLetter from './is-letter';
 
-
 export default function (char) {
     if (isLetter(char))
         return char.toUpperCase().charCodeAt(0);
 
-    var res = KEY_MAPS.shiftMap[char] ? KEY_MAPS.shiftMap[char].charCodeAt(0) : char.charCodeAt(0);
+    const res = KEY_MAPS.shiftMap[char] ? KEY_MAPS.shiftMap[char].charCodeAt(0) : char.charCodeAt(0);
 
     return KEY_MAPS.symbolCharCodeToKeyCode[res] || res;
 }

src/client/automation/utils/get-line-rect-intersection.js

@@ -6,8 +6,8 @@ function getPointsDistance (start, end) {
 }
 
 function findLineAndRectSideIntersection (startLinePoint, endLinePoint, rectSide) {
-    var intersectionX            = null;
-    var haveIntersectionInBounds = null;
+    let intersectionX            = null;
+    let haveIntersectionInBounds = null;
 
     if (rectSide.isHorizontal) {
         intersectionX            = positionUtils.getLineXByYCoord(startLinePoint, endLinePoint, rectSide.y1);
@@ -16,25 +16,25 @@ function findLineAndRectSideIntersection (startLinePoint, endLinePoint, rectSide
         return haveIntersectionInBounds ? { x: intersectionX, y: rectSide.y1 } : null;
     }
 
-    var intersectionY = positionUtils.getLineYByXCoord(startLinePoint, endLinePoint, rectSide.x1);
+    const intersectionY = positionUtils.getLineYByXCoord(startLinePoint, endLinePoint, rectSide.x1);
 
     haveIntersectionInBounds = intersectionY && intersectionY >= rectSide.y1 && intersectionY <= rectSide.y2;
 
     return haveIntersectionInBounds ? { x: rectSide.x1, y: intersectionY } : null;
 }
 
 export default function (startLinePoint, endLinePoint, rect) {
-    var res          = [];
-    var intersection = null;
+    const res        = [];
+    let intersection = null;
 
-    var rectLines = [
+    const rectLines = [
         { x1: rect.left, y1: rect.top, x2: rect.left, y2: rect.bottom, isHorizontal: false }, // left-side
         { x1: rect.right, y1: rect.top, x2: rect.right, y2: rect.bottom, isHorizontal: false }, // right-side
         { x1: rect.left, y1: rect.top, x2: rect.right, y2: rect.top, isHorizontal: true }, // top-side
         { x1: rect.left, y1: rect.bottom, x2: rect.right, y2: rect.bottom, isHorizontal: true } // bottom-side
     ];
 
-    for (var i = 0; i < rectLines.length; i++) {
+    for (let i = 0; i < rectLines.length; i++) {
         intersection = findLineAndRectSideIntersection(startLinePoint, endLinePoint, rectLines[i]);
 
         if (intersection)

src/client/automation/utils/next-tick.js

@@ -1,9 +1,9 @@
 import hammerhead from '../deps/hammerhead';
 
-var Promise = hammerhead.Promise;
-var nativeMethods  = hammerhead.nativeMethods;
-
+const Promise       = hammerhead.Promise;
+const nativeMethods = hammerhead.nativeMethods;
 
 export default function () {
     return new Promise(resolve => nativeMethods.setTimeout.call(window, resolve, 0));
 }
+

src/screenshots/capturer.js

@@ -59,21 +59,6 @@ export default class Capturer {
         return joinPath(this.baseScreenshotsPath, path);
     }
 
-    _updateScreenshotPathForTestEntry (customPath) {
-        // NOTE: if test contains takeScreenshot action with custom path
-        // we should specify the most common screenshot folder in report
-        let screenshotPathForTestEntry = this.baseScreenshotsPath;
-
-        if (!customPath) {
-            const pathForReport = this.pathPattern.getPathForReport();
-
-            screenshotPathForTestEntry = this._joinWithBaseScreenshotPath(pathForReport);
-        }
-
-
-        this.testEntry.path = screenshotPathForTestEntry;
-    }
-
     _incrementFileIndexes (forError) {
         if (forError)
             this.pathPattern.data.errorFileIndex++;
@@ -126,8 +111,6 @@ export default class Capturer {
             await generateThumbnail(screenshotPath, thumbnailPath);
         });
 
-        this._updateScreenshotPathForTestEntry(customPath);
-
         const screenshot = {
             screenshotPath,
             thumbnailPath,

src/screenshots/index.js

@@ -2,6 +2,7 @@ import { find } from 'lodash';
 import moment from 'moment';
 import Capturer from './capturer';
 import PathPattern from './path-pattern';
+import getCommonPath from '../utils/get-common-path';
 
 export default class Screenshots {
     constructor (path, pattern) {
@@ -15,7 +16,6 @@ export default class Screenshots {
     _addTestEntry (test) {
         const testEntry = {
             test:        test,
-            path:        this.screenshotsPath || '',
             screenshots: []
         };
 
@@ -46,7 +46,10 @@ export default class Screenshots {
     }
 
     getPathFor (test) {
-        return this._getTestEntry(test).path;
+        const testEntry       = this._getTestEntry(test);
+        const screenshotPaths = testEntry.screenshots.map(screenshot => screenshot.screenshotPath);
+
+        return getCommonPath(screenshotPaths);
     }
 
     createCapturerFor (test, testIndex, quarantine, connection, warningLog) {

src/screenshots/path-pattern.js

@@ -106,12 +106,6 @@ export default class PathPattern {
         return correctFilePath(path, SCRENSHOT_EXTENTION);
     }
 
-    getPathForReport () {
-        const path = PathPattern._buildPath(DEFAULT_PATH_PATTERN_FOR_REPORT, this.placeholderToDataMap);
-
-        return correctFilePath(path);
-    }
-
     // For testing purposes
     static get PLACEHOLDERS () {
         return PLACEHOLDERS;

src/utils/get-common-path.js

@@ -0,0 +1,25 @@
+import path from 'path';
+
+export default function (paths) {
+    if (!paths)
+        return null;
+
+    if (paths.length === 1)
+        return paths[0];
+
+    const pathArrs               = paths.map(item => item.split(path.sep));
+    const isCommonPathFragment   = (pathFragment, idx) => pathArrs.every(pathArray => pathArray[idx] === pathFragment);
+    const firstPathArr           = pathArrs[0];
+    let commonPathFramgemtnIndex = 0;
+
+    while (commonPathFramgemtnIndex < firstPathArr.length &&
+           isCommonPathFragment(firstPathArr[commonPathFramgemtnIndex], commonPathFramgemtnIndex))
+        commonPathFramgemtnIndex++;
+
+    if (!commonPathFramgemtnIndex)
+        return null;
+
+    const commonPathFragments = firstPathArr.slice(0, commonPathFramgemtnIndex);
+
+    return path.join(...commonPathFragments);
+}

test/functional/fixtures/api/es-next/take-screenshot/test.js

@@ -1,16 +1,19 @@
 const path            = require('path');
 const fs              = require('fs');
-const expect          = require('chai').expect;
+const chai            = require('chai');
+const { expect }      = chai;
 const config          = require('../../../../config.js');
 const assertionHelper = require('../../../../assertion-helper.js');
 
+chai.use(require('chai-string'));
+
 const SCREENSHOTS_PATH                   = assertionHelper.SCREENSHOTS_PATH;
 const THUMBNAILS_DIR_NAME                = assertionHelper.THUMBNAILS_DIR_NAME;
-const SCREENSHOT_PATH_MESSAGE_RE         = /^___test-screenshots___[\\/]\d{4,4}-\d{2,2}-\d{2,2}_\d{2,2}-\d{2,2}-\d{2,2}[\\/]test-1$/;
+const SCREENSHOT_PATH_MESSAGE_RE         = /^___test-screenshots___[\\/]\d{4,4}-\d{2,2}-\d{2,2}_\d{2,2}-\d{2,2}-\d{2,2}[\\/]test-1/;
 const SCREENSHOT_ON_FAIL_PATH_MESSAGE_RE = /^.*run-1/;
 const SLASH_RE                           = /[\\/]/g;
 
-var getReporter = function (scope) {
+const getReporter = function (scope) {
     const userAgents = { };
 
     function patchScreenshotPath (screenshotPath) {
@@ -60,7 +63,8 @@ describe('[API] t.takeScreenshot()', function () {
             return runTests('./testcafe-fixtures/take-screenshot.js', 'Take a screenshot with a custom path (OS separator)',
                 { setScreenshotPath: true })
                 .then(function () {
-                    expect(testReport.screenshotPath).eql(SCREENSHOTS_PATH);
+
+                    expect(testReport.screenshotPath).startsWith(path.join(SCREENSHOTS_PATH, 'custom'));
 
                     const screenshotsCheckingOptions = { forError: false, screenshotsCount: 2, customPath: 'custom' };
 
@@ -214,7 +218,7 @@ describe('[API] t.takeScreenshot()', function () {
                     only:                  'chrome'
                 })
                 .then(() => {
-                    expect(SCREENSHOT_PATH_MESSAGE_RE.test(testReport.screenshotPath)).eql(true);
+                    expect(testReport.screenshotPath).eql(SCREENSHOTS_PATH);
 
                     const screenshot1Path = path.join(assertionHelper.SCREENSHOTS_PATH, 'Take a screenshot-1.png');
                     const screenshot2Path = path.join(assertionHelper.SCREENSHOTS_PATH, 'Take a screenshot-2.png');

test/functional/fixtures/api/legacy/smoke/test.js

@@ -1,41 +1,39 @@
-var expect          = require('chai').expect;
-var globby          = require('globby').sync;
-var path            = require('path');
-var config          = require('../../../../config');
-var assertionHelper = require('../../../../assertion-helper');
+const { expect }       = require('chai');
+const path             = require('path');
+const config           = require('../../../../config');
+const assertionHelper  = require('../../../../assertion-helper');
 
+const SCREENSHOT_PATH_MESSAGE_RE = /^___test-screenshots___[\\/]\d{4,4}-\d{2,2}-\d{2,2}_\d{2,2}-\d{2,2}-\d{2,2}[\\/]test-1/;
+const ERROR_SCREENSHOT_PATH_RE   = /Screenshot: ___test-screenshots___[\\/]\d{4,4}-\d{2,2}-\d{2,2}_\d{2,2}-\d{2,2}-\d{2,2}[\\/]test-1[\\/]\S+[\\/]errors[\\/]\d.png/;
 
-var SCREENSHOT_PATH_MESSAGE_RE = /^___test-screenshots___[\\/]\d{4,4}-\d{2,2}-\d{2,2}_\d{2,2}-\d{2,2}-\d{2,2}[\\/]test-1$/;
-var ERROR_SCREENSHOT_PATH_RE   = /Screenshot: ___test-screenshots___[\\/]\d{4,4}-\d{2,2}-\d{2,2}_\d{2,2}-\d{2,2}-\d{2,2}[\\/]test-1[\\/]\S+[\\/]errors[\\/]\d.png/;
-
-describe('[Legacy] Smoke tests', function () {
-    it('Should run basic tests', function () {
-        return runTests(globby(path.join(__dirname, './testcafe-fixtures/basic/*test.js')), null, { skip: 'iphone,ipad' });
+describe('[Legacy] Smoke tests', () => {
+    it('Should run basic tests', () => {
+        return runTests(path.join(__dirname, './testcafe-fixtures/basic/*test.js'), null, { skip: 'iphone,ipad' });
     });
 
-    it('Should fail on errors', function () {
+    it('Should fail on errors', () => {
         return runTests('./testcafe-fixtures/errors.test.js', null, { shouldFail: true, skip: 'iphone,ipad' })
-            .catch(function (errs) {
+            .catch(errs => {
                 expect(errs[0]).contains('A target element of the click action has not been found in the DOM tree.');
             });
     });
 
     if (config.useLocalBrowsers) {
-        describe('Screenshots', function () {
+        describe('Screenshots', () => {
             afterEach(assertionHelper.removeScreenshotDir);
 
-            it('Should take a screenshot', function () {
+            it('Should take a screenshot', () => {
                 return runTests('./testcafe-fixtures/screenshots.test.js', 'Take a screenshot', { setScreenshotPath: true })
-                    .then(function () {
+                    .then(() => {
                         expect(SCREENSHOT_PATH_MESSAGE_RE.test(testReport.screenshotPath)).eql(true);
                         expect(assertionHelper.checkScreenshotsCreated({ forError: false, screenshotsCount: 2 })).eql(true);
                     });
             });
 
-            it('Should take a screenshot if an error in test code is raised', function () {
+            it('Should take a screenshot if an error in test code is raised', () => {
                 return runTests('./testcafe-fixtures/screenshots.test.js', 'Screenshot on test code error',
                     { shouldFail: true, screenshotsOnFails: true, setScreenshotPath: true })
-                    .catch(function (errs) {
+                    .catch(errs => {
                         assertionHelper.errorInEachBrowserContainsRegExp(errs, ERROR_SCREENSHOT_PATH_RE, 0);
                         expect(assertionHelper.checkScreenshotsCreated({ forError: true })).eql(true);
                     });

test/server/util-test.js

@@ -7,6 +7,7 @@ const escapeUserAgent                  = require('../../lib/utils/escape-user-ag
 const parseFileList                    = require('../../lib/utils/parse-file-list');
 const TempDirectory                    = require('../../lib/utils/temp-directory');
 const { replaceLeadingSpacesWithNbsp } = require('../../lib/utils/string');
+const getCommonPath                    = require('../../lib/utils/get-common-path');
 
 describe('Utils', () => {
     it('Correct File Path', () => {
@@ -128,4 +129,16 @@ describe('Utils', () => {
         expect(replaceLeadingSpacesWithNbsp(' test1 test2 ')).eql(' test1 test2 ');
         expect(replaceLeadingSpacesWithNbsp('  test1\n test2 \r\ntest3 ')).eql('  test1\n test2 \r\ntest3 ');
     });
+
+    it('Get common path', () => {
+        const pathFragemts = [ 'home', 'user1', 'tmp' ];
+        const path1        = path.join(...pathFragemts);
+        const path2        = path.join(pathFragemts[0], pathFragemts[1]);
+        const path3        = path.join(pathFragemts[0], pathFragemts[2]);
+
+        expect(getCommonPath([path1])).eql(path1);
+        expect(getCommonPath([path1, path1])).eql(path1);
+        expect(getCommonPath([path1, path2])).eql(path2);
+        expect(getCommonPath([path1, path2, path3])).eql(pathFragemts[0]);
+    });
 });