Example with xtasks integration #5
Conversation
There was a problem hiding this comment.
Approving to unblock further progress.
I'd like us to put some careful thought into architecture here, to avoid needing to do costly reworks across the suite. I think there is the potential to move a lot of the complexity into Kompari itself, to make writing test suites quite straightforward.
But unfortunately, I don't have the bandwidth to help with that at the moment.
| @@ -0,0 +1,5 @@ | |||
| [workspace] | |||
There was a problem hiding this comment.
I think it would be worth considering including these in the main workspace, i.e not have its own workspace.
The workspace means that the CI doesn't check this code, which is definitely unfortunate. It also means that the lint set doesn't apply.
There was a problem hiding this comment.
I wanted to make it more independent so it easier to explore the example, but I will try to refactor it because make it a part of CI is more beneficial.
There was a problem hiding this comment.
I have tried to refactor this and the result is in the example-workspace branch (https://github.com/linebender/kompari/tree/example-workspace). I am afraid that in this version it is not clear what is part of the example and what is part of kompari. @xStrom Would it be possible (without much effort) to build the example in CI without having it in the top level workspace?
example/.cargo/config.toml
Outdated
| @@ -0,0 +1,2 @@ | |||
| [alias] | |||
| xtask-kompari = "run --package xtask_kompari --" | |||
There was a problem hiding this comment.
I think it would be a better UX if the name is just xtask. That would mean that in each repository, we get the same commands (cargo xtask test, etc.)
See e.g. https://github.com/gfx-rs/wgpu/blob/trunk/.cargo/config.toml. I think for now, we can probably ignore gfx-rs/wgpu#3844 (comment)
| xtask-kompari = "run --package xtask_kompari --" | |
| xtask = "run --package xtask_kompari --" |
example/README.md
Outdated
| // Change the value ----\ | ||
| // here to e.g. 25 | | ||
| // v |
There was a problem hiding this comment.
I'd suggest moving this entire code block to inside the actual code. It took me a couple of readings to realise you didn't mean edit in the README, but in the same code inside the folder.
There was a problem hiding this comment.
When I say the code block, I mean the comments.
And I agree that having a practical demo is really good, and so if implementing this, we would point towards the moved comment in the README.
example/demolib/src/lib.rs
Outdated
| /// Directory where current tests creates images | ||
| fn current_dir() -> PathBuf { | ||
| Path::new(env!("CARGO_MANIFEST_DIR")) | ||
| .parent() | ||
| .unwrap() | ||
| .join("tests") | ||
| .join("current") | ||
| } | ||
|
|
||
| /// Directory with blessed snapshots | ||
| fn snapshot_dir() -> PathBuf { | ||
| Path::new(env!("CARGO_MANIFEST_DIR")) | ||
| .parent() | ||
| .unwrap() | ||
| .join("tests") | ||
| .join("snapshots") | ||
| } | ||
|
|
||
| fn is_generate_all_mode() -> bool { | ||
| std::env::var("DEMOLIB_TEST") | ||
| .map(|x| x.to_ascii_lowercase() == "generate-all") | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| /// Check an image against snapshot | ||
| fn check_snapshot(image: RgbImage, image_name: &str) { | ||
| let snapshot_dir = snapshot_dir(); | ||
| let snapshot = image::ImageReader::open(snapshot_dir.join(image_name)) | ||
| .map_err(|e| e.to_string()) | ||
| .and_then(|x| x.decode().map_err(|e| e.to_string())) | ||
| .map(|x| x.to_rgb8()); | ||
| if let Ok(snapshot) = snapshot { | ||
| if snapshot != image { | ||
| image.save(current_dir().join(image_name)).unwrap(); | ||
| panic!("Snapshot is different; run 'cargo xtask-test report' for report") | ||
| } | ||
| } else { | ||
| println!("{}", current_dir().join(image_name).display()); | ||
| image.save(current_dir().join(image_name)).unwrap(); | ||
| snapshot.unwrap(); | ||
| } | ||
| if is_generate_all_mode() { | ||
| image.save(current_dir().join(image_name)).unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd consider putting this infrastructure in its own file. It's important, but quite noisy for someone just trying out the infra (i.e. making the edit you suggest)
|
I am merging this to unblock my other work, but I am still not sure how to deal with the global workspace. |
No description provided.