Skip to content
This repository was archived by the owner on Sep 21, 2022. It is now read-only.

Do not get browser session for suites without states#326

Merged
j0tunn merged 4 commits intomasterfrom
fix/skipped.suite
Dec 10, 2015
Merged

Do not get browser session for suites without states#326
j0tunn merged 4 commits intomasterfrom
fix/skipped.suite

Conversation

@j0tunn
Copy link
Copy Markdown
Contributor

@j0tunn j0tunn commented Dec 8, 2015

  • Refactoring: moved actual calls for get/free browser from BrowserRunner to SuiteRunner
  • SuiteRunner hierarchy:
    • RegularSuiteRunner for suites with states
    • StatelessSuiteRunner for suites without states

Fixes #316 and makes it easy to fix #315

@levonet levonet added the review label Dec 8, 2015
@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Dec 8, 2015

@SwinX @sipayRT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Может быть сделать эту переменную а-ля приватной и геттер для нее

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

зачем?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

упс2!

@SwinX
Copy link
Copy Markdown
Contributor

SwinX commented Dec 10, 2015

Концептуально - не очень нравится, что по сути нарушена область ответственности раннеров. Сейчас браузер получает стейт раннер, а не браузер раннер, хотя это действие не про стейт раннер. Решение на уровне выпрямителя (тупо не передавать сьюты без стейтов) решает проблему ответственности, но я понимаю, что под текущую реализацию ложится задача про ничего не делать для скипнутых сьютов.

Может быть стоит посмотреть в сторону решения про диспетчер? Он будет реализовывать логику про необходимость идти дальше по раннерам или же отбить сьют.

У него может быть интерфейс типо

dispatcher.shouldProceed(suite);
dispatcher.notifyDiscardIfNeeded(suite);

Что думаешь @j0tunn ?

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Dec 10, 2015

Не совсем так, область ответственности не нарушается, я просто немного поменял API, сместил время запроса сессии браузера и сменил стратегию получения браузера с push на pull.
SuiteRunner раньше получал браузер, в котором он и выполнял действия. Этот браузер для него предварительно кто-то поднимал, а потом отпускал (push).
Сейчас SuiteRunner получает браузер по запросу (pull), при этом он по прежнему ничего не знает о том что это за браузер и как он поднимается. Ему просто сказали "эй, чувак, будет нужен браузер - попроси здесь"

Диспетчер же, как мне кажется несколько усложнит итак не очень простую логику

@j0tunn
Copy link
Copy Markdown
Contributor Author

j0tunn commented Dec 10, 2015

под текущую реализацию ложится задача про ничего не делать для скипнутых сьютов.

собственно я изначально эту задачу и решал, просто потом уперся в то, что нам сначала нужно поправить skip

@SwinX
Copy link
Copy Markdown
Contributor

SwinX commented Dec 10, 2015

В принципе да, всё так. За твоё решение ещё говорит то, что SuiteRunner - стратегия, которая инкапсулирует ifы внутри гипотетического диспетчера.

По коду - мне 🆗 Давай вливать.

j0tunn added a commit that referenced this pull request Dec 10, 2015
Do not get browser session for suites without states
@j0tunn j0tunn merged commit 88ed333 into master Dec 10, 2015
@levonet levonet removed the review label Dec 10, 2015
@j0tunn j0tunn deleted the fix/skipped.suite branch December 10, 2015 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

For suites without states are processed along with suites with states For skipped suite all actions except capture are being performed

6 participants