Skip to content

Comparing pixels instead of bytes with GoldenFileComparator#38473

Merged
Piinks merged 21 commits intoflutter:masterfrom
Piinks:localComparator
Aug 19, 2019
Merged

Comparing pixels instead of bytes with GoldenFileComparator#38473
Piinks merged 21 commits intoflutter:masterfrom
Piinks:localComparator

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Aug 13, 2019 •
edited
Loading

Description

This PR changes the implementation of the GoldenFileComparators method of comparison. While in the past, comparisons have been done bit-by-bit, this replaces that construct with pixel-by-pixel comparison.

See follow-up section below for further implications of this change in package:flutter/flutter.

This also adds the features of more feedback in describing why a comparison fails as well as providing output of images to illustrate the difference. When a pixel comparison fails due to a detected difference, the following files will be output in a failure directory located where the master golden image was specified for the given test.

Output files will follow the format:

testname_ImageType.png

Title Image
testName_masterImage.png golden-file_masterImage
testName_testImage.png golden-file_testImage
testName_isolatedDiff.png golden-file_isolatedDiff
testName_maskedDiff.png golden-file_maskedDiff

Other Options to Consider for this Change

  • An allowable pixel difference for each channel that is being compared

  • A % of pixel difference overall that can be dismissed as trivial

Follow-up

Following this PR, the improved comparator will be implemented further in the package:flutter/flutter golden file tests to incorporate Skia Gold baselines established with #36103. The follow-up to this change will enable local, pre-submit and post-submit testing for package:flutter/flutter across all platforms. 🎉

Related Issues

Fixes #17260
Fixes #17258
also maybe #17259, #30036

Tests

Updated/Added:

  • LocalFileComparator compare
  • LocalFileComparator compare succeeds when golden file is in same folder as test.
  • LocalFileComparator compare succeeds when golden file is in subfolder of test.
  • LocalFileComparator compare succeeds when comparator instantiated with uri that represents file in same folder and golden file is in same folder as test
  • LocalFileComparator compare succeeds when comparator instantiated with uri that represents file in same folder and golden file is in subfolder of test
  • LocalFileComparator compare fails when golden file does not exist
  • LocalFileComparator compare fails when images are not the same size
  • LocalFileComparator compare fails when pixels do not match
  • LocalFileComparator compare fails when golden bytes are empty
  • LocalFileComparator update updates existing file

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@Piinks Piinks added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Aug 13, 2019
@Piinks Piinks changed the title WIP Comparing Pixels instead of bytes with GoldenFileComparator WIP Comparing pixels instead of bytes with GoldenFileComparator Aug 13, 2019
@Piinks Piinks changed the title WIP Comparing pixels instead of bytes with GoldenFileComparator Comparing pixels instead of bytes with GoldenFileComparator Aug 13, 2019
@Piinks Piinks added a: images Loading, displaying, rendering images a: quality A truly polished experience customer: crowd Affects or could affect many people, though not necessarily a specific customer. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Aug 13, 2019
gspencergoog
gspencergoog approved these changes Aug 16, 2019
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few little formatting nits.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@Piinks
Copy link
Contributor Author

Piinks commented Aug 16, 2019

Thanks @gspencergoog ! I will fix the formatting.
Do you think this is a breaking change? It's running a comparison to the goldens in flutter/goldens here in pre-submit checks, and it's passing, so I am inclined to think it is ok.

@gspencergoog
Copy link
Contributor

gspencergoog commented Aug 16, 2019

Yes, I think it's probably OK too. I suspect that something that was bit-identical before is also pixel-identical now.

gspencergoog
gspencergoog reviewed Aug 16, 2019
View reviewed changes
@Piinks Piinks merged commit 6ff212b into flutter:master Aug 19, 2019
This was referenced Aug 22, 2019
Merged
Merged
@jonahwilliams jonahwilliams mentioned this pull request Aug 24, 2019
Closed
goderbauer
goderbauer reviewed Aug 26, 2019
View reviewed changes
packages/flutter_test/lib/src/goldens.dart
import 'dart:io';
import 'dart:math' as math;
import 'dart:typed_data';
import 'dart:ui';
Copy link
Member

@goderbauer goderbauer Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per style guide, this should have been imported with "show" or "as ui": https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#import-conventions

Copy link
Member

@goderbauer goderbauer Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It is not clear to me how dart currently decides if you want the Image class from dart:ui or from package:image. I am surprised that this works as is...)

@Piinks Piinks mentioned this pull request Sep 17, 2019
Closed
@apaatsio apaatsio mentioned this pull request Oct 7, 2019
Closed
@Piinks Piinks added the c: new feature Nothing broken; request for a new capability label Nov 4, 2019
@Piinks Piinks deleted the localComparator branch November 22, 2019 00:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer left review comments

@gspencergoog gspencergoog gspencergoog approved these changes

Assignees

No one assigned

Labels

a: images Loading, displaying, rendering images a: quality A truly polished experience a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. c: new feature Nothing broken; request for a new capability customer: crowd Affects or could affect many people, though not necessarily a specific customer. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Visualize golden image file failures in test output Implement better logic in golden file comparators

4 participants

@Piinks @gspencergoog @goderbauer @googlebot