WIP: Find all Image widgets and precache them before comparing #48

christian-muertz · 2020-05-25T09:07:01Z

Instead of calling matchesGoldenFile twice to prime all images, find all Image widgets and precache them so they are available instantly afterwards.

I am not sure if this is enough because their could probably be other assets that are not images which are not loaded this way.

codecov-commenter · 2020-05-25T09:09:31Z

Codecov Report

Merging #48 into master will decrease coverage by 3.71%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   99.41%   95.69%   -3.72%     
==========================================
  Files           7        7              
  Lines         172      186      +14     
==========================================
+ Hits          171      178       +7     
- Misses          1        8       +7

Impacted Files	Coverage Δ
packages/golden_toolkit/lib/src/configuration.dart	`83.33% <ø> (ø)`
...es/golden_toolkit/lib/src/multi_screen_golden.dart	`100.00% <ø> (ø)`
packages/golden_toolkit/lib/src/testing_tools.dart	`90.27% <56.25%> (-9.73%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80f9d57...cd40278. Read the comment docs.

christian-muertz · 2020-05-25T09:23:10Z

We should probably add sth like a AssetPrimeStrategy to make this change backwards compatible.

coreysprague · 2020-05-25T17:36:50Z

@christian-muertz thanks for submitting this!

I suspect we will want to allow this be configurable and extensible. I know in our projects we have some other asset related widgets that we may need to await as well. We could consider making this opt-in behavior for backwards compatibility for now. I think the global "config" concept I referenced in your other PR would be applicable for allowing this control without breaking existing consumers.

That being said, I'm really happy you started on this. It's been on my list for awhile.

christian-muertz · 2020-05-25T18:00:26Z

I know in our projects we have some other asset related widgets that we may need to await as well

Can you make an example?

coreysprague · 2020-05-27T04:55:36Z

@christian-muertz -- I believe there are some examples of images being used outside of Image widgets. For example, as part of BoxDecorations.

That being said, I think the way we should approach this is to start with this being an opt-in change. We can plan to make it a breaking change in a future release by deprecating the legacy implementation. That way we can have a few releases to solicit feedback and identify if there are other cases that this package should support out of the box.

I think it might make sense to implement this opt-in with the configuration mechanism I introduced in the linked PR. Would you be willing to make that change?

christian-muertz · 2020-05-27T19:12:30Z

@coreysprague please let me know what you think about this implementation. Is a PrimeAssets type the right way to go? Its a bit ugly because it takes in so many arguments, but otherwise we cannot call matchesGoldenFile without that info.

coreysprague · 2020-05-27T21:33:47Z

packages/golden_toolkit/lib/src/testing_tools.dart

+/// See also:
+/// * [GoldenToolkitConfiguration.primeAssets] to configure a global asset prime function.
+Future<void> defaultPrimeAssets(WidgetTester tester, String name, Finder finder) =>
+    matchesGoldenFile(name).matchAsync(finder);


so I think we can simplify the prime assets api...

this old version was such a hack, that I don't think the filename OR the finder actually matter...

I believe we can remove them from the signature, and just hardcode the filename to a constant, and the finder to be the root of the widget tree. That might make the signature more palatable 😄

and just hardcode the filename to a constant

This would result in a file always being created. I will investigate why this hack works and implement sth. similar that does not need a fileName and thus does not create a file.

Ok the legacy version does not work for huge images. Seems like calling mathcesGoldenFile only triggers the decompression and decoding of all images. For small images this may work because they finish decoding before the actual call but for bigger images this does not work reliable. @coreysprague Do you think a test that verifies that the new implementation works even for huge images would be useful? This would probably add another 5mb to the repo.

Ok now I realized that matchesGoldenFIle just consumes some time which is enough for all the small images to be decoded. I ran a benchmark and matchesGoldenFile took an average of 25ms. Waiting for the same duration has the same effect... The time spent in the matchesGoldenFile call should be relative to the time needed to load small images. I suggest just calling the heavy operations done by the function to waste some time in the legacy implementation.

Waiting for a const Duration should be very flaky because we dont know the speed of the device the test is running on.

Just commited legacyPrimeAssets which just wastes time by rasterizing the next RepaintBoundary and converting the result image to a png.

@christian-muertz you are correct. The old one triggered rasterization and essentially just wasted cycles, and generally worked, but not all the time.

I'm not too worried about the large image test for the new solution. The new solution is not dependent on winning a non-deterministic race. I think your proposal for an alternate legacy solution makes sense to me. This legacy implementation should be short lived anyway. I mostly just want to give consumers a release or two to migrate in case there are issues with the new solution.

packages/golden_toolkit/lib/src/testing_tools.dart

coreysprague · 2020-05-27T21:38:57Z

packages/golden_toolkit/test/configuration_test.dart

+      expect(globalPrimeCalledCount, 1);
+    });
+
+    testGoldens('screenMatchesGolden method level primeAssets trumps global configuration', (tester) async {


do you think we still need the method level override?

I can't decide. It would simplify the APIs if we stuck to the global override. With the way flutter test's test_config construct works, it can be set for particular sub-folders, or someone could alter it for just a file (or for a particular test by configuring before and reverting after).

what do you think?

Then probably sth like configuration.copyWith would be helpful

100% agree.

coreysprague

Left some comments. Mostly minor.

We should probably have a test that asserts a golden with an image using your new implementation, that will get the coverage back up, and ensure the behavior is working as expected.

Thanks again!

christian-muertz · 2020-05-28T10:46:37Z

We should probably have a test that asserts a golden with an image using your new implementation, that will get the coverage back up, and ensure the behavior is working as expected.

I am confused where to put such a test. Shouldn't there sth. like screen_golden_test.dart. This would imply moving the screenMatchesGolden to be moved to sth. like screen_golden.dart

christian-muertz · 2020-05-28T10:47:05Z

Or at least a testing_tools_test.dart

coreysprague · 2020-05-28T15:08:32Z

@christian-muertz the test organization is a mess. It evolved very organically and has become unwieldy. For now, you can put it wherever. I think I'll try to find some time to re-organize the tests and make them more clear.

coreysprague · 2020-05-28T15:18:45Z

packages/golden_toolkit/lib/src/testing_tools.dart

-Future<void> _primeImages(String fileName, Finder finder) => matchesGoldenFile(fileName).matchAsync(finder);
+/// A function that primes all assets by just wasting time and hoping that it is enough for all assets to
+/// finish loading. Doing so is not recommended and very flaky. Consider switching to [waitForAllImages] or
+/// a custom implementation.


Suggested change

/// a custom implementation.

/// a custom implementation. This implementation will be removed in a future release.

coreysprague · 2020-06-24T03:08:58Z

@christian-muertz are you okay if I merge this into a branch and finish up the final pieces of feedback and deal with the merge conflicts? I'd like to get this feature included in the next release.

coreysprague · 2020-06-24T03:10:01Z

Also, after thinking about it more, I think your initial instinct was correct. We should make the new image strategy the default behavior. I can provide instructions for how to use the legacy behavior in the release notes.

christian-muertz · 2020-06-24T07:22:38Z

@christian-muertz are you okay if I merge this into a branch and finish up the final pieces of feedback and deal with the merge conflicts? I'd like to get this feature included in the next release.

Sure. Make sure to precache DecoratedBoxes as well

coreysprague linked an issue May 25, 2020 that may be closed by this pull request

screenMatchesGolden sometimes creates failure folders even when the test succeeds #36

Closed

coreysprague mentioned this pull request May 26, 2020

Feature/add configuration api #50

Merged

christian-muertz added 2 commits May 27, 2020 20:24

Find all Image widgets and precache them before comparing

452f818

Make primeAssets function globally configurable

3487dad

christian-muertz force-pushed the prime_images branch from e75e0de to 3487dad Compare May 27, 2020 19:08

Add if (!shouldSkipGoldenGeneration)

2dbfa24

christian-muertz force-pushed the prime_images branch from ca1060b to 2dbfa24 Compare May 27, 2020 19:10

Make analyzer happy

c0ec5f0

coreysprague reviewed May 27, 2020

View reviewed changes

packages/golden_toolkit/lib/src/testing_tools.dart Outdated Show resolved Hide resolved

coreysprague reviewed May 27, 2020

View reviewed changes

coreysprague requested changes May 27, 2020

View reviewed changes

christian-muertz added 2 commits May 28, 2020 12:35

Implement legacyPrimeAssets as a time waste function

0498c53

Adjust PrimeAssets signature

cd40278

coreysprague reviewed May 28, 2020

View reviewed changes

coreysprague changed the base branch from master to image-loading June 24, 2020 15:35

coreysprague merged commit d66b52d into eBay:image-loading Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Find all Image widgets and precache them before comparing #48

WIP: Find all Image widgets and precache them before comparing #48

christian-muertz commented May 25, 2020

codecov-commenter commented May 25, 2020 •

edited

christian-muertz commented May 25, 2020

coreysprague commented May 25, 2020

christian-muertz commented May 25, 2020

coreysprague commented May 27, 2020

christian-muertz commented May 27, 2020

coreysprague May 27, 2020

christian-muertz May 28, 2020

christian-muertz May 28, 2020

christian-muertz May 28, 2020

christian-muertz May 28, 2020

christian-muertz May 28, 2020

coreysprague May 28, 2020

coreysprague May 27, 2020

christian-muertz May 28, 2020 •

edited

coreysprague May 28, 2020

coreysprague left a comment •

edited

christian-muertz commented May 28, 2020

christian-muertz commented May 28, 2020

coreysprague commented May 28, 2020

coreysprague May 28, 2020

coreysprague commented Jun 24, 2020

coreysprague commented Jun 24, 2020

christian-muertz commented Jun 24, 2020

	/// a custom implementation.
	/// a custom implementation. This implementation will be removed in a future release.

WIP: Find all Image widgets and precache them before comparing #48

WIP: Find all Image widgets and precache them before comparing #48

Conversation

christian-muertz commented May 25, 2020

codecov-commenter commented May 25, 2020 • edited

Codecov Report

christian-muertz commented May 25, 2020

coreysprague commented May 25, 2020

christian-muertz commented May 25, 2020

coreysprague commented May 27, 2020

christian-muertz commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-muertz May 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreysprague left a comment • edited

Choose a reason for hiding this comment

christian-muertz commented May 28, 2020

christian-muertz commented May 28, 2020

coreysprague commented May 28, 2020

Choose a reason for hiding this comment

coreysprague commented Jun 24, 2020

coreysprague commented Jun 24, 2020

christian-muertz commented Jun 24, 2020

codecov-commenter commented May 25, 2020 •

edited

christian-muertz May 28, 2020 •

edited

coreysprague left a comment •

edited