Skip to content

Fix a misuse of matchesGoldenFile() in the physical_model_test.#30141

Merged
2 commits merged intomasterfrom
unknown repository
Mar 29, 2019
Merged

Fix a misuse of matchesGoldenFile() in the physical_model_test.#30141
2 commits merged intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Mar 28, 2019

Description

When running the flutter tests with a clean tree on my Mac, I hit the following report:

$ flutter test test/widgets/physical_model_test.dart 
00:03 +1: PhysicalModel - clips when overflows and elevation is 0                                                                                                                                                                                                                                                                                                                                                                                              
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure object was thrown running a test (but after the test had completed):
  Expected: one widget whose rasterized image matches golden image "physical_model_overflow.png"
  Actual: ?:<zero widgets with key [<'test'>] (ignoring offstage widgets)>
   Which: does not match

When the exception was thrown, this was the stack:
#0      fail (package:test_api/src/frontend/expect.dart:152:30)
#1      _expect.<anonymous closure> (package:test_api/src/frontend/expect.dart:127:9)
#24     _MatchesGoldenFile.matchAsync.<anonymous closure> (package:flutter_test/src/matchers.dart)
<asynchronous suspension>
#25     AutomatedTestWidgetsFlutterBinding.runAsync.<anonymous closure> (package:flutter_test/src/binding.dart:778:22)
#28     AutomatedTestWidgetsFlutterBinding.runAsync (package:flutter_test/src/binding.dart:776:26)
#29     _MatchesGoldenFile.matchAsync (package:flutter_test/src/matchers.dart:1667:20)
#31     _MatchesGoldenFile.matchAsync (package:flutter_test/src/matchers.dart:1649:28)
#32     _expect (package:test_api/src/frontend/expect.dart:116:26)
#33     expect (package:test_api/src/frontend/expect.dart:59:3)
#34     expect (package:flutter_test/src/widget_tester.dart:196:3)
#35     main.<anonymous closure> (file:///Users/darrenaustin/src/flutter/flutter/packages/flutter/test/widgets/physical_model_test.dart:66:5)
#49     AutomatedTestWidgetsFlutterBinding.runTest.<anonymous closure> (package:flutter_test/src/binding.dart:935:17)
#51     AutomatedTestWidgetsFlutterBinding.runTest.<anonymous closure> (package:flutter_test/src/binding.dart:923:35)
(elided 54 frames from class _FakeAsync, package dart:async, and package stack_trace)
════════════════════════════════════════════════════════════════════════════════════════════════════
00:04 +2: All tests passed!

Which is weird as it reported that All tests passed! even though there was an exception thrown. Looking into the issue, the exception appears to be caused by a golden file test that was not being called inside await expectLater. So when the exception failed on my machine, it was after the test completed so didn't count as a test failure.

In addition, it appears this golden image doesn't match the rendering for it on the Mac, so I added a skip for this check on anything but Linux.

I manually checked all the other usages of matchesGoldenFile and didn't see any other instances where it was used incorrectly. However, would it might make sense to provide a expectGoldenMatch(finder, goldenFileName) that handles the synchronization (and optionally skips non-Linux platforms) to avoid this kind of issue in the future?

Related Issues

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.
  • My PR includes tests for all changed/updated/fixed behaviors (See Test Coverage).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • 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?

@ghost ghost requested review from HansMuller and Hixie March 28, 2019 22:16
HansMuller
HansMuller approved these changes Mar 28, 2019
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit 19901f6 into flutter:master Mar 29, 2019
@ghost ghost deleted the physical_model_test_golden_bug branch March 29, 2019 01:08
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hixie Hixie Awaiting requested review from Hixie

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HansMuller @googlebot @darrenaustin