Repository navigation
Local docker run - #1338
Local docker run#1338sonic16x wants to merge 18 commits into
Conversation
31261e5 to
4b60012
Compare
commit: |
✅ Testplane browser-env run succeed
|
✅ Testplane E2E run succeed
|
d364192 to
98520eb
Compare
b7b963c to
84dc6a5
Compare
DudaGod
left a comment
There was a problem hiding this comment.
Я посмотрел локальный запуск браузеров в Docker. Ниже построчные замечания по совместимости образов, сети, безопасности и удобству запуска.
| this._sessionIdForLogs = session.sessionId; | ||
| this._wdProcess?.setSessionId?.(session.sessionId); | ||
| const debuggerAddress = session.capabilities["goog:chromeOptions"]?.debuggerAddress; | ||
| if (debuggerAddress && !("browserWSEndpoint" in this._config && this._config.browserWSEndpoint)) { |
There was a problem hiding this comment.
Я бы не делал подготовку CDP обязательной при создании WebDriver-сессии. Пользовательский Chrome-образ может вернуть debuggerAddress, но не иметь devtools-адаптера, которого ожидает prepareCdp(). Тогда даже тест, использующий только WebDriver, не стартует. Можно ли инициализировать CDP только при первом обращении к нему?
| import { prepareDockerBrowserImage } from "./docker-image"; | ||
| import { timeDockerOperation } from "./docker-timing"; | ||
|
|
||
| const PLATFORM = "linux/amd64"; |
There was a problem hiding this comment.
Здесь жёстко задан linux/amd64, поэтому на Apple Silicon нативный arm64-образ будет отвергнут, а multi-platform образ запустится через эмуляцию. Я бы выбирал архитектуру текущего Docker-движка, сохранив возможность явно запросить другую платформу.
There was a problem hiding this comment.
добавил возможность указывать параметром, но по дефолту такой
| await writeFile(configPath, JSON.stringify(browsers)); | ||
| // Set before creation to also clean up when the CLI times out after creating the network. | ||
| networkCreated = true; | ||
| await timeDockerOperation(name, "create network", () => docker(["network", "create", network])); |
There was a problem hiding this comment.
Я бы проверил IPv6 именно в создаваемой здесь сети. Включённый IPv6 у стандартного bridge не означает, что новая пользовательская сеть тоже получит IPv6. Сейчас браузерный контейнер может не открыть IPv6-only сайт, хотя сама VM имеет к нему доступ.
There was a problem hiding this comment.
Добавил --ipv6 флаг
| network, | ||
| // This is the socket path on the daemon's Linux host, including Docker Desktop/Colima. | ||
| "--volume", | ||
| "/var/run/docker.sock:/var/run/docker.sock", |
There was a problem hiding this comment.
/var/run/docker.sock — путь на стороне Docker-демона, а не сокет из выбранного локального Docker context. У rootless Podman он обычно находится в другом месте, поэтому описанный в документации сценарий может не запустить Selenoid. Я бы добавил настройку способа подключения контроллера к движку и проверку этого сценария.
There was a problem hiding this comment.
по умолчанию такой, но добавил возможность менять через параметр
| ...controllerSecurityArgs, | ||
| selenoidImage, | ||
| "-conf", | ||
| "/browsers.json", |
There was a problem hiding this comment.
Я бы передавал Selenoid -disable-privileged по умолчанию. Без этого флага он запускает браузерные контейнеры в privileged-режиме, а здесь пользователь может указать произвольный образ. Если отдельным образам нужны дополнительные привилегии, лучше включать их явно.
| } | ||
|
|
||
| const { selenoidImage } = config || {}; | ||
| if (typeof selenoidImage !== "string" || !selenoidImage.trim()) { |
There was a problem hiding this comment.
Я бы не требовал от пользователя выбирать selenoidImage для базового сценария. Его цель — указать свой совместимый браузерный образ и запустить тесты; образ контроллера выглядит внутренней деталью Testplane. Можно ли дать публичный закреплённый default и оставить это поле для переопределения?
There was a problem hiding this comment.
пока так, мы пока не запушили публичный образ селеноида
| const manager = runs.get(owner); | ||
| if (manager) return manager.acquire(browserId, config, options, closeBrowser); | ||
|
|
||
| await standaloneStopping; |
There was a problem hiding this comment.
Если state.manager.stop() отклонится, standaloneStopping навсегда останется отклонённым Promise. После временного сбоя Docker любой следующий standalone-запуск упадёт на этом await. Я бы очищал состояние после ошибки и добавил тест на повторный запуск.
|
|
||
| it("should route Firefox sessions through /wd/hub inside the browser container", async () => { | ||
| const driver = await start( | ||
| "registry.yandex.net/selenium/firefox:148.0", |
There was a problem hiding this comment.
Я бы заменил адрес внутреннего registry в публичном тесте на нейтральный фикстурный адрес. Для проверки маршрута /wd/hub конкретный реестр не нужен.
There was a problem hiding this comment.
This is a wrong place to add docs. I think we'll remove the docs/ directory in the near future. Docs should live on testplane.io and we've put a lot of effort into this migration.
| ...desiredCapabilities, | ||
| }, | ||
| gridUrl: options.gridUrl || LOCAL_GRID_URL, | ||
| docker: options.docker, |
There was a problem hiding this comment.
I would think once again about the "docker" naming. Are we sure in this one?
For example, let's imagine that we'll need to support podman (a very real possibility). What will we do in that case? Add another podman field that would duplicate all the logic?
Perhaps it's not a problem. But I would seriously consider this possibility & make sure this design is future-proof in this sense.
There was a problem hiding this comment.
keep docker, because it is obvious name
637ad86 to
6f435c3
Compare
…browsers in containers
c922623 to
97832f8
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97832f8f38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab96268ac9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this._sampleState(queued.browserConfig.id); | ||
|
|
||
| this._newBrowser(queued.id, queued.opts).then(queued.resolve, queued.reject); | ||
| this._newBrowser(queued.browserConfig, queued.opts).then(queued.resolve, queued.reject); |
There was a problem hiding this comment.
Restore the queued request's test context before launching
When a browser request is queued behind a pool limit, _launchNextBrowser() runs inside the context of the test releasing the previous browser, rather than the queued test's context. Since request headers now come from getTestContext(), the queued test's session creation and SESSION_START requests receive the previous test's X-Request-ID prefix and traceparent. This incorrectly associates those requests with another test's diagnostics. Reproducing with a one-slot LimitedPool and contexts A and B yields A for both underlying launches. Capture the context when enqueueing and restore it when executing the queued launch.
Useful? React with 👍 / 👎.
No description provided.