Allow new methods to be added to ui.Image for tests by dnfield · Pull Request #65876 · flutter/flutter

dnfield · 2020-09-15T18:27:08Z

flutter/engine#21057 is going to add some new methods to ui.Image.

Our tests shouldn't break for that.

jonahwilliams

An alternative would be to add the new methods with an override and an ignore for the override warning.

At any rate, it would be nice to have a bug tracking the removal

dnfield · 2020-09-15T18:31:08Z

Do we really want to remove these? If we remove them, we cause extra pain/work every time we add a method or member to an interface that is explicitly documented as something that you should not extend or implement :\ I understand why we implement it here (for testing purposes only), but even then using the implemented case in a test can cause a unintended test failure if the fake image actually gets to the C++ side of a test.

dnfield · 2020-09-15T18:32:38Z

I actually think a better alternative would be to add something to package:flutter_test that produces an image for you with the desired dimensions (perhaps with the desired color as well?).

It should be fairly trivial to use something like decodeImageFromPixels for that. WDYT?

jonahwilliams

I think it depends on the various use cases here. We have the kTransparentImage pattern too but that might not be general enough? If there isn't actually any mocking going on maybe

goderbauer · 2020-09-15T22:52:28Z

I didn't check all of them, but the once I looked at looked all the same. Should we maybe only have one TestImage implementation under flutter/test somewhere and reuse that across these tests?

Adding something to flutter_test (or maybe even have that as a private method within flutter/test) to generate a test image also sounds reasonable to me.

dnfield · 2020-09-17T23:28:45Z

Updated this to completely do away with the TestImage fakes, and just use real images.

jonahwilliams · 2020-09-17T23:45:10Z

packages/flutter_test/lib/src/image.dart

+
+final Map<int, ui.Image> _cache = <int, ui.Image>{};
+
+/// Asynchronously creates an arbitrarily sized test image.


Maybe instead of "arbitrarily sized test image" -> "arbitrarily sized image for testing", since it is just a regular image.

I dunno if async is necessarily important here, since that is implied by the return type.

(Cleaned up a little of the rest of the doc too, and saved a file I had fixed but forogtten to save :)

jonahwilliams · 2020-09-18T00:20:07Z

packages/flutter_test/lib/src/image.dart

+
+/// Creates an arbitrarily sized image for testing.
+///
+/// If the [cache] parameter is set to true, the image will be cached. This


... for the rest of the test suite (maybe?)

dnfield · 2020-09-18T19:05:17Z

(fixed up the analysis error for real this time)

dnfield · 2020-09-21T04:34:19Z

