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

perf: small js report builder optimization #500

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
20 changes: 10 additions & 10 deletions lib/tests-tree-builder/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export interface Tree {
interface ResultPayload {
id: string;
parentId: string;
result: ParsedSuitesRow;
result: Omit<ParsedSuitesRow, 'imagesInfo'>;
}

interface BrowserPayload {
Expand Down Expand Up @@ -111,7 +111,7 @@ export class BaseTestsTreeBuilder {

addTestResult(testResult: ParsedSuitesRow, formattedResult: Pick<ReporterTestResult, 'testPath' | 'browserId' | 'attempt'>): void {
const {testPath, browserId: browserName, attempt} = formattedResult;
const {imagesInfo} = testResult;
const {imagesInfo, ...resultWithoutImagesInfo} = testResult;
Copy link
Member Author

Choose a reason for hiding this comment

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

Дальше по коду через _.omit вытаскиваются imagesInfo из testResult

Вместо того, чтобы передавать testResult полностью, сразу вытаскиваем ненужный imagesInfo без _.omit

const {browserVersion = BrowserVersions.UNKNOWN} = testResult.metaInfo as {browserVersion: string};

const suiteId = this._buildId(testPath);
Expand All @@ -122,14 +122,16 @@ export class BaseTestsTreeBuilder {

this._addSuites(testPath, browserId);
this._addBrowser({id: browserId, parentId: suiteId, name: browserName, version: browserVersion}, testResultId, attempt);
this._addResult({id: testResultId, parentId: browserId, result: testResult}, imageIds);
this._addResult({id: testResultId, parentId: browserId, result: resultWithoutImagesInfo}, imageIds);
this._addImages(imageIds, {imagesInfo, parentId: testResultId});

this._setStatusForBranch(testPath);
}

protected _buildId(parentId: string | string[] = [], name: string | string[] = []): string {
return ([] as string[]).concat(parentId, name).join(' ');
const toStr = (value: string | string[]): string => Array.isArray(value) ? value.join(' ') : value;

return name.length ? `${toStr(parentId)} ${toStr(name)}` : toStr(parentId);
Comment on lines -132 to +134
Copy link
Member Author

Choose a reason for hiding this comment

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

([]).concat порождает лишний массив при каждом вызове этой функции.

Такой вариант в два раза быстрее.

Это не много, но _buildId вызывается кучу раз при добавлении каждого результата.

Можно было и быстрее, но дальше код становится хуже

Copy link
Member

Choose a reason for hiding this comment

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

сколько мы здесь выигрываем по времени, что нужно трогать это место?

}

protected _addSuites(testPath: string[], browserId: string): void {
Expand Down Expand Up @@ -200,13 +202,11 @@ export class BaseTestsTreeBuilder {
}

protected _addResult({id, parentId, result}: ResultPayload, imageIds: string[]): void {
const resultWithoutImagesInfo = _.omit(result, 'imagesInfo');

if (!this._tree.results.byId[id]) {
this._tree.results.allIds.push(id);
}

this._tree.results.byId[id] = {id, parentId, ...resultWithoutImagesInfo, imageIds};
this._tree.results.byId[id] = {id, parentId, ...result, imageIds};
}

protected _addImages(imageIds: string[], {imagesInfo, parentId}: ImagesPayload): void {
Expand All @@ -225,18 +225,18 @@ export class BaseTestsTreeBuilder {

const suite = this._tree.suites.byId[suiteId];

const resultStatuses = _.compact(([] as (string | undefined)[]).concat(suite.browserIds))
const resultStatuses = (suite.browserIds || [])
Copy link
Member Author

Choose a reason for hiding this comment

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

Тут и дальше по коду

_.compact([].concat(suite.browserIds)) - два лишних копирования. Внутреннее не нужно, потому что suite.browserIds всегда массив, а compact не нужно, потому что в дереве все равно нет falsy значений

.map((browserId: string) => {
const browser = this._tree.browsers.byId[browserId];
const lastResultId = _.last(browser.resultIds) as string;

return this._tree.results.byId[lastResultId].status;
});

const childrenSuiteStatuses = _.compact(([] as (string | undefined)[]).concat(suite.suiteIds))
const childrenSuiteStatuses = (suite.suiteIds || [])
.map((childSuiteId: string) => this._tree.suites.byId[childSuiteId].status);

const status = determineStatus(_.compact([...resultStatuses, ...childrenSuiteStatuses]));
const status = determineStatus([...resultStatuses, ...childrenSuiteStatuses].filter(Boolean) as TestStatus[]);

// if newly determined status is the same as current status, do nothing
if (suite.status === status) {
Expand Down
10 changes: 5 additions & 5 deletions lib/tests-tree-builder/static.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ export class StaticTestsTreeBuilder extends BaseTestsTreeBuilder {
const testId = this._buildId(testPath);
const browserId = this._buildId(testId, browserName);

attemptsMap.set(browserId, attemptsMap.has(browserId) ? attemptsMap.get(browserId) as number + 1 : 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

Тут мне не понравилась эта строчка, расписал понятнее

const attempt = attemptsMap.get(browserId) as number;

const testResult = mkTestResult(row, {attempt});
const formattedResult = {browserId: browserName, testPath, attempt};
const prevAttempts = attemptsMap.get(browserId);
const curAttempts = prevAttempts === undefined ? 0 : prevAttempts + 1;
attemptsMap.set(browserId, curAttempts);
const testResult = mkTestResult(row, {attempt: curAttempts});
const formattedResult = {browserId: browserName, testPath, attempt: curAttempts};

addBrowserVersion(browsers, testResult);

Expand Down
Loading