fix issue 12999: Make assets available during tests #31207

chunhtai · 2019-04-17T17:23:38Z

Description

Make assets available during tests. This PR add a step to build asset bundle during test setup.
The test will load the bundle and mock the message channel to bypass the engine.

Related Issues

#12999

Tests

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?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

chunhtai · 2019-04-17T17:35:31Z

packages/flutter_tools/lib/src/commands/test.dart

This change will leave behind a folder 'test_assets' in root directory.

Lets put this under build

but it is not necessary to have build folder when running test, should I just create the build folder here?

It looks like test already write something into build. fixed

chunhtai · 2019-04-17T17:39:39Z

packages/flutter_test/lib/src/widget_tester.dart

This will run at the beginning of every test. I make it so it will only mock the first time it is called, after that will be no op

I think it would be better to move this into the binding initialization code.

chunhtai · 2019-04-17T17:42:55Z

This will potential be a breaking change. If developer also mocking the message channel in their code. It might cause unexpected behavior during the test. Not sure if we need to care about this

jonahwilliams · 2019-04-17T18:03:49Z

packages/flutter_tools/lib/src/test/flutter_platform.dart

Use fs.path

jonahwilliams · 2019-04-17T18:04:29Z

packages/flutter_tools/lib/src/commands/test.dart

This needs to happen after the pub check, so that any assets from package dependencies will already exist.

chunhtai · 2019-04-17T20:04:25Z

packages/flutter_test/lib/src/binding.dart

I updated this function. Previously, it only mock when the first time it is called, and no op at later call.
That will pose an issue when some of our tests also mock the message channel for some specific test cases.

I updated it so it will mock or override existing mock every time it is called. To ensure the performance, I implement _ensureInitialized method to do the expansive operation.

jonahwilliams · 2019-04-17T20:22:12Z

packages/flutter_test/lib/src/binding.dart

Are these always strings?

It should be. will you suggest using Set?

Set<String> would be preferable to dynamic.

jonahwilliams · 2019-04-17T20:36:33Z

packages/flutter_test/lib/src/binding.dart

We should call this something other than mock. mockFlutterAssets?

Actually, since all fields are static I would remove the class entirely and place them under the test bindings.

jonahwilliams · 2019-04-17T22:00:46Z

packages/flutter_tools/lib/src/commands/test.dart

Does this add measurable overhead when running the test over and over again? If so, we could potentially reuse the existing pubGet check, make it return a bool for whether or not the pubspec.yaml file had updated. Only if the pubspec file is updated, or if the asset directory doesn't exist, do we need to re-run ot

This will only run once within a flutter test. The overhead is depends on the assets size.
I check the code in flutter run. the run cold will build it everytime, while run hot will check if the pubspec.yaml change or not. I think it is ok to always rebuild during test. if we want to be smart, we not only need to check the pubspec.yaml, we also need to check ant of assets files changed or not.

Actually, thinking about it more - we can't really do this, since a user may edit an asset which would then not be copied.

We should expose a flag in the test command to disable this functionality though

you mean expose a flag to skip rebuilding asset?

yes, similar to how we have a --pub,--no-pub flag.

jonahwilliams · 2019-04-18T17:00:55Z

packages/flutter_test/lib/src/binding.dart

Why not use the setMethodCallHandler method, then it wouldn't be a breaking change as the mocks would still take precedence.

If I understand correctly, settMockMessageHandler is setting _mockHandlers, which is used for outgoing message. setMethodCallHandler is setting _handlers, which is for in coming message. The assets request is outgoing message. I think setting the _handlers will not be able to intercept the request?

Ahh good point, my bad!

chunhtai · 2019-04-18T18:07:25Z

packages/flutter_tools/lib/src/commands/test.dart

Added a flag.
Two thing concerns me here.

Should we build the assets by default?
2, When no-assets, should we still try to see if there is test assets built in previous test and use it if it exist?

My current implementation is yes for both

my two general concerns:

Test startup speed - does building the asset bundle slow it down measurably? Try time flutter test <<something_test.dart>> in the gallery and compare.

Users that are already mocking assets. Maybe --no-assets should also not set the env variable

I didn't see too much difference. maybe 1 or 2 seconds slower out of 2mins. But I do want to point out one thing. The existing test that involve assets load are currently passed because of our fake async. The assets load never finished without my fix. Otherwise they will all fail. This might add some more running time

