test passed but approve replaces images about loki HOT 9 CLOSED

I'm not sure that's something I can do much about besides having different default thresholds for gm and looks-same or even setting default threshold to 0. In my current project we use gm and tolerance of 0.01. We started out with 0 but think there was some edge cases where it would then detect changes where there weren't any.

from loki.

This is an issue with gm not working properly I believe. The images are definitely different, but gm somehow does not pick up on that (or the code in loki does not catch the error and swallows it, I haven't looked into it yet) But I saw this issue as well when I first tried out gm and decided to stick with looks-same until this is fixed. I certainly like the output of gm better from the docs, so I'll probably poke at it as well to see if I can find the issue.

from loki.

There was a recent change in how loki handles gm errors: #62 maybe that's a regression

from loki.

For me, the issue was that the default tolerance just was not triggering an issue. It appears that the change was simply not big enough to trigger the tolerance failure.

I introduced a change that for me should have broken VRT with GM but it did not (but looks-alike did)

Current

Reference

This resulted in failures in looks-alike but for gm it only registered an {equality: 0.0009348162} difference where the threshold is 0.023.

from loki.

I feel like a threshold of 0 is a better default to avoid that case like the one I posted above where technically by percentage it is within threshold but clearly is a breaking change that would have been missed.

from loki.

Having a lower threshold helped a lot. Thanks.

But there is still the point of approving and having images changed where the diff didn't raise an error.
I think a quick and easy solution would be, on loki approve not to copy over all images from current, but just the ones with matching names to the ones in 'difference' (which are all images where an error was raised).
This would be the expected behaviour for this command (at least from my point of view)

from loki.

@jacksbox this is an interesting problem. With threshold > 0 you are basically saying that you approve of a shifting baseline within a certain threshold, so images that differ but below the threshold are considered "approved" and I think continue to move along and be updated as the new baseline. Maybe this is a configurable item. I personally (for the scope of the projects I work on) would expect a threshold of 0 (no unexpected changes) because our storybooks are testing static expectations, and we are testing on a consistent platform (chrome.docker). I would personally get way to nervous about > 0 threshold for my usecases because that is just a % of pixels changed check which for a larger image and smaller change (like adding a line of text in the above example) did not trigger a failure.

Should we always raise differences (even below threshold) but just mark them as differences within threshold or above) the only non-difference being a pixel perfect representation? It seems like this might bridge the gap, but require either a report that can convey that nuance in expectation where just files on a disk (we could do (threshold_differences/ and differences/ or something to store these changes on disk, but that seems clunky). I like the idea of never swallowing threshold differences, because people would probably still want to glance at > 0 changes to confirm they are expected and that their threshold is correct vs just silently swallowing them now.

from loki.

@nowells

Should we always raise differences (even below threshold) but just mark them as differences within threshold or above)
I think that would be a good idea so one gets not suddenly modified files in git commits.

About the threshold of 0 - yes, having a threshold = 0 makes more sense, especially given, that looks-same and gm have a totally different behaviour concerning the threshold

@nowells @oblador
Regarding the last paragraph: I think the different behaviour of gm and looks-same should be mentioned and explained somewhere in the docs.

from loki.

I'm closing this for now with the default tolerance set to 0. Reopen or file a new issue if you think this is not enough.

from loki.