Use a new future instead of a cached future from another zone

[Hixie]

As far as I can tell the comment doesn't make sense -- you don't pay the microtask hit unless you await the future.

Fixes #830

[Hixie]

Interestingly, it was added in #546 which suggests that this might be needed to not break Flutter in some other way. So maybe this PR is not a good fix for the zone bug.

[natebosch]

@Hixie - here's the original thread with a link to a Travis failure that I believe is the issue _emptyFuture was intended to solve.

Can you confirm whether this change causes a similar problem?

[grouma]

This sounds related to dart-lang/sdk#32556

We had to do a hacky work around here: #787 Can the same approach apply?

[natebosch]

Another pattern I'd be curious to try:

_emptyCallback() {}
final _emptyFuture = new Future.sync(_emptyCallback):

[tvolkert]

afaik, Future.sync() just delegates to Future.value() under the hood...

[tvolkert]

here's the original thread with a link to a Travis failure

Can you post the link?

[natebosch]

Can you post the link?

😳

#529 (comment)

[tvolkert]

Ah, yes, I remember that!

So here's the test/line that was affected:

https://github.com/flutter/flutter/blob/50a1d91ff39df06c7aed57f7242968a3cf95fa7a/packages/flutter/test/services/system_chrome_test.dart#L22

[tvolkert]

Ok, in flutter/flutter#17415, I've created a test that:

1. fails without this PR
2. passes with this PR

Further, I've verified that this PR does not regress system_chrome_test.dart.

tldr: this fix LGTM

[natebosch]

@grouma can you confirm that the quiver tests and package:fake_async tests also still pass with this change? I would only expect this to make things better for those packages, not worse, but we should double check.

[Hixie]

(I don't have commit rights in this repo, so when this is ready to land, please feel free to land it. :-) Thanks!)

[grouma]

package:fake_async fails here: https://github.com/dart-lang/fake_async/blob/master/test/fake_async_test.dart#L461

package:quiver fails here: https://github.com/google/quiver-dart/blob/master/test/testing/async/fake_async_test.dart#L567

[natebosch]

Two other possibilities to try:

• final _emptyFuture = new Future.value()..then((_) {});
• Return null instead of a future and default it from expectLater.

[grouma]

Note that nbosch@ meant:
final _emptyFuture = new Future.value()..then((_) {});
I have confirmed that all tests pass in each package with either option. I'm having a difficult time confirming that either approach resolves the test in flutter/flutter#17415. I'm not sure how to do a dependency override for the flutter_test tests. @tvolkert or @Hixie can you confirm that either approach resolves your issue?

[tvolkert]

@grouma adding a dependency override at the bottom of https://github.com/flutter/flutter/blob/master/packages/flutter_test/pubspec.yaml#L77 was working for me yesterday - does it work for you?

(you'll need to run flutter packages upgrade after you add it)

[grouma]

Ran a bunch of tests with the various options. The only thing that passes for all tests in package:async, package:quiver and package:flutter_test is return new Future.sync(() {});. I guess Future.value isn't equivalent.