Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions .github/workflows/ci-software-factory.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,21 +80,30 @@ jobs:
shopt -s dotglob
cp -a .test-web-assets-artifact/. ./
- name: Install Playwright Browsers
# `--with-deps` bundles two unrelated operations: fetching the browsers
# from Microsoft's CDN, and apt-installing the system libraries they
# link against. Only the second reaches Ubuntu's mirrors, and that is
# where this step stalls — it has sat here for as long as 170 minutes,
# holding a runner, while the browser download itself has never been
# implicated. Splitting them bounds the half that fails without
# touching the half that works.
# Browsers only. `install-deps` apt-installs 182 packages (114 MB) from
# Ubuntu's mirrors, and that half is what stalls: it has held a runner
# for as long as 170 minutes, and even bounded by a timeout it burned
# five Software Factory shards in a single day, each failing before any
# test ran.
#
# A healthy install of both takes about a minute, so five is generous.
# The deps half is still required: without it Playwright reports "Host
# system is missing dependencies to run browsers".
# What it installs is fonts and GStreamer media codecs. These specs use
# Playwright's default Chromium and neither play media nor render
# non-Latin text, and the runner image already carries what Chromium
# links against — the Matrix suite in ci.yaml has always run Chromium
# with no deps step. Measured over all three shards without it: 62
# tests pass, the same as with it, and the step takes ~22s rather than
# timing out at five minutes.
Comment on lines +84 to +95

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.

[Claude Code 🤖] Non-blocking, this PR.

Roughly half of this comment measures the change rather than stating the contract the step keeps, and it rots on the next spec added. 62 tests pass, the same as with it and ~22s rather than timing out at five minutes both compare against a step that stops existing when this lands, so a reader a year out has nothing to anchor them against — and the count is wrong the moment a spec is added. burned five Software Factory shards in a single day and has always run Chromium are the same class.

The durable half is already written: what install-deps pulls (fonts and GStreamer codecs), that these specs neither play media nor render non-Latin text, that the runner image carries the libraries Chromium links against with the Matrix job in ci.yaml as the standing demonstration, and the symptom that would justify reinstating the step. Suggest keeping that and cutting the before/after numbers — the incident count and the measurements are what the commit message is for, per the evergreen-comments skill.


Generated by Claude Code

#
# If a future spec needs media playback or CJK glyphs, the failure will
# be a missing codec or tofu boxes rather than a browser that cannot
# launch — reinstate the deps step for that, ideally cached.
timeout-minutes: 10

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.

[Claude Code 🤖] Non-blocking, this PR.

The sentence that justified this ceiling left with the deps half ("a healthy install of both takes about a minute, so five is generous"), leaving an unexplained ten minutes bounding a step whose only remaining work is a CDN fetch that the previous comment recorded as never having been implicated in a stall. In a file where every other knob carries its rationale, this one now reads as arbitrary — either tighten it to something near the observed cost so a wedged download fails fast rather than holding a runner, or add the clause saying what the ten minutes is protecting against.


Generated by Claude Code

run: |
pnpm exec playwright install
timeout 5m pnpm exec playwright install-deps
# Chromium only. Bare `playwright install` fetches Firefox and WebKit
# too, which these specs never launch — and it is those browsers'
# libraries (libgtk-4, libgst*, libflite*) that Playwright's host
# validation then warns are missing, which reads alarmingly like a
# broken install rather than two unused browsers.
run: pnpm exec playwright install chromium

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.

[Claude Code 🤖] Non-blocking; this PR or a follow-up, your call.

The argument that drops Firefox and WebKit also drops the full Chrome for Testing build. In playwright-core's registry, the Chromium browser type resolves to chromium-headless-shell whenever no channel is set and headless is true, so every launch in this job lands on the shell: the Playwright specs, boxel test's chromium.launch({ headless: true }) in packages/boxel-cli/src/lib/test-engine.ts, and screenshot-execution.ts. Nothing under packages/software-factory or packages/boxel-cli sets a channel or headless: false, so the full browser this step downloads is never launched here — the headed script runs on a developer's machine, not on this runner.

pnpm exec playwright install --only-shell chromium installs the shell alone, and it is also what the local path already asks for: scripts/factory-setup.ts installs chromium-headless-shell and preflight.ts refuses to run the factory unless a chromium_headless_shell-* build is present. As written, CI and the factory's own setup name different install targets for the same requirement.


Generated by Claude Code

working-directory: packages/software-factory
- name: Run Node tests
if: ${{ matrix.shard.index == 1 }}
Expand Down