-
Notifications
You must be signed in to change notification settings - Fork 39
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
Conversation
@@ -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; |
There was a problem hiding this comment.
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
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
([]).concat
порождает лишний массив при каждом вызове этой функции.
Такой вариант в два раза быстрее.
Это не много, но _buildId
вызывается кучу раз при добавлении каждого результата.
Можно было и быстрее, но дальше код становится хуже
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
сколько мы здесь выигрываем по времени, что нужно трогать это место?
const resultStatuses = _.compact(([] as (string | undefined)[]).concat(suite.browserIds)) | ||
const resultStatuses = (suite.browserIds || []) |
There was a problem hiding this comment.
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
значений
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут мне не понравилась эта строчка, расписал понятнее
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR не выглядит полезным как что-то, что ощутимо ускоряет отчет (какие-то замеры могут меня переубедить). Больше похоже на мини-рефакторинг кусков кода, на которые ты обратил внимание во время другой задачи
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
сколько мы здесь выигрываем по времени, что нужно трогать это место?
Что сделано
Небольшие оптимизации кода построения дерева