-
Notifications
You must be signed in to change notification settings - Fork 0
First implementation #1
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
Conversation
Testplane run finisnedTestplane HTML-report is available at https://gemini-testing.github.io/gh-actions-testplane/testplane-reports/2025-02-12/13287216451/41 |
a3354f1
to
a32aff2
Compare
6a5bc27
to
a32aff2
Compare
@@ -0,0 +1,41 @@ | |||
name: GitHub action publish new version |
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.
Флоу для релиза новой версии github action
Собирает новую версию action и коммитит ее в мастер
@@ -0,0 +1,65 @@ | |||
name: Testplane CI |
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.
Тестовый флоу.
- Тестирует юнит тестами
- Прогоняет линтер
- Билдит action
- Запускает этот action на тестовом проекте
- Выкладывает html-отчет на gh-pages
- Оставляет комментарий со ссылкой на отчет
@@ -1 +1,2 @@ | |||
node_modules | |||
dist/dev.js |
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.
Собирается с npm run build
, запускается разработчиком
На самом деле, локально билдить какого-то смысла нет, кроме как "проверить, что оно собирается"
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.
А почему в игноре не весь dist?
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.
dist/index.js
будет билдиться и комиттиться.
Так же, как в npm registry
коммитится build
, так же и в github коммитится сбилженный github action
Вот пример github action (https://github.com/actions/cache), у них у всех dist
коммитится в VCS, откуда же и запускается, когда разработчик пишет что-то вроде "uses: actions/cache@v4"
@@ -0,0 +1 @@ | |||
v20.18.1 |
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.
Максимальная версия, поддерживаемая github actions
@@ -0,0 +1,43 @@ | |||
name: 'Testplane action' |
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.
Конфиг github action
Описывает входные и выходные параметры
return; | ||
} | ||
|
||
if (this.needsBrowsersCache && !cache.isFeatureAvailable()) { |
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.
gh сервис кэширования может быть недоступен
|
||
core.debug(`Testplane browsers cache primary key: "${this.cachePrimaryKey}"`); | ||
|
||
this.init = () => Promise.resolve(); |
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.
Второй раз init
не нужен
const commandOutput = await this.pm.getExecOutput(this.withConfig(["testplane", "--version"])); | ||
const semverNumbers = commandOutput.split(".").map(Number) as [MajorVersion, MinorVersion, PatchVersion]; | ||
|
||
this.version = () => Promise.resolve(semverNumbers); |
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.
Результаты подобных команд не изменяются в процессе работы action, так что их кэшируем
|
||
core.debug("Running Testplane install-deps"); | ||
|
||
await this.pm.exec(this.withConfig(["testplane", "install-deps"]), { silent: 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.
silent: false
включает вывод логов команды в stdout/stderr
const dateNow = new Date(); | ||
|
||
const year = dateNow.getUTCFullYear(); | ||
const month = String(dateNow.getUTCMonth() + 1).padStart(2, "0"); |
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.
По умолчанию getUTCMonth
выдает значения [0, 11]
431cf70
to
8939450
Compare
"type": "module", | ||
"scripts": { | ||
"build-base": "rollup --config rollup.config.ts --configPlugin @rollup/plugin-typescript", | ||
"build": "npm run build-base -- --file dist/dev.js", |
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.
Билдит action
, чтобы "проверить, что билдится". Запускается локально.
Локально нет возможности запустить из-за тесной интеграции с @actions
модулями
package.json
Outdated
"scripts": { | ||
"build-base": "rollup --config rollup.config.ts --configPlugin @rollup/plugin-typescript", | ||
"build": "npm run build-base -- --file dist/dev.js", | ||
"build-action": "npm run build-base -- --file dist/index.js", |
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.
так может и назвать ci:build
? (не критично)
sourcemap: false, | ||
plugins: [terser()], | ||
}, | ||
plugins: [typescript(), nodeResolve(), json(), commonjs()], |
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.
Просто интересно: а зачем плагин commonjs, если вся репа — ESM?
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.
Это супер неочевидно, но для того, чтобы собирать esm нужен плагин commonjs.
В данном случае он у нас из commonjs делает esm (например, @actions/core
- CJS: https://github.com/actions/toolkit/blob/main/packages/core/package.json)
Вот попробовал запустить без этого плагина: https://github.com/gemini-testing/gh-actions-testplane/actions/runs/13285834186/job/37094200943?pr=1
import { Testplane, TestplaneCache } from "./testplane/index.js"; | ||
import { writeFailureSummary, writeSuccessSummary } from "./summary.js"; | ||
|
||
async function main() { |
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.
Детально прочитав весь код, у меня остался главный верхне-уровневый вопрос — в чем ключевое преимущество кастомного action VS страница с докой и примерами флоу, как запустить Testplane (которые можно вставить без изменений и получить готовый флоу).
Судя по коду, задачи этого action:
- Кэширование зависимостей
- Установка зависимостей
- Запуск testplane
- Доп. обработка вывода, улучшение читаемости результатов — summary, структурирование логов
Меня беспокоят такие моменты:
Захватывание установки зависимостей работы с пакетными менеджерами в область ответственности нашего action.
- Я думаю, что это довольно опасный и дорогой подход, потому что пакетных менеджеров много, у каждого множество своих тонкостей и мы 100% что-то не учли.
- Уже сейчас мы захардкодили npm, yarn, pnpm. Если вдруг появится что-то еще, нужно тратить время на поддержку/внедрение.
- На каждом релизе нужно тестировать во всех 3 пакетных менеджерах = лишние сложности.
При этом какой получаем профит? Я как понимаю хотим получить максимально коробочное решение, но какой ценой? Выглядит так, что это в этом месте это может нанести больше вреда, чем пользы.
Решение недоработок интерфейса Testplane абстракциями со стороны action
Если запустить Testplane из CLI и читать его вывод сложно, то может стоит инвестировать в улучшение этого в самом инструменте? Может нам нужен summary-репортер? Если артефакты testplane сложно кэшировать обычным actions/cache
, может упросить этот момент?
Дороговизна поддержки + необходимость реализовывать фичи в этом кубике повторно
Пример: помимо реализации шардирования в Testplane, нужно реализовывать его и здесь. Помимо реализации перезапуска только упавших тестов в Testplane, нужно реализовывать его и здесь. И т.д.
Опыт коллег
Ребята из playwright прошли схожий путь (длиной в 5 лет) с action здесь: https://github.com/microsoft/playwright-github-action
И в итоге пришли к выводу, что лучше всего не иметь action. А просто использовать CLI и написать в доке пример флоу + генерить его при создании проекта
Для меня эти минусы перевешивают простоту и гибкость написания обычного флоу с помощью композиции готовых action'ов типа actions/checkout
, actions/setup-node
, actions/cache
и т.д. При этом юзерам не надо сочинять это каждый раз, если предоставить несколько готовых рецептов под наиболее популярные кейсы.
Возможно у меня тут странная позиция и она идет вразрез с мнением остальных, я не говорю, что это единственный правильный путь.
Но ревью это в том числе про взгляд со стороны, поэтому я предлагаю еще раз обратить внимание на эти моменты и вопросы, и подумать о них.
Однако я не настаиваю на этом пути. Просто озвучиваю свои мысли и взгляд со стороны. Сама задумка крутая и по коду замечаний у меня особо нет.
await cache.saveCache([this.testplaneCachePath], this.getCachePrimaryKey()); | ||
} | ||
|
||
private async init(): Promise<void> { |
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.
Я считаю, что init-методы — небольшой code-smell, поскольку само их наличие означает, что объект может находиться в невалидном состоянии. Как правило, если извлечь логику init из класса и подавать готовые данные в конструктор, то всё становится сильно проще — не нужны проверки, не надо перезаписывать init, и т.д.
.write({ overwrite: true }); | ||
}; | ||
|
||
export const writeFailureSummary = async (postMortemData: PostMortemData) => { |
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.
По коду все супер. Перед влитием предлагаю только подумать над тезисами выше
e704906
to
fefabe4
Compare
fefabe4
to
ffa7968
Compare
No description provided.