#2398. Remove excessive async. Add explicit void

[sgrekhov]

No description provided.

[osa1]

Thanks.

Removing redundant asyncs LGTM.

I'm not sure about the placements of asyncEnds. I've added inline questions/comments.

I may make reviewing easier to separate simple changes (like the async removals here) from stuff that needs careful reviews (the asyncEnds) as otherwise it becomes too easy to miss important details in the large diffs.

[osa1]

Shouldn't this asyncEnd be called after the Expect.equals(1, ...) below? My understanding is unittest-suite-success should be printed as the last thing, and there's an assertion that will run after this line. (the Expect.equals(1, ...) on line 30)

[sgrekhov]

No, it's Ok to place it there. The idea is to make sure that async method was called and only once. For example

import "dart:async";
import "../../Utils/expect.dart";

main() async {
  asyncStart();
  await Future<int>.delayed(Duration(seconds: 1), () {return 42;}).then((value) {
    print(value);
    asyncEnd(); // Ok. It works, no matter that anything is printed after unittest-suite-success
    print(0);
  });
}

The test above will fail if, for some reason, Future.then() won't be called (then test runner determines asyncStart() without asyncEnd()) or will be called more than once (number of asyncEnd() calls should be equal the number of asyncStart() calls).

[osa1]

I'm not sure that's the correct usage of asyncEnd. The test controller that runs in the browser declares a pass when it sees a unittest-suite-success: https://github.com/dart-lang/sdk/blob/12e52d8fde254a0aad0102223324f7aa953146c3/pkg/test_runner/lib/src/test_controller.js#L242-L244

So if you call asyncEnd and then do other checks, those checks will potentially run after the test result is sent back to the test driver and the test result is accepted as "pass".

[osa1]

We discussed this with @mkustermann. The key points are that (1) there should be no assertions after the last asyncEnd (when the async task counter hits), and (2) asyncEnd should never be called when there are more asyncStarts to be run in the program.

As an example to (1), here we can't call asyncEnd in the awaited future:

main() async {
    asyncStart();
    await Future<int>.delayed(Duration(seconds: 1), () {
      ... // We can't call `asyncEnd` here
    });
    Expect.equals(...);
    asyncEnd();
}

This is because the part after the await will run after the future completes, so if we call asyncEnd in the future that concludes the test before the rest of the main runs.

If we didn't have await:

main()  {
    asyncStart();
    Future<int>.delayed(Duration(seconds: 1), () { // no `await` here
      ...
      asyncEnd();
      return 123;
    });
    Expect.equals(...);
}

Now we need to call asyncEnd in the future because the future will complete after main returns (as main does not wait for the future to complete).

As an example to (2), this is a bug:

main() async {
  asyncStart();
  test1();
  asyncEnd(); // BUG: the test result will be reported here.

  asyncStart();
  test2();
  asyncEnd();
}

Here the bug is that the first asyncEnd() decrements the async task counter to 0 and prints 'unittest-suite-success', which again concludes the test early.

For this particular test (1) applies, the asyncEnd should be after the Expect.equals in main.

[eernstg]

@osa1 and @mkustermann, it's great that this topic gets clarified, thanks!

A few things that I noticed in language tests using asyncStart(...) and asyncEnd() and from looking at the declarations of asyncStart and asyncEnd:

asyncStart(k) can be used to get the same effect as calling asyncStart() k times; this is never used.

