tests(sparse_strips): add JSON diff reports for sparse test snapshot failures #1328
Conversation
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
This looks interesting; having exactly what RGBA colour things worked out to is good. I think it could land as-is, but I'd like to see a few things changed. If you do change the formatting, please re-request my review.
One small question; I notice that your example data there should be 20 entries, but it only has one. Am I correct that the rest were elided for space?
Cargo.toml
Outdated
| serde = { version = "1.0", default-features = false } | ||
| serde_json = "1.0" |
There was a problem hiding this comment.
These version numbers should be fully specified. This is a blocker.
| let json_data = serde_json::to_string_pretty(&report).unwrap(); | ||
| std::fs::write(&json_path, json_data).unwrap(); | ||
|
|
||
| panic!("test didnt match reference image"); |
There was a problem hiding this comment.
Can we provide the path to the diff report in this error message? This is not a blocker.
I keep meaning to come back through and massively improve the UX of these, but that's not for this PR...
There was a problem hiding this comment.
That’s really good advice, thank you!
I keep meaning to come back through and massively improve the UX of these, but that's not for this PR...
Honestly, I’ve already started on this and even have a branch with changes to the testing framework, including some API updates. I just haven’t had enough time to get it ready for review yet. If you’re keen, I’d be happy to share it.
Fixed.
There was a problem hiding this comment.
Making the UX of this kind of testing much less opaque is really helpful for contributors. I put a lot of care into that in Vello Classic's tests, which unfortunately didn't get carried into the sparse strips work.
When you have a PR, I can try and prioritise it.
| /// The RGBA values from the reference image. | ||
| pub reference: [u8; 4], | ||
| /// The RGBA values from the actual image. | ||
| pub actual: [u8; 4], |
There was a problem hiding this comment.
Have you considered serialising this as either #rrggbbaa or rgba(rrr, ggg, bbb, aaa) (i.e. as a string); I believe if we do that, vscode will show a colour picker as a preview. Plus, the formatting of the lines will be much shorter.
I think we can either do that with a custom serialise function (which would use https://docs.rs/serde/latest/serde/trait.Serializer.html#method.collect_str), or by storing the field as a String (which of course would be much more allocation intensive, and so wouldn't be my preferred solution).
There was a problem hiding this comment.
Good idea! I had been thinking about custom serialization from a slightly different angle, mainly focusing on putting the color data on single lines because it felt easier to read and compare. I hadn’t considered using #rrggbbaa, but that could definitely be worth adding alongside the current format. When I tried implementing it, things got a bit heavy, but I’m happy for this to be explored further, either by me or someone else as a follow-up later.
grebmeg
force-pushed
the
gemberg/tests-diff-log-data
branch
from
52be151 to
d9ec9a2
Compare
grebmeg
left a comment
grebmeg
left a comment
There was a problem hiding this comment.
One small question; I notice that your example data there should be 20 entries, but it only has one. Am I correct that the rest were elided for space?
Yes, sorry for the confusion. I just omitted the rest from the snippet.
Cargo.toml
Outdated
| serde = { version = "1.0", default-features = false } | ||
| serde_json = "1.0" |
| let json_data = serde_json::to_string_pretty(&report).unwrap(); | ||
| std::fs::write(&json_path, json_data).unwrap(); | ||
|
|
||
| panic!("test didnt match reference image"); |
There was a problem hiding this comment.
That’s really good advice, thank you!
I keep meaning to come back through and massively improve the UX of these, but that's not for this PR...
Honestly, I’ve already started on this and even have a branch with changes to the testing framework, including some API updates. I just haven’t had enough time to get it ready for review yet. If you’re keen, I’d be happy to share it.
Fixed.
| /// The RGBA values from the reference image. | ||
| pub reference: [u8; 4], | ||
| /// The RGBA values from the actual image. | ||
| pub actual: [u8; 4], |
There was a problem hiding this comment.
Good idea! I had been thinking about custom serialization from a slightly different angle, mainly focusing on putting the color data on single lines because it felt easier to read and compare. I hadn’t considered using #rrggbbaa, but that could definitely be worth adding alongside the current format. When I tried implementing it, things got a bit heavy, but I’m happy for this to be explored further, either by me or someone else as a follow-up later.
grebmeg
changed the base branch from
gemberg/perf/image-rendering-improvements
to
main
Follow-up from linebender#1328 The new formatting looks like: ```json ... { "x": 1, "y": 18, "reference": "#ece5f2", "actual": "#bfffbf", "difference": [ -45, 26, -51, 0 ] }, { "x": 2, "y": 1, "reference": "#d9cce5", "actual": "#7fff7f", "difference": [ -90, 51, -102, 0 ] }, ... ```
Follow-up from #1328 The new formatting looks like: ```json ... { "x": 1, "y": 18, "reference": "#ece5f2", "actual": "#bfffbf", "difference": [ -45, 26, -51, 0 ] }, { "x": 2, "y": 1, "reference": "#d9cce5", "actual": "#7fff7f", "difference": [ -90, 51, -102, 0 ] }, ... ``` I've also tweaked the in-repo settings of vscode to show the colour picker in these diff files: <img width="348" height="241" alt="image" src="https://github.com/user-attachments/assets/2ed7cb9f-f785-4e70-ac63-ae2fc2e2d950" />
I find this report very useful! It looks like this:
{ "pixel_count": 20, "max_difference": [ 1, 3, 1, 0 ], "pixels": [ { "x": 14, "y": 10, "reference": [ 94, 139, 198, 255 ], "actual": [ 95, 142, 199, 255 ], "difference": [ 1, 3, 1, 0 ] }, ] }