Run tests in CI, and fix fonts for snapshot testing #233
Conversation
|
Discussion in #masonry > Snapshot test differences in Masonry CI |
crates/masonry/src/render_root.rs
Outdated
| // TODO: Is this the right place for this? | ||
| pub fn add_test_font( |
There was a problem hiding this comment.
Seems reasonable. RenderRoot is the type ultimately responsible for turning semantic data into bitmaps. You can either pass a test font with a constructor argument, a build method, or a setter. I think a setter is fine.
|
Will try to run this on my machine this afternoon, then review. |
|
Ooh, a time limit to get it working? Exciting! |
DJMcNab
force-pushed
the
tests-ci
branch
2 times, most recently
from
a383a57 to
7f617da
Compare
|
The status of this is that there's a lot more work needed on testing, including moving this into Xilem. However, this does get our existing tests working in a cross-platform way. One concern I have with the size of screenshot tests. I'm planning on raising this in office hours, to see if anyone has any good solutions, so won't merge until them. |
|
Unit tests pass on my machine! 🎉 (PopOs 22.04, RTX 2070 with X11, for what it's worth.) |
|
I'm going to postpone merging this PR until after I port linebender/vello#647 to this repository, to avoid adding the new snapshots into main as-is. I think I'll do that in a different PR, because it also has implications on other caching choices, and shouldn't be controversial. |
|
Makes sense, yes. FWIW the developer experience of git-lfs in Vello seems pretty good to me. |
masonry/src/testing/harness.rs
Outdated
| if std::env::var("SKIP_RENDER_SNAPSHOTS").is_ok_and(|it| !it.is_empty()) { | ||
| // FIXME - This is a terrible, awful hack. | ||
| // We need a way to skip render snapshots on CI and locally | ||
| // until we can make sure the snapshots render the same on |
There was a problem hiding this comment.
The comment should probably be changed to reflect our likely best practices (eg test render snapshots by default, skip them if we have LFS-related problems). The "FIXME" part should be removed.
There was a problem hiding this comment.
I'll do that when I rebase this past #475
| let new_image: DynamicImage = self.render().into(); | ||
|
|
There was a problem hiding this comment.
I don't remember: is there a reason render runs after the SKIP_RENDER_SNAPSHOT early-exit? It feels like even if we wanted to skip the "compare pixels" part, we'd still want to exercise the painting code in case we find panics there.
There was a problem hiding this comment.
I'm not sure. I'm wanting to redo a lot of this testing code, but I think that will want to come after linebender/rfcs#7
There was a problem hiding this comment.
Wait, no, I misread the code. We already redraw in the early exit, the only thing this skips is actually sending the Vello scene to GPU. So that makes sense.
Using the attribution at https://fonts.google.com/attribution
This code is imo worse, but oh well
Unclear why the status of these has changed
Since if you can render, the snapshots will work
|
Requested review just to get final sign-off on the latest pushed changed (since ac7549d) |
| let new_image: DynamicImage = self.render().into(); | ||
|
|
There was a problem hiding this comment.
Wait, no, I misread the code. We already redraw in the early exit, the only thing this skips is actually sending the Vello scene to GPU. So that makes sense.
| #[cfg_attr( | ||
| not(debug_assertions), | ||
| ignore = "This test doesn't work without debug assertions (i.e. in release mode). See https://github.com/linebender/xilem/issues/477" | ||
| )] |
There was a problem hiding this comment.
I don't think you need to link to the issue. Doing a cfg_attr fixes it.
For the message I'd write something like:
"Safety rails are disabled when debug_assertions is false (i.e. in release mode)."
See also linebender/vello#439
This should open the door to doing much more comprehensive testing at higher level in the nearish future (including e.g. in
xilem_masonry)