Added a work around for web (see #49244)

goderbauer · 2020-09-21T16:44:22Z

packages/flutter/test/painting/binding_test.dart

  int get width => 10;
+
+  @override
+  dynamic noSuchMethod(Invocation invocation) {


Why can the use of this FakeImage not be replaced with the new test image stuff?

goderbauer · 2020-09-21T16:50:32Z

packages/flutter/test/rendering/image_test.dart

  void dispose() { }
+
+  @override
+  dynamic noSuchMethod(Invocation invocation) {


Same here and below.

goderbauer · 2020-09-21T16:52:06Z

packages/flutter/test/widgets/image_test.dart

  @override
  String toString() => '[$width\u00D7$height]';
+
+  @override


... and here.

Refactored all of these, thanks for catching.

goderbauer · 2020-09-21T16:53:51Z

packages/flutter_test/lib/src/image.dart

+///
+/// This method requires real async work, and will not work properly in the
+/// [FakeAsync] zones set up by [testWidgets]. Typically, it should be invoked
+/// as a setup step before [testWidgets] are run. If needed, it can be invoked


should this link to [setUpAll] / [setUp] as an appropriate place to call this?

Added links.

goderbauer · 2020-09-21T16:54:57Z

packages/flutter_test/lib/src/image.dart

+  }
+
+  if (kIsWeb) {
+    return await _webCreateTestImage(


These are not added to the cache?

Ah, I see, it happens within the method. Maybe for symmetry, pull the caching code out and place it here?

Refactored this a bit to clean it up.

goderbauer · 2020-09-21T16:56:48Z

packages/flutter_test/lib/src/image.dart

+/// Creates an arbitrarily sized image for testing.
+///
+/// If the [cache] parameter is set to true, the image will be cached for the
+/// rest of this suite. This should be avoided for images that are used only


I was slightly confused by this telling me to avoid it and the fact that it defaults to true. Maybe add a sentence describing why you usually want caching?

Why do we need the caching at all? Does re-creating the image really cost that much in terms of testing performance?

My thought is that we're going from what is a very fast operation (constructing a fake implementation) to what is now a measurably slower and more memory intensive operation, particularly if the image tested is large.

Most of our test cases use a 10x10 image, completely arbitrarily. Some of them use as big as 200x300. If our CI machines are running slowly, caching that 200x300 could save us something on the order of seconds.

Updated the docs here to explain that.

goderbauer · 2020-09-21T16:58:38Z

Looks like its still unhappy on the web?

dnfield · 2020-09-21T17:13:56Z

Web is still unhappy because Image.toString works differently on web - on VM it does [w x h], on web it isn't overridden.

goderbauer

LGTM

goderbauer · 2020-09-21T17:49:32Z

packages/flutter_test/lib/src/image.dart

+  bool cache = true,
+}) => TestAsyncUtils.guard(() async {
+  assert(width != null && width > 0);
+  assert(height != null && height > 0);


nit: probably also assert(cache != null);

goderbauer · 2020-09-21T17:50:03Z

packages/flutter_test/lib/src/image.dart

+
+  final int cacheKey = hashValues(width, height);
+  if (cache && _cache.containsKey(cacheKey)) {
+    return _cache[hashValues(width,height)];


nit: space after ,

Actually, just use cacheKey here instead of recomputing it?

Whups, done.

fluttergithubbot · 2020-09-21T18:57:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux web_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

jonahwilliams

LGTM

dnfield · 2020-09-22T00:04:44Z

I think.. I think I got it this time guys. I think it's going to work. I even ran it locally and everything.

dnfield · 2020-09-22T18:55:23Z

Tree is green, landing this before it's too late.

Allow new methods to be added to ui.Image for tests

5e72fe5

dnfield requested review from goderbauer and jonahwilliams September 15, 2020 18:27

flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Sep 15, 2020

googlebot added the cla: yes label Sep 15, 2020

jonahwilliams reviewed Sep 15, 2020

View reviewed changes

dnfield added 3 commits September 16, 2020 08:58

Merge remote-tracking branch 'upstream/master' into image_impl

fcf1c04

Refactor

974c4eb

merge

5d342e6

dnfield mentioned this pull request Sep 17, 2020

Create an ImageHandle wrapper flutter/engine#21057

Merged

1 task

jonahwilliams reviewed Sep 17, 2020

View reviewed changes

review

eb25e70

jonahwilliams reviewed Sep 18, 2020

View reviewed changes

dnfield added 2 commits September 17, 2020 18:00

review and analyzer nits

a4b84e0

save before commiting!

92facd5

workaround for web

441a1e8

dnfield force-pushed the image_impl branch from f97141b to 441a1e8 Compare September 21, 2020 15:20

goderbauer reviewed Sep 21, 2020

View reviewed changes

goderbauer review and web fix

984ea73

goderbauer approved these changes Sep 21, 2020

View reviewed changes

..

9b5a715

dnfield added the waiting for tree to go green label Sep 21, 2020

fluttergithubbot removed the waiting for tree to go green label Sep 21, 2020

one more

fe55b1b

dnfield force-pushed the image_impl branch from ef3a8bc to fe55b1b Compare September 21, 2020 19:35

and more..

9ef3eed

jonahwilliams approved these changes Sep 21, 2020

View reviewed changes

dnfield added 2 commits September 21, 2020 16:29

Merge remote-tracking branch 'upstream/master' into image_impl

e788ef8

add the right skip

2375db9

dnfield added the waiting for tree to go green label Sep 22, 2020

dnfield merged commit 7eb8447 into flutter:master Sep 22, 2020

dnfield deleted the image_impl branch September 22, 2020 18:56

dnfield mentioned this pull request Sep 22, 2020

Remove last FakeImage implementation #66379

Merged


		final Map<int, ui.Image> _cache = <int, ui.Image>{};

		/// Asynchronously creates an arbitrarily sized test image.

Conversation

dnfield commented Sep 15, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 15, 2020

Uh oh!

dnfield commented Sep 15, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Sep 15, 2020

Uh oh!

dnfield commented Sep 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 18, 2020

Uh oh!

dnfield commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Sep 21, 2020

Uh oh!

dnfield commented Sep 21, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Sep 21, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 22, 2020

Uh oh!

dnfield commented Sep 22, 2020