Add a surround() function.

[nex3]

Given Dart's Zone-based asynchrony, there are a number of use-cases for being able to wrap entire tests—including the setup and teardown—in a single zone (see for example this thread). A straightforward way to accomplish this would be to provide a void surround(Future callback(Future runTest())) function that defines a callback to run around the test and any inner setups or teardowns. For example:

void main() {
  surround((runTest) {
    return runZoned(runTest, zoneSpecification: myZoneSpecificiation);
  });

  // ..
}

There are a lot of edge-cases that would need to be worked out, and it's possible that one of them will make this not work. But it's something to keep in mind.

[natebosch]

I think this is something we should push on.

Some open questions I have:

• Naming - my current inclination is wrapCallbacks. Are there other options?
• What level of granularity should this apply to? I think for the sake of conceptual integrity it makes sense to work like setUp does today. You can define multiple levels of wrapping at each group and they would nest in the same order as groups. This has the potential to be abused and lead to confusing test patterns, but it's also I think the simplest building block that would solve all use cases uniformly.

cc @jakemac53 @grouma for thoughts.

cc @mehmetf for anything we should be thinking about in terms of solving the flutter_test goals. Does this need to even be per-test? I could see this as an additional named argument to test() if we need that level of granularity.

[mehmetf]

Yes, we would need this at the granularity of test for flutter_test use case. It creates a new FakeAsync object per test to ensure isolation.

[jakemac53]

Yes, we would need this at the granularity of test for flutter_test use case. It creates a new FakeAsync object per test to ensure isolation.

I believe the proposed API would already support that without a callback to test - it looks like it would be invoked once for each test and its corresponding setUp/tearDown invocations.

[jakemac53]

@natebosch how does this feature interact with setUpAll and tearDownAll?

[natebosch]

I believe the proposed API would already support that without a callback to test - it looks like it would be invoked once for each test and its corresponding setUp/tearDown invocations.

Yes, the question is whether you need to create a different closure for each callback. For example to capture a different FakeAsync instance to use for each test if it needs to be initialized and passed in and can't be created within that callback. flutter_test also needs to set this up within testWidgets which wraps a call to test. They don't have a call to group or similar to use to put this.

@natebosch how does this feature interact with setUpAll and tearDownAll?

I don't think it would be used for those at all.

Yes, we would need this at the granularity of test for flutter_test use case. It creates a new FakeAsync object per test to ensure isolation.

@mehmetf - one thing I'm curious about as we get further with this - there is not argument to the setUp callback. If we surround those callbacks with fakeAsync, how will time be progressed around an await within the setUp callback?

[grouma]

What happens if a user nests multiple calls to surround?

[mehmetf]

If we surround those callbacks with fakeAsync, how will time be progressed around an await within the setUp callback?

You can't and therefore you can't await in a setup function that runs in fakeAsync. The test callback needs to await/resolve futures.

Either way, the behavior is surprising but for different reasons.

Today, the surprise is due to setup functions running in real async zone. You can await in it but things break down if the future you created is not resolved by the time test callback executes. This creates subtle bugs.

With surround, the surprise is going to be the fact that await() no longer works because setup runs in fake async zone. However, at least, that... makes sense and you will be able to tell it right away (it will hang). If you want to be able to await a future then don't use setup; instead just make a method and call it from your testWidgets() function and pass in the fake async object so you can progress the time.

Other issues to consider:

• test files that contain both testWidgets and test functions. The setUp in such files need to be able to work for both of these.
• setUp that wants to do real async work (like creating files/directories). This is a tough one and I don't think we should allow such a use of setUp in fake async environment. The user should either use sync APIs or, again, make a setup method that takes in the tester object and can then call tester.runAsync. We should discourage such behavior because it is typically hard to reset reliably. For instance, they should use package:file and create an in-memory fs instead.

