Fixed "${TEST_INDEX} is initialized to 'null' for video recording path pattern" (close #3455)

src/configuration/index.js

@@ -12,6 +12,7 @@ import renderTemplate from '../utils/render-template';
 import prepareReporters from '../utils/prepare-reporters';
 import WARNING_MESSAGES from '../notifications/warning-message';
 import log from '../cli/log';
+import getConcatenatedValuesString from '../utils/get-concatenated-values-string';
 
 import {
     DEFAULT_TIMEOUT,
@@ -227,7 +228,7 @@ export default class Configuration {
         if (!this._overridenOptions.length)
             return;
 
-        const optionsStr    = this._overridenOptions.map(option => `"${option}"`).join(', ');
+        const optionsStr    = getConcatenatedValuesString(this._overridenOptions);
         const optionsSuffix = this._overridenOptions.length > 1 ? 's' : '';
 
         Configuration._showConsoleWarning(renderTemplate(WARNING_MESSAGES.configOptionsWereOverriden, optionsStr, optionsSuffix));

src/notifications/warning-message.js

@@ -22,5 +22,10 @@ export default {
 
     cannotFindConfigurationFile: 'Unable to find the "{path}" configuration file. Error details:\n' +
                                  '\n' +
-                                 '{err}'
+                                 '{err}',
+
+    problematicPathPatternPlaceholderForVideoRecording: '{placeholderList} path pattern placeholder{suffix} {verb} not suitable for the video recording\'s "pathPattern" option.\n' +
+                                                        '\n' +
+                                                        '{pronoun} value{suffix} will be replaced with an empty string.'
 };
+

src/runner/task.js

@@ -69,7 +69,7 @@ export default class Task extends AsyncEventEmitter {
     _createVideoRecorders (browserJobs) {
         const videoOptions = { timeStamp: this.timeStamp, ...this.opts.videoOptions };
 
-        return browserJobs.map(browserJob => new VideoRecorder(browserJob, this.opts.videoPath, videoOptions, this.opts.videoEncodingOptions));
+        return browserJobs.map(browserJob => new VideoRecorder(browserJob, this.opts.videoPath, videoOptions, this.opts.videoEncodingOptions, this.warningLog));
     }
 
     // API

src/utils/get-concatenated-values-string.js

@@ -0,0 +1,3 @@
+export default function (array) {
+    return array.map(item => `"${item}"`).join(', ');
+}

src/utils/path-pattern.js

@@ -1,12 +1,15 @@
 import { escapeRegExp as escapeRe } from 'lodash';
 import correctFilePath from '../utils/correct-file-path';
 import escapeUserAgent from '../utils/escape-user-agent';
+import EventEmitter from 'events';
 
 const DATE_FORMAT = 'YYYY-MM-DD';
 const TIME_FORMAT = 'HH-mm-ss';
 
 const ERRORS_FOLDER = 'errors';
 
+const PROBLEMATIC_PLACEHOLDER_VALUE = '';
+
 const PLACEHOLDERS = {
     DATE:               '${DATE}',
     TIME:               '${TIME}',
@@ -30,8 +33,10 @@ const DEFAULT_PATH_PATTERN_FOR_REPORT = `${PLACEHOLDERS.DATE}_${PLACEHOLDERS.TIM
 const TEST_ID_TEMPLATE = data => data.testIndex ? `test-${data.testIndex}` : '';
 const RUN_ID_TEMPLATE  = data => data.quarantineAttempt ? `run-${data.quarantineAttempt}` : '';
 
-export default class PathPattern {
+export default class PathPattern extends EventEmitter {
     constructor (pattern, fileExtension, data) {
+        super();
+
         this.pattern              = this._ensurePattern(pattern);
         this.data                 = this._addDefaultFields(data);
         this.placeholderToDataMap = this._createPlaceholderToDataMap();
@@ -77,8 +82,9 @@ export default class PathPattern {
         };
     }
 
-    static _buildPath (pattern, placeholderToDataMap, forError) {
-        let resultFilePath = pattern;
+    _buildPath (pattern, placeholderToDataMap, forError) {
+        let resultFilePath            = pattern;
+        const problematicPlaceholders = [];
 
         for (const placeholder in placeholderToDataMap) {
             const findPlaceholderRegExp = new RegExp(escapeRe(placeholder), 'g');
@@ -100,15 +106,26 @@ export default class PathPattern {
                     return escapeUserAgent(userAgent);
                 }
 
-                return placeholderToDataMap[placeholder];
+                let calculatedValue = placeholderToDataMap[placeholder];
+
+                if (calculatedValue === null || calculatedValue === void 0) {
+                    problematicPlaceholders.push(placeholder);
+
+                    calculatedValue = PROBLEMATIC_PLACEHOLDER_VALUE;
+                }
+
+                return calculatedValue;
             });
         }
 
+        if (problematicPlaceholders.length)
+            this.emit('problematic-placeholders-found', { placeholders: problematicPlaceholders });
+
         return resultFilePath;
     }
 
     getPath (forError) {
-        const path = PathPattern._buildPath(this.pattern, this.placeholderToDataMap, forError);
+        const path = this._buildPath(this.pattern, this.placeholderToDataMap, forError);
 
         return correctFilePath(path, this.fileExtension);
     }

src/video-recorder/index.js

@@ -7,7 +7,7 @@ import VideoRecorderProcess from './process';
 import TempDirectory from '../utils/temp-directory';
 import PathPattern from '../utils/path-pattern';
 import WARNING_MESSAGES from '../notifications/warning-message';
-
+import getConcatenatedValuesString from '../utils/get-concatenated-values-string';
 
 const DEBUG_LOGGER = debug('testcafe:video-recorder');
 
@@ -21,7 +21,7 @@ const TEMP_MERGE_CONFIG_FILE_PREFIX    = 'config';
 const TEMP_MERGE_CONFIG_FILE_EXTENSION = 'txt';
 
 export default class VideoRecorder {
-    constructor (browserJob, basePath, opts, encodingOpts) {
+    constructor (browserJob, basePath, opts, encodingOpts, warningLog) {
         this.browserJob        = browserJob;
         this.basePath          = basePath;
         this.failedOnly        = opts.failedOnly;
@@ -31,6 +31,8 @@ export default class VideoRecorder {
         this.timeStamp         = opts.timeStamp;
         this.encodingOptions   = encodingOpts;
 
+        this.warningLog = warningLog;
+
         this.tempDirectory       = new TempDirectory(TEMP_DIR_PREFIX);
         this.tempVideoPath       = '';
         this.tempMergeConfigPath = '';
@@ -63,6 +65,15 @@ export default class VideoRecorder {
         browserJob.on('test-run-before-done', this._createSafeListener(this._onTestRunBeforeDone));
     }
 
+    _addProblematicPlaceholdersWarning (placeholders) {
+        const problematicPlaceholderListStr = getConcatenatedValuesString(placeholders);
+        const suffix                        = placeholders.length > 1 ? 's' : '';
+        const verb                          = placeholders.length > 1 ? 'are' : 'is';
+        const pronoun                       = placeholders.length > 1 ? 'Their' : 'Its';
+
+        this.warningLog.addWarning(WARNING_MESSAGES.problematicPathPatternPlaceholderForVideoRecording, problematicPlaceholderListStr, suffix, verb, pronoun, suffix);
+    }
+
     _getTargetVideoPath (testRunInfo) {
         const { test, index, testRun } = testRunInfo;
 
@@ -72,11 +83,13 @@ export default class VideoRecorder {
             testIndex:         this.singleFile ? null : index,
             quarantineAttempt: null,
             now:               this.timeStamp,
-            fixture:           this.singleFile ? '' : test.fixture.name,
-            test:              this.singleFile ? '' : test.name,
+            fixture:           this.singleFile ? null : test.fixture.name,
+            test:              this.singleFile ? null : test.name,
             parsedUserAgent:   connection.browserInfo.parsedUserAgent,
         });
 
+        pathPattern.on('problematic-placeholders-found', ({ placeholders }) => this._addProblematicPlaceholdersWarning(placeholders));
+
         return join(this.basePath, pathPattern.getPath());
     }
 

test/functional/fixtures/video-recording/test.js

@@ -99,5 +99,23 @@ if (config.useLocalBrowsers) {
                     expect(videoFiles.length).to.equal(2);
                 });
         });
+
+        it('Should display the warning if there is the not suitable placeholder for the "pathPattern" option was specified', () => {
+            return runTests('./testcafe-fixtures/index-test.js', '', {
+                only:         'chrome',
+                shouldFail:   true,
+                setVideoPath: true,
+
+                videoOptions: {
+                    singleFile:  true,
+                    pathPattern: '${TEST_INDEX}_.mp4'
+                }
+            })
+                .catch(() => {
+                    expect(testReport.warnings).eql(['"${TEST_INDEX}" path pattern placeholder is not suitable for the video recording\'s "pathPattern" option.' +
+                                                     '\n\n' +
+                                                     'Its value will be replaced with an empty string.']);
+                });
+        });
     });
 }

test/server/util-test.js

@@ -15,6 +15,7 @@ const getCommonPath                    = require('../../lib/utils/get-common-pat
 const resolvePathRelativelyCwd         = require('../../lib/utils/resolve-path-relatively-cwd');
 const getFilterFn                      = require('../../lib/utils/get-filter-fn');
 const prepareReporters                 = require('../../lib/utils/prepare-reporters');
+const getConcatenatedValuesString      = require('../../lib/utils/get-concatenated-values-string');
 
 describe('Utils', () => {
     it('Correct File Path', () => {
@@ -168,6 +169,11 @@ describe('Utils', () => {
         expect(getFilterFn({ fixture: 'test' })).to.be.a('function');
     });
 
+    it('Get concatenated values string', () => {
+        expect(getConcatenatedValuesString(['param_1'])).eql('"param_1"');
+        expect(getConcatenatedValuesString(['param_1', 'param_2', 'param_3'])).eql('"param_1", "param_2", "param_3"');
+    });
+
     describe('Moment Module Loader', () => {
         const moduleCacheDesciptor    = Object.getOwnPropertyDescriptor(Module, '_cache');
         const originalLoad            = Module._load;

test/server/video-recorder-test.js

@@ -1,7 +1,7 @@
-const expect        = require('chai').expect;
+const { expect }    = require('chai');
 const VideoRecorder = require('../../lib/video-recorder');
 const AsyncEmitter  = require('../../lib/utils/async-event-emitter');
-
+const WarningLog    = require('../../lib/notifications/warning-log');
 
 const VIDEOS_BASE_PATH = '__videos__';
 
@@ -25,4 +25,25 @@ describe('Video Recorder', () => {
                 expect(videoRecorder.testRunInfo).to.be.empty;
             });
     });
+
+    it('Should correctly format the warning message about no suitable path pattern placeholders', () => {
+        const browserJobMock = new AsyncEmitter();
+        const warningLog     = new WarningLog();
+        const videoRecorder  = new VideoRecorder(browserJobMock, VIDEOS_BASE_PATH, {}, {}, warningLog);
+
+        videoRecorder._addProblematicPlaceholdersWarning(['${TEST_INDEX}']);
+        expect(warningLog.messages).eql([
+            '"${TEST_INDEX}" path pattern placeholder is not suitable for the video recording\'s "pathPattern" option.' +
+            '\n\n' +
+            'Its value will be replaced with an empty string.'
+        ]);
+        warningLog.messages = [];
+
+        videoRecorder._addProblematicPlaceholdersWarning(['${TEST_INDEX}', '${FIXTURE}']);
+        expect(warningLog.messages).eql([
+            '"${TEST_INDEX}", "${FIXTURE}" path pattern placeholders are not suitable for the video recording\'s "pathPattern" option.' +
+            '\n\n' +
+            'Their values will be replaced with an empty string.'
+        ]);
+    });
 });