jonahwilliams · 2019-04-18T18:07:33Z

packages/flutter_tools/lib/src/commands/test.dart

I would name this something more specific like --no-test-assets.

jonahwilliams · 2019-04-18T18:16:55Z

Also, if you're concerned about rolling this out, you can always default it to off. though you'll want to make sure it is used by the gallery tests

jonahwilliams

LGTM

I have no other concerns from a tooling perspective. This implementation leaves us some room to optimize later if we need to, without breaking any users.

chunhtai · 2019-04-18T19:46:06Z

@jonahwilliams I will like to enable the assets load by default since it is more intuitive to have the functionality enabled.

After think a little bit more, this should not be a breaking change, either. We should be good to merge this pr

Hixie · 2019-04-23T19:47:11Z

This hurt test performance (see e.g. flutter_test_performance with_coverage_time_ms).
Filed #31511

gadfly361 · 2019-04-26T17:02:41Z

@chunhtai Did this PR cover flutter drive or is it just for making assets available to flutter test? If just for flutter test is there planned support forflutter drive coming?

chunhtai · 2019-04-26T17:06:34Z

this is only for flutter test. I believe flutter drive is using the really device and assets is already available? @jonahwilliams

jonahwilliams · 2019-04-26T17:27:44Z

@gadfly361 flutter drive runs your real application - assets are already loaded

gadfly361 · 2019-04-26T19:47:59Z

@jonahwilliams @chunhtai Thank you for the quick reply!

leishuai · 2019-07-08T02:47:48Z

@jonahwilliams @chunhtai Thank you for the quick reply!

I found flutter drive can't load assets when running.

chunhtai · 2019-07-08T16:54:25Z

@jonahwilliams @chunhtai Thank you for the quick reply!

I found flutter drive can't load assets when running.

Hi @leishuai
Can you provide a reproducible code that can demonstrate the issue you are facing? In theory, the asset should be able to load without problem since driver test have the real message channel.

leishuai · 2019-07-09T12:36:29Z

@chunhtai
If I try with a simple demo app, it works. But in my app, there are lots of plugin and startup works in native(swift/kotlin) side. So I really don't know why it doesn't work. I also found other people complained about these downside.

chunhtai · 2019-07-09T16:34:52Z

@leishuai Let's continue our discussion in here #35720

MisterJimson · 2019-12-10T17:39:47Z

FYI this PR seems to have caused #35907

googlebot added the cla: yes label Apr 17, 2019

chunhtai force-pushed the issue/12999 branch from ef50714 to c2d7e2a Compare April 17, 2019 17:33

chunhtai commented Apr 17, 2019

View reviewed changes

chunhtai requested review from Hixie, goderbauer and jonahwilliams April 17, 2019 17:39

jonahwilliams reviewed Apr 17, 2019

View reviewed changes

chunhtai commented Apr 17, 2019

View reviewed changes

jonahwilliams reviewed Apr 17, 2019

View reviewed changes

goderbauer added framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Apr 17, 2019

jonahwilliams reviewed Apr 17, 2019

View reviewed changes

jonahwilliams reviewed Apr 18, 2019

View reviewed changes

chunhtai commented Apr 18, 2019

View reviewed changes

jonahwilliams reviewed Apr 18, 2019

View reviewed changes

jonahwilliams approved these changes Apr 18, 2019

View reviewed changes

fix issue 12999: Make assets available during tests

d01b397

chunhtai force-pushed the issue/12999 branch from a6bd01b to d01b397 Compare April 18, 2019 20:51

chunhtai merged commit b275e11 into flutter:master Apr 18, 2019

Hixie mentioned this pull request Apr 23, 2019

Test performance has regressed #31511

Closed

mo-fu mentioned this pull request Jun 3, 2019

Widget tests fails with 'Unable to load asset' in 1.5.4-hotfix.2 #32609

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

fix issue 12999: Make assets available during tests #31207

fix issue 12999: Make assets available during tests #31207

Uh oh!

Conversation

chunhtai commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

chunhtai commented Apr 17, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Apr 17, 2019 •

edited

Loading

chunhtai commented Apr 26, 2019 •

edited

Loading

chunhtai commented Jul 9, 2019 •

edited

Loading