Feature/precision parameter

[alexey1312]

This PR adds the precision and perceptualPrecision parameters to the different snapshot functions.

• Upgraded library swift-snapshot-testing version to 1.11.0.
• Adds parameters only for swift-snapshot-testing

https://github.com/cashapp/AccessibilitySnapshot/issues/63 https://github.com/cashapp/AccessibilitySnapshot/pull/64

[yakovshkolnikovveriff]

Hello! Which steps needs to be done to merge that pr? I'm really waiting for it 😄

[alexey1312]

@NickEntin pls review 🙏

[NickEntin]

Hey! Apologies, I was out on vacation and fell way behind on my GitHub queue.

Would perceptual precision alone provide sufficient control for you? See the discussion in #63. From our testing we've found precision-based control can make it really easy for regressions to slip through, especially in larger snapshot images (which accessibility snapshots tend to be since they include the legend). I'm worried about making it very easy to accidentally undermine the confidence gained from these snapshots. Perceptual precision seems a bit safer than the standard precision parameter, though I'm doing some testing right now to confirm that.

[alexey1312]

Welcome back, returning from vacation) In this PR, I am supporting both parameters, and it seems appropriate for users to decide which parameter they want to use)

[bonkey]

I've got the same one for FBSnapshotTestCase here #123.

And I agree with @alexey1312 that its should be user's decision.

It's also crucial to introduce compatibility between snapshots recorded and checked on different architectures (Intel/M1) or environments (older/newer macOS).

[NickEntin]

it seems appropriate for users to decide which parameter they want to use

I'm generally in favor of giving consumers control, but I think there's an important caveat: it needs to be very obvious when what you're doing might result in unexpected/undesirable behavior. You can see a good example of this in Swift's API with the "unsafe" terminology around memory (UnsafePointer, withUnsafeBytes(of:_:), etc.). Any time you see "unsafe" in your code, it's a sign to be extra careful. Snapshot precision/tolerance falls in the same category in my mind - it's easy to write code that seems like it should be very safe, but in practice isn't. For example, 99% precision intuitively sounds like it will be very accurate, but in practice we've found that 99% precision can let a lot of regressions through, especially as the size of the snapshots increases. This can lead to your test suite giving you a false sense of confidence.

crucial to introduce compatibility between snapshots recorded and checked on different architectures (Intel/M1) or environments (older/newer macOS)

Absolutely agreed that's a problem we need to solve. We're currently investigating some alternate approaches to comparing snapshots that are (hopefully) less prone to some of the issues we've seen with the existing precision/tolerance options, but don't have any ready to apply here quite yet.

I'm open to at least temporarily introducing precision/tolerance as a workaround, but I think it's important to do so in a way that (a) limits the potential impact where possible, which is why I was asking if perceptual precision is sufficient and (b) makes it very clear you're opening the door to non-trivial regressions slipping through. What would you think about adding a copy of the verification methods with something like "imprecise" or "inexact" in the name, e.g. .impreciseAccessibilityImage(...) and SnapshotImpreciseVerifyAccessibility(...)?

I'm also trying to gather some samples to better illustrate the problem (#124), but have been running into issues with our CI builds. 😞 Hoping to have some more concrete examples to show how regressions can slip through soon, so we can frame this discussion around real cases.

[bonkey]

What would you think about adding a copy of the verification methods with something like "imprecise" or "inexact" in the name, e.g. .impreciseAccessibilityImage(...) and SnapshotImpreciseVerifyAccessibility(...)?

@NickEntin Generally it sounds good, and my only concern is that's incompatible with API of FBSnapshotTestCase and SnapshotTesting, where those arguments are in the single function.

BTW. I see in #124 that you only use overallTolerance but not perPixelTolerance which is also useful.

[NickEntin]

BTW. I see in https://github.com/cashapp/AccessibilitySnapshot/pull/124 that you only use overallTolerance but not perPixelTolerance which is also useful.

Yep, totally agreed. Unfortunately we've found that perPixelTolerance isn't enough in many cases though (especially around things like transforms and shadows), and even very low overallTolerance values are enough to let through regressions.

my only concern is that's incompatible with API of FBSnapshotTestCase and SnapshotTesting, where those arguments are in the single function.

That's fair, but since we provide a separate API already, I think it's okay to have two sets of methods. I'm going to decline this PR in favor of #143, which introduces the "imprecise"-prefixed variants. Note that it's currently a draft due to the same dependency issues this PR faces, as I commented on above. Thanks for opening this one @alexey1312 , appreciate it moving forward this conversation.