Make testBenchmarks() synchronous.

[munificent]

This avoids calling group() after an async pause, which breaks running short_format_test.dart and tall_format_test.dart outside of the test runner.

cc @scheglov

[natebosch]

FWIW the test mechanism for doing async work before running tests is setUpAll.

  late final List<Benchmark> benchmarks;

  setUpAll(() async {
    benchmarks = await Benchmark.findAll();
  });

This avoids calling group() after an async pause

Can you clarify this? In testBenchmarks there is still an await findPackageDirectory() preceding the group call.

[natebosch]

What do you think of dart-lang/test#2251

This would change the error output to:

Bad state: Can't call group() once tests have begun running.
When running a test as an executable directly (not as a suite by the test runner), tests must be declared in a synchronous block.
If async work is required before any tests are run use a `setUpAll` callback.
If async work cannot be avoided before declaring tests, all async events must be complete before declaring the first test.

[munificent]

Yeah, that's definitely clearer.

At the same time... I wonder if it would be better for the test package to have the same semantics when you run a test file standalone or not? That would be a breaking change and maybe hard to roll out, but it would avoid problems like this.

[natebosch]

I wonder if it would be better for the test package to have the same semantics when you run a test file standalone or not? That would be a breaking change and maybe hard to roll out, but it would avoid problems like this.

I think I have a vague recollection of discussing this previously and deciding against it, but I can't track it down. My understanding is that the vast majority of tests are run with the test runner so most users never see this difference. A downside to changing behavior would be that many more people will hit this error, with the upside that it's less confusing for the small fraction of users who do run tests standalone.

[jakemac53]

Fwiw, when macros are released we could very likely fix this issue for standalone tests because we could wrap the users main and await the future it returns.

[natebosch]

Fwiw, when macros are released we could very likely fix this issue for standalone tests because we could wrap the users main and await the future it returns.

Interesting - so as long as the user applies the macro to main it could make the tests work the same in both cases. I think that will be a good feature to look at down the line - we can then update this error to suggest applying the annotation.

[munificent]

Can you clarify this? In testBenchmarks there is still an await findPackageDirectory() preceding the group call.

Oh, geez, you're right. Sorry, still got some brain fog.

Actually, now I don't know how this PR fixes the issue. It definitely does. I couldn't run the tests standalone before this change but I can now.

Given that this is a fix for an unusual way of running the tests, I'm inclined to just leave it alone but I do wonder what's going on here. There is a whole bunch of other async work that happens to queue up tests (all of the await testDirectory() calls), so I don't know what kind of async is problematic and what kind isn't.

[natebosch]

I'm inclined to just leave it alone

Any concern with moving to the setUpAll pattern? I can open a PR

[natebosch]

Oh, this falls into the second category of my message. Async work that is required before declaring tests.

I'm also confused how this change caused them to start passing...

It looks like it would be pretty invasive to refactor these tests with all async work done before the first test is declared.

[munificent]

It looks like it would be pretty invasive to refactor these tests with all async work done before the first test is declared.

Yeah, it would be hard. Doable, but annoying.

[natebosch]

it would be hard.

If you hit more problems with the standalone test behavior add a comment on dart-lang/test#2253 and we can consider something like Future<void> declareTests(Future<void> Function()) to workaround this without having to do a heavy refactor.

If the current tests are working then I think it's OK to wait until they break again to worry about it further. I have no idea what async interleaving happens to make it work, but all the test cases are running and it's passing so 🤷