Merge pull request #474 from gemini-testing/HERMIONE-984.fix_undo

fix: undo with remote images

Author: KuznetsovRoman

lib/gui/server.js

@@ -74,7 +74,7 @@ exports.start = async ({paths, hermione, guiApi, configs}) => {
         }
     });
 
-    server.post('/get-update-reference-data', (req, res) => {
+    server.post('/reference-data-to-update', (req, res) => {
         try {
             const data = app.getTestsDataToUpdateRefs(req.body);
             res.json(data);

lib/gui/tool-runner/index.js

@@ -97,7 +97,7 @@ module.exports = class ToolRunner {
 
     updateReferenceImage(tests) {
         return Promise.map(tests, (test) => {
-            const updateResult = this._prepareUpdateResult(test);
+            const updateResult = this._prepareTestResult(test);
             const formattedResult = this._reportBuilder.format(updateResult, UPDATED);
             const failResultId = formattedResult.id;
             const updateAttempt = this._reportBuilder.getUpdatedAttempt(formattedResult);
@@ -121,17 +121,43 @@ module.exports = class ToolRunner {
         });
     }
 
-    async undoAcceptImages(imageIds) {
-        const {
-            referencesToRemove,
-            referencesToUpdate,
-            ...rest
-        } = await this._reportBuilder.undoAcceptImages(imageIds, this._reportPath);
+    async undoAcceptImages(tests) {
+        const updatedImages = [], removedResults = [];
 
-        await reporterHelper.overwriteReferenceImage(referencesToUpdate, this._reportPath);
-        await reporterHelper.removeReferenceImage(referencesToRemove);
+        await Promise.map(tests, async (test) => {
+            const updateResult = this._prepareTestResult(test);
+            const formattedResult = this._reportBuilder.format(updateResult);
 
-        return rest;
+            formattedResult.attempt = updateResult.attempt;
+
+            await Promise.map(updateResult.imagesInfo, async (imageInfo) => {
+                const {stateName} = imageInfo;
+                const {
+                    updatedImage,
+                    removedResult,
+                    previousExpectedPath,
+                    shouldRemoveReference,
+                    shouldRevertReference
+                } = await this._reportBuilder.undoAcceptImage(formattedResult, stateName);
+
+                updatedImage && updatedImages.push(updatedImage);
+                removedResult && removedResults.push(removedResult);
+
+                if (shouldRemoveReference) {
+                    await reporterHelper.removeReferenceImage(formattedResult, stateName);
+                }
+
+                if (shouldRevertReference) {
+                    await reporterHelper.revertReferenceImage(formattedResult, stateName);
+                }
+
+                if (previousExpectedPath) {
+                    formattedResult.updateCacheExpectedPath(stateName, previousExpectedPath);
+                }
+            });
+        });
+
+        return {updatedImages, removedResults};
     }
 
     async findEqualDiffs(images) {
@@ -204,7 +230,7 @@ module.exports = class ToolRunner {
         subscribeOnToolEvents(this._hermione, this._reportBuilder, this._eventSource, this._reportPath);
     }
 
-    _prepareUpdateResult(test) {
+    _prepareTestResult(test) {
         const {browserId, attempt} = test;
         const fullTitle = mkFullTitle(test);
         const testId = formatId(getShortMD5(fullTitle), browserId);

lib/local-images-saver.js

@@ -4,7 +4,7 @@ const utils = require('./server-utils');
 
 module.exports = {
     saveImg: async (srcCurrPath, {destPath, reportDir}) => {
-        await utils.copyFileAsync(srcCurrPath, destPath, reportDir);
+        await utils.copyFileAsync(srcCurrPath, destPath, {reportDir});
 
         return destPath;
     }

lib/report-builder/gui.js

@@ -106,63 +106,33 @@ module.exports = class GuiReportBuilder extends StaticReportBuilder {
         return isUpdated ? attempt : attempt + 1;
     }
 
-    async undoAcceptImages(imageIds, reportPath) {
-        const updatedImages = [], removedResults = [], referencesToRemove = [], referencesToUpdate = [];
-
-        for (const imageId of imageIds) {
-            const {
-                updatedImage,
-                removedResult,
-                referenceToRemove,
-                referenceToUpdate
-            } = await this._undoAcceptImage(imageId, reportPath);
-
-            updatedImage && updatedImages.push(updatedImage);
-            removedResult && removedResults.push(removedResult);
-            referenceToRemove && referencesToRemove.push(referenceToRemove);
-            referenceToUpdate && referencesToUpdate.push(referenceToUpdate);
-        }
-
-        return {updatedImages, removedResults, referencesToRemove, referencesToUpdate};
-    }
-
-    async _undoAcceptImage(imageId, reportPath) {
+    async undoAcceptImage(formattedResult, stateName) {
+        const resultId = formattedResult.id;
+        const suitePath = formattedResult.testPath;
+        const browserName = formattedResult.browserId;
         const {
-            suitePath,
-            browserName,
-            stateName,
+            imageId,
             status,
             timestamp,
-            image,
             previousImage,
-            resultIdToRemove
-        } = this._testsTree.getResultDataToUnacceptImage(imageId);
+            shouldRemoveResult
+        } = this._testsTree.getResultDataToUnacceptImage(resultId, stateName);
 
         if (!isUpdatedStatus(status)) {
             return {};
         }
 
-        const fullTitle = suitePath.join(' ');
-        const previousExpectedImgPath = _.get(previousImage, 'expectedImg.path', null);
-        const refImgPath = previousImage.refImg.path;
-        const currentExpectedImgPath = image.expectedImg.path;
+        const previousExpectedPath = _.get(previousImage, 'expectedImg.path', null);
         const shouldRemoveReference = _.isNull(previousImage.refImg.size);
-        let updatedImage, removedResult, referenceToRemove, referenceToUpdate;
+        const shouldRevertReference = !shouldRemoveReference;
 
-        await this._removeImageFromReport(reportPath, currentExpectedImgPath);
-        this._updateTestExpectedPath(fullTitle, browserName, stateName, previousExpectedImgPath);
-
-        if (shouldRemoveReference) {
-            referenceToRemove = refImgPath;
-        } else {
-            referenceToUpdate = {path: refImgPath, source: previousExpectedImgPath};
-        }
+        let updatedImage, removedResult;
 
-        if (resultIdToRemove) {
-            this._testsTree.removeTestResult(resultIdToRemove);
-            this._decreaseTestAttemptNumber(fullTitle, browserName);
+        if (shouldRemoveResult) {
+            this._testsTree.removeTestResult(resultId);
+            formattedResult.decreaseAttemptNumber();
 
-            removedResult = resultIdToRemove;
+            removedResult = resultId;
         } else {
             updatedImage = this._testsTree.updateImageInfo(imageId, previousImage);
         }
@@ -175,7 +145,7 @@ module.exports = class GuiReportBuilder extends StaticReportBuilder {
             `json_extract(${DB_COLUMNS.IMAGES_INFO}, '$[0].stateName') = ?`
         ].join(' AND ')}, JSON.stringify(suitePath), browserName, status, timestamp, stateName);
 
-        return {updatedImage, removedResult, referenceToRemove, referenceToUpdate};
+        return {updatedImage, removedResult, previousExpectedPath, shouldRemoveReference, shouldRevertReference};
     }
 
     _addTestResult(formattedResult, props, opts = {}) {

lib/report-builder/static.js

@@ -147,14 +147,6 @@ module.exports = class StaticReportBuilder {
         this._sqliteAdapter.delete({where, orderBy, orderDescending, limit}, ...args);
     }
 
-    _updateTestExpectedPath(fullTitle, browserId, stateName, expectedPath) {
-        TestAdapter.updateCacheExpectedPath(fullTitle, browserId, stateName, expectedPath);
-    }
-
-    _decreaseTestAttemptNumber(fullTitle, browserName) {
-        TestAdapter.decreaseAttemptNumber(fullTitle, browserName);
-    }
-
     async finalize() {
         this._sqliteAdapter.close();
 

lib/reporter-helpers.js

@@ -1,28 +1,42 @@
 'use strict';
 
 const path = require('path');
+const tmp = require('tmp');
 const Promise = require('bluebird');
+const {getShortMD5} = require('./common-utils');
 const utils = require('./server-utils');
 
-exports.updateReferenceImage = (testResult, reportPath, stateName) => {
+const mkReferenceId = (testId, stateName) => getShortMD5(`${testId}#${stateName}`);
+
+exports.updateReferenceImage = async (testResult, reportPath, stateName) => {
     const currImg = testResult.getCurrImg(stateName);
 
     const src = currImg.path
         ? path.resolve(reportPath, currImg.path)
         : utils.getCurrentAbsolutePath(testResult, reportPath, stateName);
 
+    const referencePath = testResult.getRefImg(stateName).path;
+
+    if (utils.fileExists(referencePath)) {
+        const referenceId = mkReferenceId(testResult.id, stateName);
+        const oldReferencePath = path.resolve(tmp.tmpdir, referenceId);
+        await utils.copyFileAsync(referencePath, oldReferencePath);
+    }
+
     return Promise.all([
-        utils.copyFileAsync(src, testResult.getRefImg(stateName).path),
+        utils.copyFileAsync(src, referencePath),
         utils.copyFileAsync(src, utils.getReferenceAbsolutePath(testResult, reportPath, stateName))
     ]);
 };
 
-exports.removeReferenceImage = (refImgPaths) => {
-    return Promise.map([].concat(refImgPaths), (refImgPath) => utils.deleteFile(refImgPath));
+exports.revertReferenceImage = (testResult, stateName) => {
+    const referenceId = mkReferenceId(testResult.id, stateName);
+    const oldReferencePath = path.resolve(tmp.tmpdir, referenceId);
+    const referencePath = testResult.getRefImg(stateName).path;
+
+    return utils.copyFileAsync(oldReferencePath, referencePath);
 };
 
-exports.overwriteReferenceImage = (referencesToUpdate, reportPath) => {
-    return Promise.map([].concat(referencesToUpdate), (referenceToUpdate) => {
-        return utils.copyFileAsync(path.resolve(reportPath, referenceToUpdate.source), referenceToUpdate.path);
-    });
+exports.removeReferenceImage = (testResult, stateName) => {
+    return utils.deleteFile(testResult.getRefImg(stateName).path);
 };

lib/server-utils.js

@@ -47,9 +47,9 @@ function createPath(kind, result, stateName) {
     return path.join.apply(null, components);
 }
 
-function copyFileAsync(srcPath, destPath, reportDir = '') {
+function copyFileAsync(srcPath, destPath, {reportDir = '', overwrite = true} = {}) {
     const resolvedDestPath = path.resolve(reportDir, destPath);
-    return makeDirFor(resolvedDestPath).then(() => fs.copy(srcPath, resolvedDestPath));
+    return makeDirFor(resolvedDestPath).then(() => fs.copy(srcPath, resolvedDestPath, {overwrite}));
 }
 
 /**
@@ -67,6 +67,10 @@ function makeDirFor(destPath) {
     return fs.mkdirs(path.dirname(destPath));
 }
 
+function fileExists(path) {
+    return fs.existsSync(path);
+}
+
 function logPathToHtmlReport(pluginConfig) {
     const reportPath = `file://${path.resolve(pluginConfig.path, 'index.html')}`;
 
@@ -278,6 +282,7 @@ module.exports = {
     copyFileAsync,
     deleteFile,
     makeDirFor,
+    fileExists,
 
     logPathToHtmlReport,
     logError,

lib/static/modules/actions.js

@@ -135,7 +135,7 @@ export const acceptOpened = (imageIds, type = actionNames.ACCEPT_OPENED_SCREENSH
         dispatch({type: actionNames.PROCESS_BEGIN});
 
         try {
-            const {data} = await axios.post('/get-update-reference-data', imageIds);
+            const {data} = await axios.post('/reference-data-to-update', imageIds);
             const {data: updatedData} = await axios.post('/update-reference', data);
             dispatch({type, payload: updatedData});
 
@@ -168,8 +168,9 @@ export const undoAcceptImages = (imageIds, {skipTreeUpdate = false} = {}) => {
         dispatch({type: actionNames.PROCESS_BEGIN});
 
         try {
-            const {data} = await axios.post('/undo-accept-images', imageIds);
-            const payload = {...data, skipTreeUpdate};
+            const {data} = await axios.post('/reference-data-to-update', imageIds);
+            const {data: updatedData} = await axios.post('/undo-accept-images', data);
+            const payload = {...updatedData, skipTreeUpdate};
 
             dispatch({type: actionNames.UNDO_ACCEPT_IMAGES, payload});
         } catch (e) {

lib/test-adapter.js

@@ -39,7 +39,7 @@ module.exports = class TestAdapter {
         this._errors = this._hermione.errors;
         this._suite = SuiteAdapter.create(this._testResult);
         this._imagesSaver = this._hermione.htmlReporter.imagesSaver;
-        this._testId = TestAdapter._mkTestId(this._testResult.fullTitle(), this._testResult.browserId);
+        this._testId = this._mkTestId();
         this._errorDetails = undefined;
         this._timestamp = this._testResult.timestamp;
 
@@ -95,14 +95,6 @@ module.exports = class TestAdapter {
         return imagesInfo.find(info => info.stateName === stateName);
     }
 
-    _getExpectedKey(stateName) {
-        return TestAdapter._getExpectedKey(
-            this._testResult.fullTitle(),
-            this._testResult.browserId,
-            stateName
-        );
-    }
-
     _getExpectedPath(stateName, status, cacheExpectedPaths) {
         const key = this._getExpectedKey(stateName);
 
@@ -433,6 +425,38 @@ module.exports = class TestAdapter {
         return result;
     }
 
+    updateCacheExpectedPath(stateName, expectedPath) {
+        const key = this._getExpectedKey(stateName);
+
+        if (expectedPath) {
+            globalCacheExpectedPaths.set(key, expectedPath);
+        } else {
+            globalCacheExpectedPaths.delete(key);
+        }
+    }
+
+    decreaseAttemptNumber() {
+        const testId = this._mkTestId();
+        const currentTestAttempt = testsAttempts.get(testId);
+        const previousTestAttempt = currentTestAttempt - 1;
+
+        if (previousTestAttempt) {
+            testsAttempts.set(testId, previousTestAttempt);
+        } else {
+            testsAttempts.delete(testId);
+        }
+    }
+
+    _mkTestId() {
+        return this._testResult.fullTitle() + '.' + this._testResult.browserId;
+    }
+
+    _getExpectedKey(stateName) {
+        const shortTestId = getShortMD5(this._mkTestId());
+
+        return shortTestId + '#' + stateName;
+    }
+
     //parallelize and cache of 'looks-same.createDiff' (because it is very slow)
     async _saveDiffInWorker(imageDiffError, destPath, workers, cacheDiffImages = globalCacheDiffImages) {
         await utils.makeDirFor(destPath);
@@ -458,27 +482,4 @@ module.exports = class TestAdapter {
 
         cacheDiffImages.set(hash, destPath);
     }
-
-    static _mkTestId(fullTitle, browserId) {
-        return fullTitle + '.' + browserId;
-    }
-
-    static _getExpectedKey(fullTitle, browserId, stateName) {
-        const shortTestId = getShortMD5(this._mkTestId(fullTitle, browserId));
-
-        return shortTestId + '#' + stateName;
-    }
-
-    static updateCacheExpectedPath(fullTitle, browserId, stateName, expectedPath) {
-        const key = this._getExpectedKey(fullTitle, browserId, stateName);
-
-        globalCacheExpectedPaths.set(key, expectedPath);
-    }
-
-    static decreaseAttemptNumber(fullTitle, browserName) {
-        const testId = this._mkTestId(fullTitle, browserName);
-        const currentTestAttempt = testsAttempts.get(testId);
-
-        testsAttempts.set(testId, currentTestAttempt - 1);
-    }
 };