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.

Sign up for GitHub

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

Jump to bottom

feat: ability to run tests in isolated environment #792

Merged

DudaGod merged 2 commits into master from HERMIONE-1199.chrome_isolation

Oct 17, 2023

Member

DudaGod commented Oct 10, 2023

What's done?

added ability to run tests in isolated environment (incognito browser context) which currently works only in chrome@93 and higher.

Why?

allows not to think about clearing the state after each test execution. And much more faster than using testsPerSession: 1

DudaGod force-pushed the HERMIONE-1199.chrome_isolation branch from 6a49e98 to 1c0d736 Compare

October 10, 2023 08:52

DudaGod commented

View reviewed changes

README.md Outdated

@@ @@ -841,6 +841,7 @@ Option name | Description @@
               `key`                     | Cloud service access key or secret key. Default value is `null`.
               `region`                  | Ability to choose different datacenters for run in cloud service. Default value is `null`.
               `headless`                | Ability to run headless browser in cloud service. Default value is `null`.
+              `isolation`               | Ability to execute tests in isolated clean-state environment ([incognito browser context](https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createBrowserContext)). Default value is `false`.

Member Author

DudaGod Oct 10, 2023

Возможно опцию правильнее назвать isolate

Member Author

DudaGod Oct 10, 2023

todo: выполнить doctoc

Member

KuznetsovRoman Oct 11, 2023

Почему по умолчанию false, если мастер сейчас на бете восьмой версии? Давай сразу ещё один breaking change сделаем

Member Author

DudaGod Oct 11, 2023

Хотел отдельным PR-ом переключить в hermione@8 с false на true, чтобы удобно чери пикнуть текущий коммит в hermione@7 и никому ничего не сломать. Но можно просто отдельный коммит сверху докинуть. Наверное так и сделаю.

src/browser/existing-browser.js

+                          return;
+                      }
+                      const { browserName, browserVersion = "", version = "" } = sessionCaps;

Member Author

DudaGod Oct 10, 2023

В sessionCaps хранится результат ответа браузера. Т.е. он содержит реальные версии браузеров.

version использую в случае если пользователь запускает jsonwp браузер.

src/browser/existing-browser.js

