-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: correctly generate test x request id for each test in one browser #825
Conversation
7572601
to
443056b
Compare
|
||
this._config = config.forBrowser(this.id); | ||
this._debug = config.system.debug; | ||
this._session = null; | ||
this._callstackHistory = null; | ||
this._state = { | ||
...opts.state, |
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.
Поле теперь прокидываю в state
. Посчитал, что нет необходимости в отдельном поле.
this._browser = await this._browserAgent.getBrowser({ state }); | ||
|
||
// use correct state for cached browsers | ||
if (this._browser.state.testXReqId !== state.testXReqId) { |
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.
Бага исправляется за счет этого куска кода. Сюда из браузер пулов может прилететь закешированный браузер в котором находится свой testXReqId
и мне его нужно переопределить.
Мне этот if
не очень нравится, но нормального решения не нашел. Пытался прокидывать state
через массу браузер пулов (cached, limited, basic, ...), но выглядит очень топорно и сложно. Поэтому решил остановится на этом варианте.
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.
а как эти testXReqId-ы вообще могут совпасть, если ты в state-е его новый генеришь?
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.
Они не совпадают. Проблема возникает когда используется 1 инстанс браузера на N тестов. В первом тесте выполняется генерация testXReqId
и запись его в состояние браузера. А следующий тест уже использует существующий инстанс браузера в котором имеется testXReqId
от первого теста. В итоге testXReqId
от второго теста никуда не попадает. Т.е. он генерится, но из cached пула возвращается существующий браузер.
Как вариант можно выполнять applyState
внутри этого самого caching пула.
@@ -69,7 +69,6 @@ module.exports = class ExistingBrowser extends Browser { | |||
|
|||
quit() { | |||
this._meta = this._initMeta(); | |||
this._state = { isBroken: false }; |
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.
Вот это бесполезная строка была. Решил удалить ее.
In previous PR - #819 I did not take into account the case with the correct specification of
testXReqId
when using the same browser for several tests. The problem was that the browser was cached and its fieldtestXReqId
was not modified for next test.