fix: fix test attempt manager, review issues, status computing

lib/common-utils.ts

@@ -7,6 +7,7 @@ import {SUCCESS, FAIL, ERROR, SKIPPED, UPDATED, IDLE, RUNNING, QUEUED, TestStatu
 import {UNCHECKED, INDETERMINATE, CHECKED} from './constants/checked-statuses';
 import {ImageData, ImageBase64, ImageInfoFull, TestError, ImageInfoError} from './types';
 import {ErrorName, ImageDiffError, NoRefImageError} from './errors';
+import {ReporterTestResult} from './test-adapter';
 export const getShortMD5 = (str: string): string => {
     return crypto.createHash('md5').update(str, 'ascii').digest('hex').substr(0, 7);
 };
@@ -29,7 +30,7 @@ export const isErrorStatus = (status: TestStatus): boolean => status === ERROR;
 export const isSkippedStatus = (status: TestStatus): boolean => status === SKIPPED;
 export const isUpdatedStatus = (status: TestStatus): boolean => status === UPDATED;
 
-export const determineStatus = (statuses: TestStatus[]): TestStatus | null => {
+export const determineFinalStatus = (statuses: TestStatus[]): TestStatus | null => {
     if (!statuses.length) {
         return SUCCESS;
     }
@@ -103,7 +104,7 @@ export const hasNoRefImageErrors = ({assertViewResults = []}: {assertViewResults
     return assertViewResults.some((assertViewResult) => isNoRefImageError(assertViewResult));
 };
 
-const hasFailedImages = (result: {imagesInfo?: ImageInfoFull[]}): boolean => {
+export const hasFailedImages = (result: {imagesInfo?: ImageInfoFull[]}): boolean => {
     const {imagesInfo = []} = result;
 
     return imagesInfo.some((imageInfo: ImageInfoFull) => {
@@ -128,6 +129,24 @@ export const hasDiff = (assertViewResults: {name?: string}[]): boolean => {
     return assertViewResults.some((result) => isImageDiffError(result as {name?: string}));
 };
 
+/* This method tries to determine true status of testResult by using fields like error, imagesInfo */
+export const determineStatus = (testResult: Pick<ReporterTestResult, 'status' | 'error' | 'imagesInfo'>): TestStatus => {
+    if (!hasFailedImages(testResult) && !isSkippedStatus(testResult.status) && isEmpty(testResult.error)) {
+        return SUCCESS;
+    }
+
+    const imageErrors = (testResult.imagesInfo ?? []).map(imagesInfo => (imagesInfo as {error: {name?: string}}).error ?? {});
+    if (hasDiff(imageErrors) || hasNoRefImageErrors({assertViewResults: imageErrors})) {
+        return FAIL;
+    }
+
+    if (!isEmpty(testResult.error)) {
+        return ERROR;
+    }
+
+    return testResult.status;
+};
+
 export const isBase64Image = (image: ImageData | ImageBase64 | null | undefined): image is ImageBase64 => {
     return Boolean((image as ImageBase64 | undefined)?.base64);
 };

lib/gui/tool-runner/index.ts

@@ -72,6 +72,8 @@ const formatTestResultUnsafe = (
     return formatTestResult(test as HermioneTestResult, status, attempt, {imageHandler});
 };
 
+const HERMIONE_TITLE_DELIMITER = ' ';
+
 export class ToolRunner {
     private _testFiles: string[];
     private _hermione: Hermione & HtmlReporterApi;
@@ -84,6 +86,7 @@ export class ToolRunner {
     private _eventSource: EventSource;
     protected _reportBuilder: GuiReportBuilder | null;
     private _tests: Record<string, HermioneTest>;
+    private readonly _testAttemptManager: TestAttemptManager;
 
     static create<T extends ToolRunner>(this: new (...args: ToolRunnerArgs) => T, ...args: ToolRunnerArgs): T {
         return new this(...args);
@@ -104,6 +107,7 @@ export class ToolRunner {
         this._reportBuilder = null;
 
         this._tests = {};
+        this._testAttemptManager = new TestAttemptManager();
     }
 
     get config(): HermioneConfig {
@@ -118,9 +122,8 @@ export class ToolRunner {
         await mergeDatabasesForReuse(this._reportPath);
 
         const dbClient = await SqliteClient.create({htmlReporter: this._hermione.htmlReporter, reportPath: this._reportPath, reuse: true});
-        const testAttemptManager = new TestAttemptManager();
 
-        this._reportBuilder = GuiReportBuilder.create(this._hermione.htmlReporter, this._pluginConfig, {dbClient, testAttemptManager});
+        this._reportBuilder = GuiReportBuilder.create(this._hermione.htmlReporter, this._pluginConfig, {dbClient, testAttemptManager: this._testAttemptManager});
         this._subscribeOnEvents();
 
         this._collection = await this._readTests();
@@ -185,10 +188,11 @@ export class ToolRunner {
         return Promise.all(tests.map(async (test): Promise<TestBranch> => {
             const updateResult = this._prepareTestResult(test);
 
-            const fullName = test.suite.path.join(' ');
+            const fullName = [...test.suite.path, test.state.name].join(HERMIONE_TITLE_DELIMITER);
             const updateAttempt = reportBuilder.testAttemptManager.registerAttempt({fullName, browserId: test.browserId}, UPDATED);
             const formattedResult = formatTestResultUnsafe(updateResult, UPDATED, updateAttempt, reportBuilder);
-            const failResultId = formattedResult.id;
+
+            const failResultId = formatTestResultUnsafe(updateResult, UPDATED, updateAttempt - 1, reportBuilder).id;
 
             updateResult.attempt = updateAttempt;
 
@@ -213,8 +217,8 @@ export class ToolRunner {
 
         await Promise.all(tests.map(async (test) => {
             const updateResult = this._prepareTestResult(test);
-            const fullName = test.suite.path.join(' ');
-            const attempt = reportBuilder.testAttemptManager.removeAttempt({fullName, browserId: test.browserId});
+            const fullName = [...test.suite.path, test.state.name].join(' ');
+            const attempt = reportBuilder.testAttemptManager.getCurrentAttempt({fullName, browserId: test.browserId});
             const formattedResult = formatTestResultUnsafe(updateResult, UPDATED, attempt, reportBuilder);
 
             await Promise.all(updateResult.imagesInfo.map(async (imageInfo) => {
@@ -318,11 +322,10 @@ export class ToolRunner {
             const testId = formatId(test.id.toString(), browserId);
             this._tests[testId] = _.extend(test, {browserId});
 
+            const attempt = 0;
             if (test.pending) {
-                const attempt = reportBuilder.testAttemptManager.registerAttempt({fullName: test.fullTitle(), browserId: test.browserId}, SKIPPED);
                 reportBuilder.addSkipped(formatTestResultUnsafe(test, SKIPPED, attempt, reportBuilder));
             } else {
-                const attempt = reportBuilder.testAttemptManager.registerAttempt({fullName: test.fullTitle(), browserId: test.browserId}, IDLE);
                 reportBuilder.addIdle(formatTestResultUnsafe(test, IDLE, attempt, reportBuilder));
             }
         });
@@ -358,7 +361,15 @@ export class ToolRunner {
                 return _.extend(imageInfo, {expectedImg: refImg});
             });
 
-        const res = _.merge({}, rawTest, {assertViewResults, imagesInfo, sessionId, attempt, meta: {url}, updated: true});
+        const res = _.merge({}, rawTest, {
+            assertViewResults,
+            err: test.error,
+            imagesInfo,
+            sessionId,
+            attempt,
+            meta: {url},
+            updated: true
+        });
 
         // _.merge can't fully clone test object since hermione@7+
         // TODO: use separate object to represent test results. Do not extend test object with test results
@@ -380,8 +391,16 @@ export class ToolRunner {
         const {autoRun} = this._guiOpts;
         const testsTree = await this._loadDataFromDatabase();
 
-        if (!_.isEmpty(testsTree)) {
+        if (testsTree && !_.isEmpty(testsTree)) {
             reportBuilder.reuseTestsTree(testsTree);
+
+            // Fill test attempt manager with data from db
+            for (const [, testResult] of Object.entries(testsTree.results.byId)) {
+                this._testAttemptManager.registerAttempt({
+                    fullName: testResult.suitePath.join(HERMIONE_TITLE_DELIMITER),
+                    browserId: testResult.name
+                }, testResult.status, testResult.attempt);
+            }
         }
 
         this._tree = {...reportBuilder.getResult(), autoRun};

lib/report-builder/gui.ts

@@ -2,14 +2,14 @@ import * as _ from 'lodash';
 import {StaticReportBuilder} from './static';
 import {GuiTestsTreeBuilder, TestBranch, TestEqualDiffsData, TestRefUpdateData} from '../tests-tree-builder/gui';
 import {
-    IDLE, RUNNING, FAIL, SUCCESS, UPDATED, TestStatus, DB_COLUMNS, ToolName, ERROR
+    IDLE, RUNNING, UPDATED, TestStatus, DB_COLUMNS, ToolName
 } from '../constants';
 import {ConfigForStaticFile, getConfigForStaticFile} from '../server-utils';
 import {ReporterTestResult} from '../test-adapter';
 import {PreparedTestResult} from '../sqlite-client';
-import {Tree, TreeImage, TreeResult} from '../tests-tree-builder/base';
+import {Tree, TreeImage} from '../tests-tree-builder/base';
 import {ImageInfoWithState, ReporterConfig} from '../types';
-import {hasDiff, hasNoRefImageErrors, hasResultFails, isSkippedStatus, isUpdatedStatus} from '../common-utils';
+import {isUpdatedStatus} from '../common-utils';
 import {HtmlReporterValues} from '../plugin-api';
 import {SkipItem} from '../tests-tree-builder/static';
 import {copyAndUpdate} from '../test-adapter/utils';
@@ -164,45 +164,11 @@ export class GuiReportBuilder extends StaticReportBuilder {
 
         this._extendTestWithImagePaths(testResult, formattedResult, opts);
 
-        if (![IDLE, RUNNING].includes(testResult.status)) {
-            this._updateTestResultStatus(testResult, formattedResult);
-        }
-
         this._testsTree.addTestResult(testResult, formattedResult);
 
         return formattedResult;
     }
 
-    protected _checkResult(result: TreeResult | undefined, formattedResult: ReporterTestResult): asserts result is TreeResult {
-        if (!result) {
-            const filteredTestTreeResults = _.pickBy(
-                this._testsTree.tree.results.byId,
-                (_result, resultId) => resultId.startsWith(formattedResult.fullName));
-
-            throw new Error('Failed to get last result for test:\n' +
-                `fullName = ${formattedResult.fullName}; browserId = ${formattedResult.browserId}\n` +
-                `Related testsTree results: ${JSON.stringify(filteredTestTreeResults)}\n`);
-        }
-    }
-
-    private _updateTestResultStatus(testResult: PreparedTestResult, formattedResult: ReporterTestResult): void {
-        if (!hasResultFails(testResult) && !isSkippedStatus(testResult.status) && _.isEmpty(testResult.error)) {
-            testResult.status = SUCCESS;
-            return;
-        }
-
-        const imageErrors = testResult.imagesInfo.map(imagesInfo => (imagesInfo as {error: {name?: string}}).error ?? {});
-        if (hasDiff(imageErrors) || hasNoRefImageErrors({assertViewResults: imageErrors})) {
-            testResult.status = FAIL;
-            return;
-        }
-
-        if (!_.isEmpty(formattedResult.error)) {
-            testResult.status = ERROR;
-            return;
-        }
-    }
-
     private _extendTestWithImagePaths(testResult: PreparedTestResult, formattedResult: ReporterTestResult, opts: {failResultId?: string} = {}): void {
         const newImagesInfo = formattedResult.imagesInfo;
 

lib/report-builder/static.ts

@@ -143,7 +143,7 @@ export class StaticReportBuilder {
     protected _createTestResult(result: ReporterTestResult, props: {attempt?: number | null, status: TestStatus, timestamp: number;} & Partial<PreparedTestResult>): PreparedTestResult {
         const {
             browserId, file, sessionId, description, history,
-            imagesInfo = [], screenshot, multipleTabs, errorDetails
+            imagesInfo = [], screenshot, multipleTabs, errorDetails, testPath
         } = result;
 
         const {baseHost, saveErrorDetails} = this._pluginConfig;
@@ -153,7 +153,8 @@ export class StaticReportBuilder {
 
         const testResult: PreparedTestResult = Object.assign({
             suiteUrl, name: browserId, metaInfo, description, history,
-            imagesInfo, screenshot: Boolean(screenshot), multipleTabs
+            imagesInfo, screenshot: Boolean(screenshot), multipleTabs,
+            suitePath: testPath
         }, props);
 
         const error = getError(result.error);

lib/sqlite-client.ts

@@ -44,6 +44,7 @@ export interface PreparedTestResult {
     status: TestStatus;
     timestamp: number;
     errorDetails?: ErrorDetails;
+    suitePath: string[];
 }
 
 export interface DbTestResult {

lib/static/modules/reducers/tree/index.js

@@ -1,4 +1,4 @@
-import {findLast, isEmpty} from 'lodash';
+import {findLast, isEmpty, pick} from 'lodash';
 import {produce} from 'immer';
 import actionNames from '../../action-names';
 import {
@@ -16,7 +16,7 @@ import {
 import {ViewMode} from '../../../../constants/view-modes';
 import {EXPAND_RETRIES} from '../../../../constants/expand-modes';
 import {FAIL} from '../../../../constants/test-statuses';
-import {isSuccessStatus} from '../../../../common-utils';
+import {determineStatus, isSuccessStatus, isUpdatedStatus} from '../../../../common-utils';
 import {applyStateUpdate, ensureDiffProperty, getUpdatedProperty} from '../../utils/state';
 
 export default ((state, action) => {
@@ -37,6 +37,7 @@ export default ((state, action) => {
 
             updateAllSuitesStatus(tree, filteredBrowsers);
             initNodesStates({tree, view: state.view});
+            // TODO: handle "updated" status here
 
             return {...state, tree};
         }
@@ -350,6 +351,20 @@ function addNodesToTree(state, payload) {
     const {tree, view} = state;
 
     [].concat(payload).forEach(({result, images, suites}) => {
+        if (isUpdatedStatus(result.status)) {
+            const computedStatus = determineStatus({
+                status: result.status,
+                error: result.error,
+                imagesInfo: Object.values(pick(tree.images.byId, result.imageIds))
+            });
+
+            result.status = computedStatus;
+
+            for (const suite of suites.filter(suite => isUpdatedStatus(suite.status))) {
+                suite.status = computedStatus;
+            }
+        }
+
         addResult(tree, result);
         setBrowsersLastRetry(tree, result.parentId);
         addImages(tree, images, view.expand);

lib/static/modules/reducers/tree/nodes/suites.js

@@ -2,7 +2,7 @@ import _ from 'lodash';
 import {getSuiteResults} from '../../../selectors/tree';
 import {isNodeFailed} from '../../../utils';
 import {ensureDiffProperty, getUpdatedProperty} from '../../../utils/state';
-import {determineStatus, isFailStatus} from '../../../../../common-utils';
+import {determineFinalStatus, isFailStatus} from '../../../../../common-utils';
 import {changeNodeState, getShownCheckedChildCount, shouldNodeBeOpened} from '../helpers';
 import {EXPAND_RETRIES} from '../../../../../constants/expand-modes';
 import {FAIL} from '../../../../../constants/test-statuses';
@@ -285,5 +285,5 @@ function getChildSuitesStatus(tree, suite, filteredBrowsers, diff = tree) {
         childStatuses = childStatuses.concat(suiteBrowserStatuses);
     }
 
-    return determineStatus(childStatuses);
+    return determineFinalStatus(childStatuses);
 }

lib/test-adapter/utils/index.ts

@@ -1,5 +1,42 @@
 import _ from 'lodash';
 import {ReporterTestResult} from '../index';
+import {TupleToUnion} from 'type-fest';
 
-export const copyAndUpdate = (original: ReporterTestResult, updates: Partial<ReporterTestResult>): ReporterTestResult =>
-    _.assign({}, original, updates);
+export const copyAndUpdate = (original: ReporterTestResult, updates: Partial<ReporterTestResult>): ReporterTestResult => {
+    const keys = [
+        'assertViewResults',
+        'attempt',
+        'browserId',
+        'description',
+        'error',
+        'errorDetails',
+        'file',
+        'fullName',
+        'history',
+        'id',
+        'image',
+        'imageDir',
+        'imagesInfo',
+        'isUpdated',
+        'meta',
+        'multipleTabs',
+        'screenshot',
+        'sessionId',
+        'skipReason',
+        'state',
+        'status',
+        'testPath',
+        'timestamp',
+        'url'
+    ] as const;
+
+    // Type-level check that we didn't forget to include any keys
+    type A = TupleToUnion<typeof keys>;
+    type B = keyof ReporterTestResult;
+
+    const keysTypeChecked: B extends A ?
+        B extends A ? typeof keys : never
+        : never = keys;
+
+    return _.assign({}, _.pick(original, keysTypeChecked) as ReporterTestResult, updates);
+};

lib/test-attempt-manager.ts

@@ -32,12 +32,14 @@ export class TestAttemptManager {
         return Math.max(data.statuses.length - 1, 0);
     }
 
-    registerAttempt(testResult: TestSpec, status: TestStatus): number {
+    registerAttempt(testResult: TestSpec, status: TestStatus, index: number | null = null): number {
         const [hash, data] = this._getData(testResult);
 
-        if (![IDLE, RUNNING].includes(status)) {
-            data.statuses.push(status);
-        }
+        const isManualOverride = index !== null;
+        const isLastStatusTemporary = [IDLE, RUNNING].includes(data.statuses.at(-1) as TestStatus);
+        const shouldReplace = Number(isManualOverride || isLastStatusTemporary);
+
+        data.statuses.splice(index ?? data.statuses.length - shouldReplace, shouldReplace, status);
 
         this._attempts.set(hash, data);
 

lib/tests-tree-builder/base.ts

@@ -1,5 +1,5 @@
 import _ from 'lodash';
-import {determineStatus} from '../common-utils';
+import {determineFinalStatus} from '../common-utils';
 import {BrowserVersions, PWT_TITLE_DELIMITER, TestStatus, ToolName} from '../constants';
 import {ReporterTestResult} from '../test-adapter';
 import {ImageInfoFull, ParsedSuitesRow} from '../types';
@@ -253,7 +253,7 @@ export class BaseTestsTreeBuilder {
         const childrenSuiteStatuses = _.compact(([] as (string | undefined)[]).concat(suite.suiteIds))
             .map((childSuiteId: string) => this._tree.suites.byId[childSuiteId].status);
 
-        const status = determineStatus(_.compact([...resultStatuses, ...childrenSuiteStatuses]));
+        const status = determineFinalStatus(_.compact([...resultStatuses, ...childrenSuiteStatuses]));
 
         // if newly determined status is the same as current status, do nothing
         if (suite.status === status) {

lib/tests-tree-builder/static.ts

@@ -201,6 +201,7 @@ function mkTestResult(row: RawSuitesRow, data: {attempt: number}): ParsedSuitesR
         name: row[DB_COLUMN_INDEXES.name] as string,
         screenshot: Boolean(row[DB_COLUMN_INDEXES.screenshot]),
         status: row[DB_COLUMN_INDEXES.status] as TestStatus,
+        suitePath: JSON.parse(row[DB_COLUMN_INDEXES.suitePath] as string),
         suiteUrl: row[DB_COLUMN_INDEXES.suiteUrl] as string,
         skipReason: row[DB_COLUMN_INDEXES.skipReason] as string,
         error: JSON.parse(row[DB_COLUMN_INDEXES.error] as string),