+                      const incognitoCtx = await puppeteer.createIncognitoBrowserContext();
+                      const page = await incognitoCtx.newPage();
+                      if (automationProtocol === WEBDRIVER_PROTOCOL) {

Member Author

DudaGod Oct 10, 2023

Переключение между окнами нужно только в webdriver протоколе. В devtools с этим проблем нет.

src/browser/existing-browser.js

+                      if (automationProtocol === WEBDRIVER_PROTOCOL) {
+                          const windowIds = await this._session.getWindowHandles();
+                          const incognitoWindowId = windowIds.find(id => id.includes(page.target()._targetId));

Member Author

DudaGod Oct 10, 2023

Тут приходится использовать приватное свойство _targetId. Очень странно, что оно не публичное. Мне оно нужно чтобы получить id нового открытого окна в который необходимо переключится. Изначально пытался переключаться в последнее окно из результата getWindowHandles, но оказалось, что есть кейсы когда последнее окно не является только что созданным. Поэтому нашел только такой вариант.

Еще id не матчатся один к одному. В webdriver они называются CDwindow_12345, а в cdp просто 12345. Issue про сделать данное поле публичным заведу в puppeteer и опишу свой кейс использования.

Версия wdio у нас четко зафиксирована поэтому кейсов когда это может сломаться на данный момент нет.

src/browser/existing-browser.js

+                      for (const ctx of browserCtxs) {
+                          if (ctx.isIncognito()) {
+                              await ctx.close();

Member Author

DudaGod Oct 10, 2023

Инкогнито контексты можно сразу закрывать без необходимости явно закрывать все страницы.

src/browser/existing-browser.js

+                          }
+                          for (const page of await ctx.pages()) {
+                              await page.close();

Member Author

DudaGod Oct 10, 2023

Не инкогнито контексты закрыть невозможно. В данном случае не инкогнито контекстом является дефолтный контекст, который запускается при запуске браузера. Его закрыть нельзя, можно закрыть только страницы.

src/constants/browser.ts Outdated Show resolved Hide resolved

test/src/browser/existing-browser.js

@@ @@ -390,6 +398,156 @@ describe("ExistingBrowser", () => { @@
                           });
                       });
+                      describe("perform isolation", () => {
+                          let cdp, incognitoBrowserCtx, incognitoPage, incognitoTarget;

Member Author

DudaGod Oct 10, 2023

Не очень люблю создавать сущности в beforeEach, но иначе некоторые тесты получались довольно жирные.

KuznetsovRoman requested changes

View reviewed changes

Member

KuznetsovRoman left a comment

В текущей реализации пользователю нужно в и так не маленькой документации изучать, что это за опция, как оно себя будет вести с testsPerSession: 1 и infinity.

Что не нравится:

isolation по умолчанию false. Почему бы не включить изначально, если вливаем в бету?
присутствие такой опции в принципе. Опять же, если можно по умолчанию ее включить и нет смысла ее выключать, то выглядит как лишняя
testsPerSession ты указываешь, что при использовании изоляции можно поставить в infinity. А какие есть причины так не делать? Кажется, что никаких, и этот testsPerSession можно под капотом в нужных браузерах ставить в Infinity

По итогу получится, что с 8 версии Гермионы оно работает как нужно из коробки. Пользователей может смутить различие в :visited, но тут предлагаю в testsPerSession просто указать, что в новых хромах этот параметр игнорируются. Так сферический пользователь в вакууме, у которого возникнут вопросы по цвету посещённых ссылок, сможет прочитать ответ в документации, вместо того, чтобы самому разбираться в этих параметрах и браузерных контекстах в целом. Не-сферический пользователь не-в-вакууме просто оставит конфиги по умолчанию и не будет пользоваться такой классной штукой.

README.md Outdated

@@ @@ -841,6 +841,7 @@ Option name | Description @@
               `key`                     | Cloud service access key or secret key. Default value is `null`.
               `region`                  | Ability to choose different datacenters for run in cloud service. Default value is `null`.
               `headless`                | Ability to run headless browser in cloud service. Default value is `null`.
+              `isolation`               | Ability to execute tests in isolated clean-state environment ([incognito browser context](https://chromedevtools.github.io/devtools-protocol/tot/Target/#method-createBrowserContext)). Default value is `false`.

Member

KuznetsovRoman Oct 11, 2023

Почему по умолчанию false, если мастер сейчас на бете восьмой версии? Давай сразу ещё один breaking change сделаем

src/constants/browser.ts Outdated Show resolved Hide resolved

test/src/browser/existing-browser.js

+                                  assert.notCalled(logger.warn);
+                              });
+                              it("test wasn't run in chrome", async () => {

Member

KuznetsovRoman Oct 11, 2023

А как насчёт других браузеров, поддерживающих cdp? Типа Firefox/webkit

Member Author

DudaGod Oct 11, 2023

Так как коммент оставлен к тесту, то не вижу необходимости проверять другие варианты browserName.

Если вопрос был в целом, то webkit вообще не поддерживает CDP. А firefox имеет поддержку только в nightly сборке. Поэтому их поддерживать не стал.

Member Author

DudaGod commented Oct 11, 2023

isolation по умолчанию false. Почему бы не включить изначально, если вливаем в бету?

Забыл написать, что хотел отдельный PR в hermione@8 оформить (хотел коммит черипикнуть в hermione@7). Но докину просто дополнительный коммит с переключением в true

присутствие такой опции в принципе. Опять же, если можно по умолчанию ее включить и нет смысла ее выключать, то выглядит как лишняя

Пользователь может осознанно хотеть запускать зависимые тесты и чтобы состояние браузера не чистилось. Из-за этого я даже пилил такой плагин - https://github.com/gemini-testing/hermione-sequential-tests-run. Поэтому опцию я бы не стал удалять.

testsPerSession ты указываешь, что при использовании изоляции можно поставить в infinity. А какие есть причины так не делать? Кажется, что никаких, и этот testsPerSession можно под капотом в нужных браузерах ставить в Infinity

Для hermione@8 можно в этом кейсе только дефолт поменять в случае если изоляция не выключена и версия браузера удовлетворяет условию. Но если пользователь явно укажет testsPerSession, то это значение я не должен переопределять.

KuznetsovRoman approved these changes

View reviewed changes

DudaGod added 2 commits

October 17, 2023 23:41


          feat: ability to run tests in isolated environment

0f12eb3


          feat: run tests in isolated env in chrome@93 and higher by default

473e96a

BREAKING CHANGE:
- tests in chrome@93 and higher now run in as isolated environment by default

DudaGod force-pushed the HERMIONE-1199.chrome_isolation branch from 1c0d736 to 473e96a Compare

October 17, 2023 21:01

DudaGod merged commit 05ec28e into master

2 checks passed

DudaGod deleted the HERMIONE-1199.chrome_isolation branch

October 17, 2023 21:03

DudaGod mentioned this pull request

feat: ability to run tests in isolated environment #799

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet