The value passed into the failureThreshold option is compared against a ratio, not a percent, which is confusing since the failureThresholdType is percent.

[theblang]

See this conditional block where the comparison is done. It is really comparing the ratio, not the percent, against the threshold option. That is confusing since the options documentation says to specify percent for the type.

There are two options to fix this: change the comparison to actually be a percentage or change the documentation to specify ratio as the type. The former seems more straightforward, while the latter could be backwards compatible (continue to accept percent but change the documentation to say ratio and check for either when deciding which comparison to do).

[anescobar1991]

I think we should change the comparison to actually use a percentage. This might be controversial but in my mind this is a bug and so the fix would not require a major version bump for the breaking change.

@theblang @10xLaCroixDrinker what do you guys think?

[PixnBits]

I'm probably missing something obvious, but to clarify what's the difference between the ratio of pixels and the percentage of pixels? const diffRatio = diffPixelCount / totalPixels; Seems like the const name could be diffPercent and still be accurate?

[10xLaCroixDrinker]

@anescobar1991 This would certainly break every user that is using this failureThresholdType. I strongly disagree with regard to version bumping; it should be a major version bump to implement the change you are suggesting.

@PixnBits a ratio being 0-1 and a percentage 0-100

[PixnBits]

Ah, cheers. Percentage vales are displayed as xx.x% but stored in machines or expressed in Math as 0.xxx, hence my confusion. I'm guessing this issue is requesting the value to be the 'xx%' form instead of '0.xx' currently in use? I wonder if looking for the trailing % would be a way jest-image-snapshot could use to differentiate the two forms when making a comparison?

[theblang]

@PixnBits Yeah, so if you see this code you'll notice that it does a calculation of "diff pixels / total pixels". So if you had 500 pixels that were different in an image of 1000 total pixels then you'd get 500 / 1000, or 0.5, as the ratio; then multiplied by 100 you'd get 50%. Since the failureThresholdType mentions "percent" in the docs for the failureThreshold value, this came across as a bug to me.

@anescobar1991 @10xLaCroixDrinker My opinion is a bit torn about whether this is a bugfix or a breaking change. I lean more towards bugfix since the docs clearly state "percent" for the failureThreshold value. I imagine people could be seeing successes and not even realize they are giving two orders of magnitude more threshold than they mean. If anyone did notice, they had to knowingly massage the value to account for the error.

[PixnBits]

I guess what I'm getting at is that percentage is a presentation of a ratio, 50% === 0.5 (the multiplication/shifting the decimal over 2 points is just for the presentation)

ex: https://stackoverflow.com/a/1602345

[10xLaCroixDrinker]

Good point @PixnBits. Perhaps what we need is to just clarify this in the README

[theblang]

@PixnBits @10xLaCroixDrinker So just making changes to specify ratio in the documentation was my latter thought in the original post. However, my personal opinion is to change the comparison value in code and leave the documentation as is. You mention that percent is only presentational, which is true. But the reason that that presentational concept exists is because it is easier to think about. The big difference here with that SO answer is that in the SO answer they are storing numbers in a DB. Here you are "presenting" an API to a user. I personally think it will be weird to say "failureThresholdType can be pixel or ratio".

[PixnBits]

totally agree, and ambiguity on what something like '0.5' means (50%? 0.5%?) is a bad experience 👍

thoughts on

I wonder if looking for [a] trailing % would be a way…to differentiate the two forms

from issuecomment-415063294?

[10xLaCroixDrinker]

@PixnBits sounds like a great solution to me

[anescobar1991]

I think that's sensible. @theblang do you want to update your PR to reflect @PixnBits's thoughts?

[theblang]

@anescobar1991 @PixnBits @10xLaCroixDrinker Sounds good. Sure, I can tweak #100 . One question, do we want to remove failureThresholdType? Maybe leave it in for backwards compatibility but tweak the docs? It seems pointless now, and even confusing, if we are deducing the type based on the failureThreshold value.

[10xLaCroixDrinker]

No, we still have the pixel type. We've just added a new way to input the percentage type.

[theblang]

@10xLaCroixDrinker So my thought was that maybe all three types could now be deduced from the failureThreshold value instead of needing the additional failureThresholdType config. There'd be either a number with a percentage sign for a percent, a number less than 1 for a ratio, or a number greater than or equal to 1 for a pixel count. I suppose though that a value of 1 would be ambiguous, as it could be either a ratio meaning 100 percent or literally just 1 pixel.

So just to double check before I tweak my PR. We will still keep the two types as they are, pixel and percent, but just allow a value of #% to be passed in?

[anescobar1991]

Yes that’s right @theblang

[spinus]

is this relevant anymore?

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity.

[merlinstardust]

Has this update been made? I too would like to have the option to use 50% instead of 0.5