Dynamically, the invocations of asyncStart() and asyncEnd() must determine a correct parenthesis structure. That is, if we use ( to indicate that asyncStart() has been invoked and ) to indicate asyncEnd(), we're allowed to have invocation histories like '()' or '((()))' or '(()()()(()))', but not ')' or '(()()))('. In other words, for every asyncEnd() there must be a corresponding asyncStart() in the history, and the total execution history must have a corresponding asyncEnd() for every asyncStart(). The final asyncEnd() is special in that it posts the 'unittest-suite-success' message which is interpreted by processMessage in 'test_controller.js'.

A common pattern in tests is to have an outer pair of asyncStart/asyncEnd invocations, and then possible a bunch of inner pairs. E.g., in 'language/await/await_test.dart':

main() {
  asyncStart();
  for (int i = 0; i < 11; i++) {
    asyncTest(staticMembers);
    ...
  }
  asyncEnd();
}

The asyncStart() is typically performed directly at the top level (here: of the body of main), and the asyncEnd() is usually performed in a callback, e.g., in 'language/await/postfix_expr_test.dart':

main() {
  asyncStart();
  test().then((_) { asyncEnd(); });
}

In the case where asyncEnd() is invoked at the top level it is typically guarded by a synchronization command (like await completer.future in 'async_star/async_star_cancel_test.dart'). In any case, some amount of synchronization may be required in order to ensure that the parenthesis structure is correct at run time, and also to ensure that the final asyncEnd() occurs after all elements of the test have been performed.

It may seem more visually appealing to have asyncStart() and asyncEnd() occur symmetrically: They both occur in the body of the same function, with the same amount of indentation. However, the approach where asyncEnd() is called in an argument to .then of a future may be simpler because there is no need to write extra code to achieve the required synchronization. So the style from 'postfix_expr_test.dart' could be a useful standard.

When we have a top-level asyncStart/asyncEnd pair (say, in main), we're free to have nested pairs, as shown in @mkustermann's example in the comment on 'element_type_A02_t03.dart'. This is like '(())'; however, as @mkustermann mentions, we're actually running the asyncStart() in main, then the asyncStart() in isRuntime..., then the asyncEnd() in main and finally asyncEnd() in isRuntime.... In other words, the parenthesis structure of the dynamic invocation sequence exists, but it doesn't reflect the syntactic location of the invocations of asyncStart and asyncEnd.

I think it might be helpful for readers of the code if the invocations of asyncStart and asyncEnd that end up matching each other in the dynamic invocation sequence were also the ones that are located together syntactically. Anyway, that's good for reasoning about the sequence of events, but it isn't a requirement for passing the test run.

We can also have a structure that looks like ()()()()(), as shown in 'async/async_test.dart'. The actual sequence of events in that case is probably ((((())))) because the top-level statement list in main is executed without suspension, and then all the callbacks (function literals passed to .then) are executed after main has returned. Again, this works, but the actual sequencing of events is not necessarily self-explanatory.

@osa1 wrote that @mkustermann said:

asyncEnd should never be called when there are more asyncStarts to be run in the program.

I do think 'async/error_timing_test.dart' will do this, and this test succeeds on 79 out of 83 configurations at this time. I think the actual requirement is that the parenthesis structure is maintained. This basically means that we aren't executing too many asyncEnd() too early, and at the end we have executed exactly the same number of asyncStart() and asyncEnd(), and that balance is not achieved at any time before the end (because that would cause a spurious 'unittest-suite-success'). It is not a problem if we execute asyncEnd() followed by asyncStart(), as long as we maintain this parenthesis structure.

[osa1]

asyncEnd should never be called when there are more asyncStarts to be run in the program.

I do think 'async/error_timing_test.dart' will do this, and this test succeeds on 79 out of 83 configurations at this time. I think the actual requirement is that the parenthesis structure is maintained.

You're right, the rule I mentioned more strict than necessary. The parenthesis rule mostly makes sense to me, but

We can also have a structure that looks like ()()()()(),

Here the first ')' will just end the test. There needs to be an outer parens like (()()()()()).

@osa1 wrote that @mkustermann said:

To clarify, I wasn't quoting Martin. I just wrote my understanding from our discussion. All mistakes are mine 😅 .

---

I think we just can't make the asyncStart/asyncEnd have the same syntactic and runtime order while still testing everything we want to test. The example in my previous comment:

main()  {
    asyncStart();
    Future<int>.delayed(Duration(seconds: 1), () {
      ...
      asyncEnd();
      return 123;
    });
    Expect.equals(...);
}

I agree that it would be good if asyncStart and ends occured symmetrically and in the same lexical scope, but I'm not sure if that's possible in general.

[eernstg]

@osa1 wrote, in response to a remark from me:

We can also have a structure that looks like ()()()()(),

Here the first ')' will just end the test. There needs to be an outer parens like (()()()()()).

Right, that's the reason why I said "looks like" and then continued to note that the actual sequencing in that test ('async/async_test.dart') is ((((())))), in spite of the fact that it looks like ()()()()() syntactically. The point I was trying to make was that the syntax can look nice and still be misleading. In this case, 'async_test.dart' can succeed even though it doesn't syntactically have an extra pair of asyncStart()/asyncEnd() around the whole thing, but it just takes one await to break that.

Actually, we can have a main like this and get in trouble:

void main() {
  asyncStart();
  ... // Perform the actions 
  asyncEnd();
}

This test will fail with an 'asyncStart() was called even though we are done with testing' error if all the actions are delayed, because we will then invoke asyncStart() (the first one) at the beginning of main, then asyncEnd() (the one that is considered to mark the end of testing) at the end of main, and then a bunch of other asyncStart()/asyncEnd() pairs during the execution of various callbacks after the control has returned from main—except that the test will fail soon after main returns. This can easily happen, say because the actions are async functions that are called in main and whose returned futures are discarded.

I tend to think that the safe approach would be to aim for the following style, in all functions used by a test, with main as just one example:

// One approach seen in language tests.
main() {
  asyncStart();
  test().then((_) { asyncEnd(); });
}

// Alternatively.
Future<T> test() async {
  asyncStart();
  await anotherTest();
  asyncEnd();
}

So the idea is that asyncStart() occurs "synchronously" at the top level, and the corresponding asyncEnd() occurs in a callback to .then; or, equivalently, asyncEnd() occurs after code that awaits all futures.

[sgrekhov]

I'm going to have only the following two approaches

main() {
  asyncStart();
  test().then((_) { asyncEnd(); });
}

And if we have more checks

main() {
  asyncMultyStart(2);
  test().then((_) { 
    ...
    asyncEnd(); 
  });
  // More checks
  asyncEnd();
}

This will ensure that async routine was called and will preserve from false-positive outcome in the case when we have additional checks after the first asyncEnd()

In the cases like

Future<T> test() async {
  asyncStart();
  await anotherTest();
  asyncEnd();
}

and

void main() {
  asyncStart();
  await ... 
  asyncEnd();
}

What are the benefits of using asyncStart/End() here? If, in the first case, test() won't be called at all for some reasons, we'll never know about it. If anotherTest() misteriously won't be called, pair of async guards won't catch it. For these two examples I prefer not use asyncStart/End() at all. Am I missing anyhthing?

[eernstg]

What are the benefits of using asyncStart/End() here?

I would expect the test runner to see 'unittest-suite-wait-for-done' and expect 'unittest-suite-success', and it would be detected and reported as a test failure if we get the former and not the latter. This is needed in order to verify that we don't have portions of a test that are skipped entirely (such that there are some expected but missing asyncEnd() invocations). Confirmed: the following test causes an test run failure and shows 'unittest-suite-wait-for-done' on stdout.

import 'package:async_helper/async_helper.dart';
void main() async => asyncStart();

There are many ways we could create a situation where some code isn't executed even though it's associated with a future: It could be deadlocked, the we could have done myFuture.ignore() (or something with the same effect), etc. So I'd say that checking that we get enough invocations of asyncEnd() does make sense.

[sgrekhov]

I mean comparing of

void main() async {
  asyncStart();
  await test(); 
  asyncEnd();
}

vs

void main() async {
  await test(); 
}

I still think that await should discover all of the issues that may happen with test() and async quards won't give much advantages. If test() won't be executed, then shouldn't we expect a timeout in both cases (with and withoun guards)?

[osa1]

Same as my other comment, I think this should be the last line in main.

[osa1]

Revisiting this after my other comment, it looks like this asyncEnd is correct, because the part after the await in this function will run after main ends.

It would be helpful if someone else could confirm (maybe @lrhn or @mkustermann).

[mkustermann]

It's a correct change, but I'd write it differently (imho) somewhat more cleanly like this:

void isRuntimeTypeImplementsStream<T>(Object? o) async {
  ...
  asyncStart();
  List<T> list = await o.toList();
  try {
    list.addAll(<T>[]);
    asyncEnd();
  } catch (...) {
    ...
  }
}

main() async {
  asyncStart();
  dynamic d = await foo();
  FutureOr<Stream<int>> o = d;
  isRuntimeTypeImplementsStream<int>(d);
  asyncEnd();
}

This works because we're going to call asyncStart() before any async operation. So the counter will go up to 2, then main decreases it and then finally isRuntimeTypeImplementsStream decreases it and reports success.

The code is easier to read as the asyncStart and asyncEnd are contained within functions and balanced.

[sgrekhov]

@osa1 @mkustermann how can I reproduce the issue that we are going to fix? I did the following test

import "dart:async";
import "../../Utils/expect.dart";

main() async {
  asyncStart();
  await Future<int>.delayed(Duration(seconds: 1), () => 42).then((value) {
    print(value);
    asyncEnd();
  });
  print("Check 2");
  Expect.isTrue(false);
}

This test has an assertion after asyncEnd() that fails. But the test is not false-positive on VM and Chrome

k$ python3 tools/test.py -n vm-linux-release-x64 co19/LanguageFeatures/Class-modifiers/_A01_t01
No build targets found.
Test configuration:
    vm-linux-release-x64(architecture: x64, compiler: dartk, mode: release, runtime: vm, system: linux)
Suites tested: co19

FAILED: dartk-vm release_x64 co19/LanguageFeatures/Class-modifiers/_A01_t01
Expected: Pass
Actual: RuntimeError

--- Command "vm" (took 02.000479s):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart --sound-null-safety -Dtest_runner.configuration=vm-linux-release-x64 --ignore-unrecognized-flags --packages=/home/sgrekhov/Google/dart-sdk/sdk/.dart_tool/package_config.json /home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/LanguageFeatures/Class-modifiers/_A01_t01.dart

exit code:
255

stdout:
unittest-suite-wait-for-done
42
unittest-suite-success
Check 2

stderr:
Unhandled exception:
Expect.isTrue(false) fails.
#0      _fail (file:///home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/Utils/expect.dart:18:5)
#1      Expect.isTrue (file:///home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/Utils/expect_common.dart:23:7)
#2      main (file:///home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/LanguageFeatures/Class-modifiers/_A01_t01.dart:11:10)
<asynchronous suspension>

--- Re-run this test:
python3 tools/test.py -n vm-linux-release-x64 co19/LanguageFeatures/Class-modifiers/_A01_t01
[00:02 | 100% | +    0 | -    1]

=== 0 tests passed, 1 failed ===

$ python3 tools/test.py -n dart2js-linux-chrome co19/LanguageFeatures/Class-modifiers/_A01_t01
No build targets found.
Test configuration:
    dart2js-linux-chrome(architecture: x64, compiler: dart2js, mode: release, runtime: chrome, system: linux, use-sdk)
Suites tested: co19

FAILED: dart2js-chrome release_x64 co19/LanguageFeatures/Class-modifiers/_A01_t01
Expected: Pass
Actual: RuntimeError

--- Command "dart2js" (took 02.000026s):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dart compile js -Dtest_runner.configuration=dart2js-linux-chrome --sound-null-safety --test-mode --packages=/home/sgrekhov/Google/dart-sdk/sdk/.dart_tool/package_config.json /home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/LanguageFeatures/Class-modifiers/_A01_t01.dart --sound-null-safety --out=/home/sgrekhov/Google/dart-sdk/sdk/out/ReleaseX64/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js

exit code:
0

stdout:
Hint: When run on the command-line, the compiled output might require a preamble file located in:
  <sdk>/lib/_internal/js_runtime/lib/preambles.
Compiled 10,056,105 input bytes (5,087,392 characters source) to 162,985 characters JavaScript in 1.78 seconds using 207.477 MB of memory

--- Command "chrome" (took 01.000075s):
/home/sgrekhov/Google/dart-sdk/sdk/tools/sdks/dart-sdk/bin/dart pkg/test_runner/bin/launch_browser.dart chrome http://127.0.0.1:40193/root_build/generated_tests/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/test.html?crossOriginPort=39325

Runtime window.onerror:
window.onerror called:

http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:2993:7:
Uncaught
Expect.isTrue(false) fails.
    at Object.wrapException (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:412:43)
    at Object._fail (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3151:15)
    at http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3139:17
    at _wrapJsFunctionForAsync_closure.$protected (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:2553:15)
    at _wrapJsFunctionForAsync_closure.call$2 (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3558:12)
    at _awaitOnObject_closure.call$1 (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3546:32)
    at _RootZone.runUnary$2$2 (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3999:18)
    at _Future__propagateToListeners_handleValueCallback.call$0 (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3926:51)
    at Object._Future__propagateToListeners (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:2673:93)
    at _Future._complete$1 (http://127.0.0.1:40193/root_build/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:3771:11)



Deobfuscated error and stack:
window.onerror called:

out/ReleaseX64/generated_compilations/dart2js-linux-chrome/tests_co19_src_LanguageFeatures_Class-modifiers__A01_t01/_A01_t01.js:2993:7:
Uncaught
Expect.isTrue(false) fails.
    at wrapException                                            org-dartlang-sdk:///lib/_internal/js_runtime/lib/js_helper.dart 1196:37
    at _fail                                                    ../../../../../tests/co19/src/Utils/expect.dart 18:5
    at isTrue(inlined)                                          ../../../../../tests/co19/src/Utils/expect_common.dart 23:7
    at main                                                     ../../../../../tests/co19/src/LanguageFeatures/Class-modifiers/_A01_t01.dart 11:10
    at _awaitOnObject                                           org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 303:19
    at _wrapJsFunctionForAsync.<anonymous>                      org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 328:23
    at _awaitOnObject.<anonymous>                               org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 279:19
    at _RootZone.runUnary                                       org-dartlang-sdk:///lib/async/zone.dart 1661:46
    at _zone(inlined)                                           org-dartlang-sdk:///lib/async/future_impl.dart 127:29
    at handleValue(inlined)                                     org-dartlang-sdk:///lib/async/future_impl.dart 162:12
    at _Future._propagateToListeners.handleValueCallback        org-dartlang-sdk:///lib/async/future_impl.dart 838:36
    at _Future._propagateToListeners                            org-dartlang-sdk:///lib/async/future_impl.dart 867:13
    at _Future._complete                                        org-dartlang-sdk:///lib/async/future_impl.dart 634:7


exit code:
0

--- To debug locally, run:
/home/sgrekhov/Google/dart-sdk/sdk/tools/sdks/dart-sdk/bin/dart /home/sgrekhov/Google/dart-sdk/sdk/pkg/test_runner/bin/http_server.dart -p 40193 -c 39325 --network 127.0.0.1 --build-directory=/home/sgrekhov/Google/dart-sdk/sdk/out/ReleaseX64/ --runtime=chrome --packages=/home/sgrekhov/Google/dart-sdk/sdk/.dart_tool/package_config.json

--- Re-run this test:
python3 tools/test.py -n dart2js-linux-chrome co19/LanguageFeatures/Class-modifiers/_A01_t01
[00:04 | 100% | +    0 | -    1]

=== 0 tests passed, 1 failed ===

So, are we reaaly going to fix an existing problem?

[sgrekhov]

And just a note. We have asyncMultiStart(int count) for several asyncEnds() in the test. The following will work

main() async {
  asyncMultyStart(2);
  test1();
  asyncEnd(); 

  test2();
  asyncEnd();
}

The following also will work

void isRuntimeTypeImplementsStream<T>(Object? o) async {
  ...
  List<T> list = await o.toList();
  try {
    list.addAll(<T>[]);
    asyncEnd();
  } catch (...) {
    ...
  }
}

main() async {
  asyncMultiStart(2);
  dynamic d = await foo();
  FutureOr<Stream<int>> o = d;
  isRuntimeTypeImplementsStream<int>(d);
  asyncEnd();
}

So, if the problem really exists the solution will be to use asyncMultiStart(). But I can't reproduce the problem yet

[sgrekhov]

Ah... After more carefull look at test_controller.js I was able to reproduce the issue. notifyDone() sends a message to test driver, so the test bacome racy. It may complete or the driver may sucessfully complete the test. Example

import "dart:async";
import "../../Utils/expect.dart";

main() async {
  asyncStart();
  await Future<int>.delayed(Duration(seconds: 1), () => 42).then((value) {
    print(value);
    asyncEnd();
  });
  print("Wait...");
  await Future.delayed(Duration(seconds: 2));
  print("Wait done.");
  Expect.isTrue(false);
}

This test fails in VM (expected) but successes in Chrome. False-positive outcome. So, we are really have an issue here. @osa1, thank you for discovering this! But... @osa1 @eernstg @mkustermann isn't it a test controller issue? Shouldn't the test runner run the tests on VM and Web in the same way?

[osa1]

isn't it a test controller issue? Shouldn't the test runner run the tests on VM and Web in the same way?

The main function returns before the created futures finish in the VM the same way, but the process runs until all microtasks finish.

In the browser there's no "process" that can finish.

In principle we could keep track of all the created microtasks in the browser and make the main function wait until they're all finished. I mentioned this a few days ago in dart-lang/sdk#52561 (comment).

I don't know what guarantees we give today about the Dart main function compiled to Wasm or JS, but due to Hyrum's Law, there should be some users who depend on the current behavior of main returning before the tasks finish.

So it would be a rather significant change and take time and effort (assuming it's possible).

(I will talk to the JS interop team to get their feedback on this)

[sgrekhov]

Ok. I'll update the tests and add appropriate comments to asyncStart/End() documentation. Turning this PR into a draft for now

[osa1]

I still think it might make sense to remove the redundant asyncs separately because it should be relatively straightforward change that we can just merge.

[sgrekhov]

Makes sense. Updated async tests in this PR to not give false-positive results on web platforms (yes, I know that io library is not supported on web, but, anyway, if we have asyncStart/End() there let they be used in the right way (now we know what is the right way!). I'm going to update all other tests in the same way in a separate PR(s). Please review.

[osa1]

I think it's still not quite right. I've added some inline comments.

Again for the third time, I think it makes sense to split this into two PRs, one with trivial changes (removing asyncs) and one with the important changes. Currently we have to go through hundreds of files every time there's a revision.

[osa1]

I think you need asyncStart and end in main. When you call run_main you create a future which creates a microtask. If the asyncMultiStart line doesn't run synchronously (and runs in the microtask) then you'll potentially miss that print and conclude the test early. cc @mkustermann.

[osa1]

Same as above.

[osa1]

Same as above.

[mkustermann]

lgtm with comments (that apply also to similar cases)

[mkustermann]

This is somewhat confusing, as locally only reading main one doesn't know where the second asyncEnd() is. I'd really encourage you to follow my suggestion above: Any strait line code that triggers an async operation (e.g. await, .then((_) {})) should have an asyncStart at the beginning and an asyncEnd when the operations completes.

Usually there will be balanced asyncStart and asyncEnd in the same function, making it easily understandable.

[mkustermann]

Consider explicitly adding void to these functions.

[mkustermann]

These tests are async tests, they will execute after _main was invoked (the finally {} clause in inSandbox - which theoretically could throw)

[mkustermann]

What is the purpose of this line here? The test isn't using asyncStart() so basically after main returns we expect the test to be successfully done, so any delayed actions are ignored.

I think a slightly better test would be

Future test(Stdout sink) async {
  final future = sink.addStream(Stream.fromIterable([[1, 2, 3, ]));

  // While the `sink.addStream()` is in-progress we cannot touch the sink.
  Expect.throws(() { sink.writeln(); }, (e) => e is StateError);
  // After the `sink.addStream()` is done, we can write to it again.
  await future;
  sink.writeln();
}
main() async {
  asyncStart();
  await test(stdout);
  await test(stderr);
  asyncEnd();
}

[sgrekhov]

All non-trivial changes reverted. They will be added separately (with respect to all comments made here). Now this PR contains only removing of excessive async and adding explicit void to filesystem tests. Please review

[eernstg]

LGTM.

[eernstg]

Yes, that's good, we shouldn't completely eliminate testing the case where main does not have an explicit return type.

[osa1]

Thanks!

[eernstg]

I'm landing this PR. It changes the declaration of main to have return type void (rather than no explicit return type), and it removes unneeded occurrences of async. Other changes that have been discussed here will be handled in a separate PR.