From 158695b099dce51a0495b2c269449a72eb701672 Mon Sep 17 00:00:00 2001 From: Mikhail Losev Date: Fri, 14 Sep 2018 12:40:07 +0300 Subject: [PATCH] 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 | 1 + src/client/automation/playback/rclick.js | 19 +++++------ src/client/automation/utils/get-key-code.js | 3 +- .../utils/get-line-rect-intersection.js | 14 ++++---- src/client/automation/utils/next-tick.js | 6 ++-- src/screenshots/capturer.js | 17 ---------- src/screenshots/index.js | 7 ++-- src/screenshots/path-pattern.js | 6 ---- src/utils/get-common-path.js | 25 ++++++++++++++ .../api/es-next/take-screenshot/test.js | 14 +++++--- .../fixtures/api/legacy/smoke/test.js | 34 +++++++++---------- test/server/util-test.js | 13 +++++++ 12 files changed, 88 insertions(+), 71 deletions(-) create mode 100644 src/utils/get-common-path.js diff --git a/package.json b/package.json index 94e603dd832..78c06c586d5 100644 --- a/package.json +++ b/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", diff --git a/src/client/automation/playback/rclick.js b/src/client/automation/playback/rclick.js index f779e22913a..f11bf46e942 100644 --- a/src/client/automation/playback/rclick.js +++ b/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) diff --git a/src/client/automation/utils/get-key-code.js b/src/client/automation/utils/get-key-code.js index dddb2ce47ef..6baf48d8765 100644 --- a/src/client/automation/utils/get-key-code.js +++ b/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; } diff --git a/src/client/automation/utils/get-line-rect-intersection.js b/src/client/automation/utils/get-line-rect-intersection.js index a79b14571ee..5a48ce24b48 100644 --- a/src/client/automation/utils/get-line-rect-intersection.js +++ b/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,7 +16,7 @@ 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; @@ -24,17 +24,17 @@ function findLineAndRectSideIntersection (startLinePoint, endLinePoint, rectSide } 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) diff --git a/src/client/automation/utils/next-tick.js b/src/client/automation/utils/next-tick.js index bc21cfeafd6..369d7908fe9 100644 --- a/src/client/automation/utils/next-tick.js +++ b/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)); } + diff --git a/src/screenshots/capturer.js b/src/screenshots/capturer.js index 9adc6799f29..cd8c9756770 100644 --- a/src/screenshots/capturer.js +++ b/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, diff --git a/src/screenshots/index.js b/src/screenshots/index.js index df6f5cf2eb1..c59bf18c7e2 100644 --- a/src/screenshots/index.js +++ b/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) { diff --git a/src/screenshots/path-pattern.js b/src/screenshots/path-pattern.js index 59bc880bd68..51c1f6e12ac 100644 --- a/src/screenshots/path-pattern.js +++ b/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; diff --git a/src/utils/get-common-path.js b/src/utils/get-common-path.js new file mode 100644 index 00000000000..8bc5deb4316 --- /dev/null +++ b/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); +} diff --git a/test/functional/fixtures/api/es-next/take-screenshot/test.js b/test/functional/fixtures/api/es-next/take-screenshot/test.js index b5a0cd333a5..a1cfb8b8582 100644 --- a/test/functional/fixtures/api/es-next/take-screenshot/test.js +++ b/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'); diff --git a/test/functional/fixtures/api/legacy/smoke/test.js b/test/functional/fixtures/api/legacy/smoke/test.js index 748bb8a3d1e..a9f9e5ee5c6 100644 --- a/test/functional/fixtures/api/legacy/smoke/test.js +++ b/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); }); diff --git a/test/server/util-test.js b/test/server/util-test.js index aec2ebd7800..1c57f842c16 100644 --- a/test/server/util-test.js +++ b/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]); + }); });