Add ability to specify tolerance level for golden files by Piinks · Pull Request #76545 · flutter/flutter

Piinks · 2021-02-22T18:16:36Z

This adds the ability to specify a tolerance level for golden file comparison.
A good example of this use would be golden files that fail due to very small imperceptible changes, like from Skia rolls.
Here is one such case:

This image fails due to a realllly small diff.

Fixes #76337
Related to #71437

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

CaseyHillers

LGTM, thank you for the quick turn around!

goderbauer · 2021-02-22T19:55:42Z

+  double _tolerance = 0.0;
+
+  @override
+  void setPrecisionTolerance(double precisionTolerance) {


Could the base class "GoldenFileComparator" just have a getter/setter for this?

goderbauer · 2021-02-22T20:00:55Z

      throw 'must provide a Finder, Image, or Future<Image>';
    }

+    goldenFileComparator.setPrecisionTolerance(precisionTolerance);


Instead of modifying the global state of the goldenFileComparator (which could be a surprising side effect), could the tolerance just be passed in below to the compare method?

I thought of going that route, but it might be more breaking that way. Still evaluating. The comparator class (and therefore compare function) is extended/implemented/overridden by many customers.

Piinks · 2021-02-22T22:30:27Z

+  double get precisionTolerance => _precisionTolerance;
+  double _precisionTolerance = 0.0;
+  /// Sets an allowable percentage of difference between golden file images.
+  set precisionTolerance(double value) {


@goderbauer I made this getter/setter instead, it has ramifications for subclasses like the regular and web Trivial* comparators.. Not sure which is better.

Piinks · 2021-02-26T21:07:23Z

I am not convinced adding this is a good idea yet, especially after conversation in #76831
Will wait to see decision from that discussion before proceeding.

@CaseyHillers what if this was just reduced to exposing the calculated diff in ComparisonResult? Then users could extend LocalFileComparator and just implement compare to allow pass through for whatever tolerance level the user would like?
In that case, you would not have to pass a tolerance level for each individual test. That feels a bit annoying and clunky.

CaseyHillers · 2021-02-26T22:11:25Z

@CaseyHillers what if this was just reduced to exposing the calculated diff in ComparisonResult? Then users could extend LocalFileComparator and just implement compare to allow pass through for whatever tolerance level the user would like?
In that case, you would not have to pass a tolerance level for each individual test. That feels a bit annoying and clunky.

Either SGTM. In flutter/cocoon, the tests use a cocoon wrapper util for golden tests, so it's a one line change.

As another data point, not surfacing this as a parameter should have an example of constructing a comparator that can be used for enabling a diff tolerance.

Piinks · 2021-03-01T22:30:54Z

I am going to close this and open a new PR to expose the ComparisonResult.diff, it'll be easier to start fresh than revert here. :)

lukepighetti · 2021-03-09T13:11:14Z

@Piinks I didn't see this PR until I was nearly done with mine. I pushed it up here. #77703 If it's of value to you feel free to cannibalize.

lukepighetti · 2021-03-15T00:14:00Z

@Piinks I tried #77014 and I have some feedback. I put it in here because the discussion in here is most pertinent to my feedback.

I am not convinced adding this is a good idea yet, especially after conversation in #76831
Will wait to see decision from that discussion before proceeding.

@CaseyHillers what if this was just reduced to exposing the calculated diff in ComparisonResult? Then users could extend LocalFileComparator and just implement compare to allow pass through for whatever tolerance level the user would like?

I'm trying this out now and I'm not sure how to proceed. There are private methods we don't have access to. Once you start unraveling that you're better off doing a copy-paste from what I can tell.

From #76545 (comment)

In that case, you would not have to pass a tolerance level for each individual test. That feels a bit annoying and clunky.

I know the ship has sailed, but IMHO that's a non-issue because you can make a curried version of matchesGoldenFile with the tolerance baked in and use it everywhere. I also don't necessarily want to have a global tolerance. My strategy would be to employ this on problematic tests, picking a tolerance that matches what we typically see when they flake. An incremental tolerance that is easy to implement was my strong preference when I looked at addressing this.

Piinks · 2021-03-15T16:36:20Z

Hey @lukepighetti! Thanks for the feedback. While implementing a custom comparator for flutter/cocoon (flutter/cocoon#1106), I too found this difficult. I just made some changes the other day to make this easier in #77825

Piinks added 3 commits February 19, 2021 19:19

Init

d1949c9

Add tests

a9e8ce7

Docs

172d2da

flutter-dashboard Bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Feb 22, 2021

google-cla Bot added the cla: yes label Feb 22, 2021

Piinks requested review from CaseyHillers and goderbauer February 22, 2021 18:16

CaseyHillers approved these changes Feb 22, 2021

View reviewed changes

Comment thread packages/flutter_test/lib/src/_matchers_io.dart

goderbauer reviewed Feb 22, 2021

View reviewed changes

++

4b085d6

Piinks commented Feb 22, 2021

View reviewed changes

Fix test

b9f8faf

Piinks closed this Mar 1, 2021

This was referenced Mar 9, 2021

Add diffTolerance parameter to matchesGolden #76337

Closed

allow golden tests to specify epsilon tolerance #77703

Closed

Conversation

Piinks commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

CaseyHillers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

Piinks Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Piinks Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

Piinks commented Feb 26, 2021

Uh oh!

CaseyHillers commented Feb 26, 2021

Uh oh!

Piinks commented Mar 1, 2021

Uh oh!

lukepighetti commented Mar 9, 2021

Uh oh!

lukepighetti commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Piinks commented Feb 22, 2021 •

edited

Loading

lukepighetti commented Mar 15, 2021 •

edited

Loading