Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: prepare html-reporter for pwt GUI integration #522

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions lib/common-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
Expand All @@ -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;
}
Expand Down Expand Up @@ -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) => {
Expand All @@ -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);
};
Expand Down
39 changes: 29 additions & 10 deletions lib/gui/tool-runner/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -104,6 +107,7 @@ export class ToolRunner {
this._reportBuilder = null;

this._tests = {};
this._testAttemptManager = new TestAttemptManager();
}

get config(): HermioneConfig {
Expand All @@ -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();
Expand Down Expand Up @@ -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;

Expand All @@ -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) => {
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

мы разве не можем добавлять попытку уже внутри метода addSkipped? а то получается, что часть по созданию ретрая мы делаем отдельно

Copy link
Member Author

@shadowusr shadowusr Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

При текущем подходе не можем по следующим причинам:

  • везде в приложении, включая addSkipped, мы условились работать с ReporterTestResult
  • для создания ReporterTestResult нужен attempt

Можно было бы поместить его в фабрику formatTestResult, но тогда каждый раз, когда хотим отформатировать testResult считался бы ретраем, а это — неожиданный сайд-эффект.

В целом я задумывал testAttemptManager как независимый сервис по отслеживанию попыток теста, можно думать про него, что это база данных. Сам результат теста не знает, какой он по счету, в каждом инстансе ReporterTestResult зафиксирована конкретная попытка и он тоже не знает о глобальном состоянии. А testAttemptManager — как раз глобальное хранилище состояния попыток.

Если есть идеи, как можно упростить/улучшить этот момент — буду рад услышать!

} else {
const attempt = reportBuilder.testAttemptManager.registerAttempt({fullName: test.fullTitle(), browserId: test.browserId}, IDLE);
reportBuilder.addIdle(formatTestResultUnsafe(test, IDLE, attempt, reportBuilder));
}
});
Expand Down Expand Up @@ -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
Expand All @@ -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};
Expand Down
40 changes: 3 additions & 37 deletions lib/report-builder/gui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;

Expand Down
5 changes: 3 additions & 2 deletions lib/report-builder/static.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions lib/sqlite-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export interface PreparedTestResult {
status: TestStatus;
timestamp: number;
errorDetails?: ErrorDetails;
suitePath: string[];
}

export interface DbTestResult {
Expand Down
19 changes: 17 additions & 2 deletions lib/static/modules/reducers/tree/index.js
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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) => {
Expand All @@ -37,6 +37,7 @@ export default ((state, action) => {

updateAllSuitesStatus(tree, filteredBrowsers);
initNodesStates({tree, view: state.view});
// TODO: handle "updated" status here

shadowusr marked this conversation as resolved.
Show resolved Hide resolved
return {...state, tree};
}
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions lib/static/modules/reducers/tree/nodes/suites.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -285,5 +285,5 @@ function getChildSuitesStatus(tree, suite, filteredBrowsers, diff = tree) {
childStatuses = childStatuses.concat(suiteBrowserStatuses);
}

return determineStatus(childStatuses);
return determineFinalStatus(childStatuses);
}
41 changes: 39 additions & 2 deletions lib/test-adapter/utils/index.ts
Original file line number Diff line number Diff line change
@@ -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
shadowusr marked this conversation as resolved.
Show resolved Hide resolved
: never = keys;

return _.assign({}, _.pick(original, keysTypeChecked) as ReporterTestResult, updates);
};
10 changes: 6 additions & 4 deletions lib/test-attempt-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions lib/tests-tree-builder/base.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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) {
Expand Down
1 change: 1 addition & 0 deletions lib/tests-tree-builder/static.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Loading
Loading