Feat/readme screenshot - #114
Conversation
|
Thanks, makes sense. 2, 3 and 4: agreed. 4 is in the latest commit, the generated image now goes to For 1, you're right that a bigger tolerance is the wrong fix. Idea: generate inside Is that roughly what you meant or were you after something lighter? Also merged current main into the branch. The PDF.js CSP changes did not move the screenshot, still zero pixels difference locally. |
|
I think your container idea can work, but before deciding I would prefer to measure the cost. Since this check will run on pull requests, could you test both approaches in CI?
Then we can compare the workflow time and see if the extra container setup and Without measuring it in this repository, I think we would just be guessing which approach is better. |
|
I also noticed that you updated the branch by merging For this kind of update, I suggest using rebase instead of merge. It keeps the PR history linear and avoids an extra merge commit that is only there to bring the branch up to date with A common flow would be: git fetch upstream
git rebase upstream/main
git push --force-with-leaseIf there are conflicts, resolve them during the rebase, run git rebase --continueUsing |
c932a6d to
ea74a0a
Compare
|
Measured both in the same run: Container job: 39s — image pull 26s, The 26s pull is basically paid for by the 22s of Interesting bit: the container reports 2.2648% against the committed image, the runner 1.9069%. Both Linux, different fonts. So Linux alone isn't enough, it has to be the same pinned image on both sides. |
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
…ntainer Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
2a09086 to
a24bb24
Compare
|
Both done. 900x1000 now, the header block is a fixed ~320px so making it shorter just gave the PDF less room, narrower plus taller worked better. Regenerated after #107, the action icons are visible now. |
There was a problem hiding this comment.
Thanks for the benchmark. The numbers are enough to choose the direction.
Please keep the screenshot validation in the separate pinned Playwright container job. It runs in parallel and, based on your measurements, does not add relevant wall-clock time to the workflow.
For the final implementation, please:
- remove
npm run screenshots:checkfrom the existing e2e job, so the screenshot is checked only once; - keep the separate container job as the screenshot validation job;
- regenerate
img/screenshot/demo.pngusing the same pinned container environment used by CI, so the committed baseline and CI use the same browser/fonts/rendering environment; - make sure the screenshot job uploads the generated screenshot and visual diff when it fails;
- address the remaining open review comment in
scripts/check-screenshots.mjsso the generated screenshot is also saved when the image dimensions are different.
After these changes, the screenshot workflow should be stable and green. The other review comments about the viewport and regenerating after #107 are already resolved.
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
|
Good catch, fixed. It writes |
|
After applying these changes:
|
Signed-off-by: Maximilian Mayer <office@maximilianmayer.at>
|
All green now. Screenshot is generated in the pinned container and the validation job passes with zero pixel difference. Needed one fix to get there: two container runs differed by 0.69%, and the only difference was the "Add Signature" button still showing its active label. Now waiting for it to flip back before the screenshot. |
Closes #83.
Screenshot is now a file in the repo,
npm run screenshots:updateregenerates it andnpm run screenshots:checkcompares. The check is a step in the existing Playwright workflow, and the diff image lands intest-results/, which already gets uploaded.I went with plain scripts instead of a spec, because a test writing into the repo felt wrong: every local
test:e2ewould quietly rewrite the committed image. Put them inscripts/next to the other two. Viewport and locale come fromdevices['Desktop Chrome']so nothing is copy-pasted out of the Playwright config.Tolerance is 0.1%. Two runs here give a byte-identical file, and shifting the signature by 20px gives 0.24%, so that felt like a reasonable spot. My reference image is from macOS though and CI regenerates on Linux, so the first run might say otherwise.
One thing: the empty white box above the signature is the invisible icon from #107. If that goes in first I'll regenerate.