These solutions are not ideal :(

The best solution is either supporting fake_async in dart testing explicitly or disallowing setup/teardown functions completely in widget tests. The latter is what Ian had proposed but it is an enormous breaking change that will make a lot of people upset.

[natebosch]

The best solution is either supporting fake_async in dart testing explicitly or disallowing setup/teardown functions completely in widget tests. The latter is what Ian had proposed but it is an enormous breaking change that will make a lot of people upset.

I worry that breaking all async setUp callbacks will be nearly as difficult to roll out.

You can await in it but things break down if the future you created is not resolved by the time test callback executes. This creates subtle bugs.

How often does that happen given that we strongly encourage awaiting all futures? Do you have an example of a bug where a Future was not awaited in setUp and did not resolve before the test callback started?

[mehmetf]

I worry that breaking all async setUp callbacks will be nearly as difficult to roll out.

I worry that you are right. There does not seem to be a good solution to this. Either way, the user is restricted and needs to understand some nuance between fake and real async zones.

I will put together a concrete use case and we can examine. Given how difficult this road has been and the fact that the solution will create other problems, I am inclined to look at that case and see what else we can do. Perhaps lints? documentation?

I even wonder if a language level change might help here. Perhaps we could tag certain Futures so that if they are awaited in a different zone than they were created, we throw a runtime error instead of hanging...

[mehmetf]

I looked at a couple of use cases that necessitated running setUp/tearDown functions in fake async zone. In either case, the problem is that we are using some framework that is relying on an asynchronous operation but is not giving the user a way to await for the said operation directly. Rather, it relies on callbacks which often gets buried under several setState calls. This is (unfortunately) a growing pattern in Flutter because you can get away with it due to unidirectional data flow. There are ways around this. You can always construct a Completer which completes when the side-effect callback gets executed and await that. However, it feel unnatural and it means you are putting in code that's only meant for testing, which is a design-smell.

Given that our efforts so far did not yield a desirable design, I am OK simply pushing frameworks to expose such operations as Futures that can be awaited. This is easier to do in google3 since both of these cases were internal. Thanks for the very valuable discussion regardless.

PS: There are other use cases for surround or running setup/teardown callbacks in the same zone as test. Being able to reuse zone variables, for instance. However, I will no longer push for using this feature to run the callbacks in FakeAsync.

[natebosch]

cc @lrhn for thoughts.

This API could make it easier to test with zone overridden values for the redesign of package:platform.

[lrhn]

It would bring us one step close to proper aspect oriented programming 👍

It's not technically necessary, you can always define your own:

surround(
   void Function() Function(void Function()) surround,
   void Function(void Function(String, void Function()) test) callback) {
  callback((msg, body) => test(msg, surround(body));
}
/// and then
surround((body) {
   runZoned(body, zoneSpecification : ...);
}, (test) {
  test("test1", () { 
     // runs in zone
  });
  test("test2", () {
    // also runs in zone.
  });
}

It doesn't solve the issue of running multiple tests in the same zone, but keeping tests as separate as possible is probably a good idea anyway.

We could also make surround (or wrap) an optional named parameter on the group method.

void group(String name, void Function() tests, {void Function() wrap(void Function()? test)} { ... }

and then each test invocation would call the surrounding group's wrap methods in inside-out order to get the actual test to run.
Works much better with "named arguments anywhere" - dart-lang/language#1072, so you can put the wrap function before the tests instead of after. We should totally do that!

(I'd probably want the same for tear-down and set-up, but then I never liked the current approach where you declare shared variables and set them up and tear them down repeatedly. It makes it impossible to run async tests concurrently (but then again, single threaded execution is so much easier to reason about, so it's probably better for the average user. Still, it would be safer to have surround set up the state for each test, and then pass it as an argument to the test body. Much harder to type, though.).

[natebosch]

Another potential use case:

https://github.com/nt4f04uNd/flutter/blob/1fccad1b1ef0631362e97bfb4fcf8bfc5304e571/packages/flutter/test/foundation/leak_tracking.dart#L46-L73

cc @polina-c - would you be interested in an API where you could configure the leak tracker wrapping at the group level?