Add visual regression reference workflows#631
Conversation
Add local and committed visual regression workflows with separate reference directories. Store Linux baselines, keep local references ignored for fast iteration, run committed comparisons directly on Linux, and provide a CI-like dependency container helper for local macOS and Windows runs.
2b3f55a to
9ff90f7
Compare
| app = MakeraApp() | ||
| app._run_prepare() | ||
|
|
||
| # Disable screen transition animations so page switches are instant |
There was a problem hiding this comment.
I personally feel importing stuff in the middle of a function deserves a comment explaining what's going on. I see there's a lot of these comments that were removed?
There was a problem hiding this comment.
overly eager LLM, sorry. I can bring the comments back.
There was a problem hiding this comment.
These will all render differently on different people's machines, window sizes, platforms, pixel density, etc. Because of this, my approach is to require the runner to first manually generate their reference screenshots before then running against their changes.
There was a problem hiding this comment.
Could you speak to the value of containerization here? This looks like it's a separate list of dependencies from the existing set that the app is built with.
|
|
||
| ```bash | ||
| poetry run python -m pytest tests -q | ||
| poetry run -- python3 -m pytest tests -q |
There was a problem hiding this comment.
I hate python's idiosyncrasies across different operating systems. I would argue this change (all of the python3 vs python command changes) should be done/discussed in a different commit - changing styling/formatting for all commands in the project is out of the scope this commit aims to achieve.
There was a problem hiding this comment.
I can undo it if you insist, I just thought python3 is more portable than python - do Windows python installations not provide it or something?
There was a problem hiding this comment.
It's a mess, depends on how/when you installed and if you need a specific/multiple run time. Generally speaking python3 is a posix specific thing. If you need multiple runtimes, windows instead uses the py.exe launcher.
There was a problem hiding this comment.
https://xkcd.com/1987/ is evergreen. I'll see what I can do about this.
|
Okay, I have a better idea of what you're trying to achieve than at first glance. I'm very opinionated on this, so others should definitely weigh in. I don't believe that trying to generate one canonical set of screenshots is an effective approach in practice. In my experience, this leads to a number of pitfalls that end up causing more harm than good, compared to running visual diffs locally against the individual developer's machines.
If the goal is to have one consistent check to make sure visual changes aren't unintentionally happening against a specific target device, I would suggest that this is best achieved by having the actual GitHub action runner machine run the tests. This can be achieved with the existing approach without any code changes to the actual project codebase - it's just a few lines of gh-actions tasks. |
that's the whole idea behind using the container and Xvfb (which stands for "X virtual framebuffer") - it is a purely software renderer that, at least on paper, should not be affected by the hardware of the machine it is running on.
the main value is CI checks on pull requests. it is a public open-source project after all, having automatic guardrails against someone accidentally breaking the visuals with a seemingly unrelated change is nice, even if those guardrails are not 100% perfect at testing platform-specific behavior. it also saves the developers the tedium of having to remember to generate the local references before starting work and, worse, updating those references if the develop branch has moved on github while their work is underway. having the references checked in to git also safeguards against two or more separate PRs targeting the same visual area and making changes that are fine on their own but result in a mess once both are merged.
I agree with the sentiment, but Linux is the only way to have a baseline that can work on all three development platforms. nothing here is requiring you to use it during the development process - you can still generate the local references and work against them without worrying about Linux until the very end, where you need to regenerate the canonical references if your work results in changed visuals. I'd actually argue it is beneficial as it gives you Linux tests on Mac/Windows that you won't otherwise get, covering two platforms instead of one.
but... this is what this PR also includes, or am I missing something? |
I'm saying this can be accomplished purely in gh-actions without any of the architectural changes. The runner generates images (or grabs artifacts from a previous merged run), builds with the commit changes, runs the new set of images, and checks the diff. That doesn't require any of these changes to accomplish - the runner is a constant machine, so there's no need to maintain containers for different operating systems, commit ephemeral binary files into the codebase, etc. It shouldn't be more than a few additional lines in the runner. If that's your main goal here (which I totally agree with), I think that's a much simpler approach. |
and there is a diff because the commits intentionally change the visuals. what should happen then?
I'm not sure what you mean by this. there's only one container, all it needs is docker/podman. |
to be clear, the purpose of the container itself is to mimic the (Linux) GH action runners on the local developer machine, so the developer can run the tests in an environment that's as close to the CI as possible, and update the committed references when needed without having a separate Linux machine for it. the actions themselves don't use it. |
The commit is flagged for review, not summarily rejected. The runner can even embed the diff image right here in the comments so a reviewer doesn't have to go look at the artifacts.
Sorry, wrong terminology. There's checks for arm/x64, then docker or podman support. That's a matrix of different configurations that need to be tested and maintained. And with each of them, there's the potential for a bug in their code to affect something in ours. On Windows, this requires developers to install WSL2 on top of docker/podman, which has an additional layer of problems that can go wrong in getting your environment set up. To me there are additional maintenance and development costs to adding this, but I don't understand the value we gain with this approach. Yes, you could get your local developer environment closer to a linux machine's output. But, why does that help compared to running the diff locally against how the application actually looks on your machine? I view the gh-action approach as the final sanity check, not the thing you should be running during your active development - for active development, how it runs on your machine is sufficient (and preferred, since multiple devs test against more scenarios than a single machine config). Like I said, I'm opinionated on the subject. I truly do appreciate the effort and explanations, I just have to be convinced of the value here, and then I can/will fully change my opinion. Can you give me a practical example of where this helps a developer write better code? |
|
working with local references is error-prone and relies on the human factor too much, in my opinion. it's a very manual workflow, the developers need to have very good discipline when working with them.
with the container and checked-in references, all it takes is a one-time setup for docker/podman/wsl2, after that it is one command to run the tests and one command to update the references when needed. I agree there's a maintenance cost to having the Containerfile and the runner, but I think it is by far outweighed by having to do a bunch of manual steps every time you are working on the code. I can break up this PR in two, if you prefer. the first part would fix the tests themselves while still using .gitignore'd references - the state isolation/run ordering issues, reliance on the screen geometry (I get test failures when I plug in an external monitor, if the references were generated without it) and other things like that that I forget at the moment. the second part would deal with running these tests on CI, we can discuss how to best achieve it separately. |
|
That sounds like a good approach, splitting this up. I don't think any of your goals are bad, I just think we differ in how best to achieve those outcomes. If you're finding running these scripts burdensome, that needs to be fixed so they'll actually be utilized. I personally think it's less burdensome to stash your changes and re-run the baseline if you need to do that than deal with the headaches WSL creates. The Android build has a dependency that requires it be built in Linux (arrrgh!), and combined with a different issue breaking the builds, I found it much easier to spin up a Mint VM than to keep dealing with WSL's issues. My primary aversion to adding the reference images to git is the bloat it will create to the repo. Each screenshot is roughly 50KB, and being a binary file, each time it's updated it will replace the whole previous file. If we expand test coverage to the entire application, with every screen, menu, dialog, etc, we're looking at potentially hundreds of screenshots. And as changes get added, hundreds or thousands of screenshots are permanently added to the git repository. A small change to styling or color scheme might regenerate them all, and that's 20-40MB permanently in git. It is very easy to see the repository bloat into gigabytes in the future.
The first PR gets merged in to develop. Before the second PR can be merged in, that branch must update from develop, where the tests would be re-run, and the issue identified before it can be merged in. What do you think about this: When gh-runner generates its screenshots, it saves them as a build artifact. We can then include a python script utility in the repo, where you give it a git commit, and it fetches the artifacts. That frees you from the burden of having to stash your changes to regenerate the baseline images. You could even have a post-merge hook so this part is done automatically for you when you pull in changes. |
3 participants
Add local and committed visual regression workflows with separate reference directories. Store Linux baselines, keep local references ignored for fast iteration, run committed comparisons directly on Linux, and provide a CI-like dependency container helper for local macOS and Windows runs.