Conversation
ajakubowicz-canva
left a comment
ajakubowicz-canva
left a comment
There was a problem hiding this comment.
I am so excited for this. I need a bit of help before I can review this as I unfortunately was not able to locally test this.
It would be really awesome if this workflow could be referenced in the vello_tests README somewhere, and if appropriate in the vello_sparse_tests package. I was naively trying to change and break some tests but I couldn't figure out how to preview a diff.
My attempt:
- Change a test within
vello_tests. - Run
cargo test - Run
cargo xtask snapshots review - I probably messed up because I couldn't get a diff.
I also tried to change a test within vello_sparse_tests. I got:
---- basic::full_cover_1_cpu_f32 stdout ----
thread 'basic::full_cover_1_cpu_f32' panicked at sparse_strips/vello_sparse_tests/tests/util.rs:285:9:
test didnt match reference image
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- basic::full_cover_1_cpu_u8 stdout ----
thread 'basic::full_cover_1_cpu_u8' panicked at sparse_strips/vello_sparse_tests/tests/util.rs:285:9:
test didnt match reference image
---- basic::full_cover_1_hybrid stdout ----
thread 'basic::full_cover_1_hybrid' panicked at sparse_strips/vello_sparse_tests/tests/util.rs:285:9:
test didnt match reference image
These also didn't show up in Kompari. So excited for this!
|
Hi, thanks for feedback. I have fix it and slightly change the PR, so in the current version there are now If you broke a Vello test and then run tests and then run I was not aware of separate tests in sparse strips, so they are not included in this PR. I will look at it in the following work. |
|
Would you be able to fix the clippy error in CI: I think you'll need to add the "exclude xtask" as you did in the parley PR: https://github.com/linebender/parley/pull/272/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR158 |
spirali
force-pushed
the
kompari-xtask
branch
2 times, most recently
from
fa454ef to
3df6d92
Compare
Fixed. Sorry for delay. |
ajakubowicz-canva
left a comment
ajakubowicz-canva
left a comment
There was a problem hiding this comment.
This LGTM!
I'm a new maintainer to the repository and this PR mainly adds functionality for vello tests. I'm more familiar with vello_hybrid; so going to ask if someone else can take a look and provide final LGTM. Maybe @tomcur who looked at linebender/parley#272
I played around with the commands and also changed some values in vello to break things. The xtask commands work nice!
| @@ -0,0 +1,113 @@ | |||
| // Copyright 2024 the Vello Authors | |||
There was a problem hiding this comment.
Nit: year is 2025
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Looks really good. A few details I'd like to see changed, but looking forward to seeing this land
xtask/Cargo.toml
Outdated
| [dependencies] | ||
| kompari = { git = "https://github.com/linebender/kompari.git", rev = "4b851413e1b17307064aa48c50e59d7e29656543" } | ||
| kompari-tasks = { git = "https://github.com/linebender/kompari.git", rev = "4b851413e1b17307064aa48c50e59d7e29656543" } | ||
| clap = { version = "4", features = ["derive"] } |
There was a problem hiding this comment.
| clap = { version = "4", features = ["derive"] } | |
| clap = { workspace = true, features = ["derive"] } |
xtask/src/main.rs
Outdated
| #[command(version, about, long_about = None)] | ||
| /// Top-level command line parser for xtask | ||
| pub struct Cli { | ||
| /// Command |
There was a problem hiding this comment.
These should have more descriptive comments, or nothing at all.
| /// Command | |
| /// The possible commands in this CLI. | |
| /// This enables (future) global flags to be added to this struct |
xtask/src/main.rs
Outdated
| Command::new(&cargo) | ||
| .arg("test") |
There was a problem hiding this comment.
We probably should make this use nextest. That makes this testing portable (in particular, openGL implementations run into segfaults if too many instances are opened at once).
Ideally, we'd have good instructions about install nextest (we shouldn't do it ourselves) printed, but cargo does that itself:
error: no such command: `nonexistant`
help: view all installed commands with `cargo --list`
help: find a package to install `nonexistant` with `cargo search cargo-nonexistant`
3 participants
This PR adds xtask similar to linebender/parley#272 for Parley.
Because Vello has two types of tests: snapshots and comparisons (cpu vs. gpu),
xtask is divided to two subcommands:
The first one offers all Kompari's functionality (report, review, size-check, dead-snapshots detection).
The second one offers only report as others do not make sense.
Btw1: Current size check output:
Btw2: Current dead snapshots report (This may be broken as some tests are skipped on my machine)