Conversation
Reviewing a change to this theme means opening an admin and looking at it, which is slow enough that it does not happen. This shoots the looking. `rake screens` generates a throwaway Rails app with ActiveAdmin installed, compiles the theme twice — once from the pull request's base, once from its head — and takes the same set of screenshots against each. A pull request then gets one comment with a before/after row per scenario, so the whole visual effect of a change is on the page without checking anything out. Three decisions worth stating: * The dummy app supplies markup only. The theme is compiled with sassc and swapped into the page at screenshot time rather than wired through the app's asset pipeline, so "before" and "after" differ by exactly one stylesheet and switching revisions costs a recompile instead of a rebuild. It also keeps sassc-rails and any opinion about host asset pipelines out of this. ActiveAdmin is taken as `~> 3.0`, so the gallery tracks the latest 3.x without anyone remembering to bump it. * Scenarios are data (test/screens/scenarios.rb): a name, the variable overrides to compile with, a path, what to hover or click, and what to crop. Adding a case is adding a hash. The fifteen here cover the menu and its dropdowns, the title bar, the index table and its tool row, pagination, the filter sidebar, forms and show pages — under light, dark-panel, light-panel and enlarged configurations. * No pixel comparison against golden files. Font rendering differs between machines, so a byte-exact baseline would mostly produce false alarms and teach everyone to ignore it. Identical digests only decide whether a scenario is listed or folded away as untouched; a human looks at the rest. Two workflows rather than one because both open theme pull requests come from forks: Screens runs in the pull request's context, where the token is read-only, and leaves an artifact; Screens comment picks it up afterwards with the repository's own permissions. Images are published as assets on a prerelease, so they stay out of the git history — nothing is committed to a branch and a clone does not carry them. Ferrum drives Chrome over CDP from Ruby, so CI needs no Node toolchain, only the browser the runner already has.
ruby/setup-ruby with bundler-cache writes .bundle/config into the repository with frozen and a vendored path. The generated app and the bootstrap Gemfile live under the repository, so bundler walked up and applied a lockfile that does not describe them: CI could not resolve rails for the dummy app at all.
ruby/setup-ruby exports several; missing one left the bootstrap Gemfile resolving in local mode against the repository's lockfile and failing to find rails at all.
Pinning Rails 7.2 meant installing a second Rails and then fighting over which one the shim resolved to. Rails 8 runs ActiveAdmin 3.5.2 fine; the one thing it stopped doing is writing app/assets/config/manifest.js for --asset-pipeline=sprockets, which sprockets-rails refuses to boot without, so the generator writes it. No version pin left to drift.
ruby/setup-ruby writes .bundle/config into the working directory and bundler walks up from wherever it runs, so an app generated under tmp/ inherited a lockfile that does not describe it and resolved in local mode — CI could not install the app's own gems at all.
Clearing BUNDLE_* by hand missed whatever ruby/setup-ruby adds, and the symptom was bundler resolving the dummy app in local mode: it refused to fetch the app's gems and reported them as not installed. with_unbundled_env restores the environment as it was before bundler touched it, which is what this needs and does not have to be kept in step with the CI action.
Gem.bin_path in the parent found the railties vendored for this gem, which is not on the load path once with_unbundled_env drops the bundle, so exe/rails could not require rails/cli. Install Rails as an ordinary gem and let the unbundled child resolve it.
The runner is slower than a developer machine and timed out on one page, which failed the job and cost the whole gallery. Raise Ferrum's timeouts, and report a scenario that could not be shot in the comment instead of aborting — only an entirely empty run is a failure now.
Member
Author
|
Closing: the CI half is not worth what it costs here. The screenshots are just as useful generated locally, and getting The tooling lives outside the repository now and shoots per commit rather than just base-vs-head, which is more useful for reviewing a branch anyway. What this PR did establish is worth keeping in mind: shooting against a real ActiveAdmin admin, rather than hand-written markup, immediately caught |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewing a change to this theme means opening an admin and looking at it, which is slow enough that in practice it does not happen. This shoots the looking.
rake screensgenerates a throwaway Rails app with ActiveAdmin installed, compiles the theme twice — once from the PR's base, once from its head — and takes the same set of screenshots against each. The PR then gets one comment with a before/after row per scenario.What a reviewer sees
One section per scenario that the PR actually changes, each with a line saying what to look at:
Scenarios the PR leaves untouched are collapsed into a
<details>list rather than padding the comment — decided by digest, since both revisions are shot on the same runner in the same job.Design
The dummy app supplies markup only. The theme is compiled with
sasscand swapped into the page at screenshot time instead of being wired through the app's asset pipeline. "Before" and "after" then differ by exactly one stylesheet, switching revisions costs a recompile rather than a rebuild, andsassc-rails— plus any opinion about how a host project builds assets — stays out of it. ActiveAdmin is taken as~> 3.0, so the gallery follows the latest 3.x without anyone remembering to bump it.Scenarios are data.
test/screens/scenarios.rbis a list of hashes: a name, the variable overrides to compile with, a path, what to hover or click, what to crop. Adding a case is adding a hash; there is no code to touch.The fifteen here cover the menu and its dropdowns, the title bar, the index table and its tool row, pagination, the filter sidebar, forms and show pages — under default, light-panel, dark-panel, split-colour and enlarged configurations.
No pixel comparison against golden files. Font rendering differs between machines, so a byte-exact baseline would mostly produce false alarms and train everyone to ignore the job. Digests only decide whether a scenario is shown or folded away; a human looks at the rest.
Why two workflows
Both open theme PRs come from forks, and a
pull_requestrun on a fork gets a read-only token and no secrets, so it cannot comment.Screenstherefore runs in the PR's own context and only leaves an artifact;Screens commentpicks it up onworkflow_runwith the repository's permissions and does the publishing. This is the standard split for fork-safe PR feedback.Images are published as assets on a
screensprerelease. Release assets are not part of the git history, so nothing is committed to a branch and a clone does not carry them.Ferrum drives Chrome over CDP from Ruby, so CI needs no Node toolchain — only the browser the runner already has.
Checked locally
Full run against a real ActiveAdmin 3.5.2 admin:
It has already paid for itself once: shooting #51 against a real admin showed
$skinMenuPillTextColordefaulting to#ffffffindependently of$skinMenuTextColor, so light-theming the menu left the top-level pill label white on a light fill. That is a defect reading the diff did not catch, and it is fixed on that branch.On this PR the gallery should report no changed scenarios — the theme is untouched here, so it doubles as a smoke test of the pipeline.
One thing this PR cannot prove about itself
Screensis green here and uploads its artifact — 30 PNGs, 15 scenarios twice.Screens commentcannot run yet: GitHub only triggersworkflow_runfrom theworkflow file as it exists on the default branch, so the comment half starts
working on the first PR after this merges, and this PR will not comment on
itself. The artifact on the run above is the same input that half consumes, so
it can be inspected now if you want to see the images before merging.
Follow-up
docs/pr-47-fixes/preview.rbon #51 is an earlier, hand-rolled version of this against static markup. Once both land it can go, with the README there pointing atrake screensinstead.