-
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
refactor: introduce adapter pattern for test results #493
refactor: introduce adapter pattern for test results #493
Conversation
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.
👍
@@ -83,7 +83,6 @@ | |||
"qs": "^6.9.1", | |||
"signal-exit": "^3.0.2", | |||
"tmp": "^0.1.0", | |||
"urijs": "^1.18.12", |
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.
а разве из package-lock.json он не должен был тоже пропасть?
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.
Да, должен, просто package-lock я обычно обновляю перед влитием. Обязательно обновлю. Ну и не хотел пугать большим диффом в PR )
@@ -77,11 +81,12 @@ module.exports = (hermione, reportBuilder, client, reportPath) => { | |||
|
|||
hermione.on(hermione.events.TEST_FAIL, (testResult) => { | |||
queue.add(async () => { | |||
const formattedResult = reportBuilder.format(testResult, hermione.events.TEST_FAIL); | |||
const status = hasDiff(testResult.assertViewResults) ? TestStatus.FAIL : TestStatus.ERROR; |
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-а, но почему мы выставляем fail именно при наличии диффа? чем это отличается от обычного провалившегося ассерта?
//cc @DudaGod
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.
Я сам недавно задавался этим вопросом когда интеграцию pwt с testcop пилил. Сходу не помню почему так было сделано.
|
||
return {name, suitePath, browserId}; | ||
get url(): string | undefined { | ||
return this._testResult.meta.url as string | undefined; | ||
} | ||
|
||
get multipleTabs(): boolean { |
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.
Согласен. Но на это есть много завязок в html-reporter, многое может сломаться, если сейчас выгасить. Мне кажется это лучше делать в отдельном PR
9db24c7
to
e10d2fe
Compare
package-lock.json
Outdated
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.
дифф большой из-за добавления pwt в devDependencies.
e10d2fe
to
a4a6b9b
Compare
a4a6b9b
to
a679d08
Compare
What's done?
ReporterTestResult
as a single interface to interact with across all of the html-reporter code. AdjustedTestAdapter
accordingly, renamed it toHermioneTestAdapter
and got rid of unnecessary field/methodsSuiteAdapter
by partially merging it intoTestAdapter
and partially moving the logic to helpersWhy?