[test_reflective_loader] Use test groups instead of combining names - #2107

@scheglov this is not a priority, but if you have chance to confirm this change wouldn't negatively affect you, I will work on some tests and changelog/etc.

With this change everything works fine for me.

The presentation is obviously slightly different

00:12 +8985: analyzer | generated | SimpleResolverTest | test_argumentResolution_requiredAndNamed_extra

vs new

00:14 +8983: analyzer SimpleResolverTest test_argumentResolution_required_tooFew

We seem to not include generated group though.
If you know why and can fix it, might be fix it?
Otherwise, I think we still can go with this change anyway.

With separators it was a tiny bit easier to read, but this is very small, and probably should be solved the way it was, with combining names, but in the test reporter instead.

I think if the generated groups have the name like SimpleResolverTest | instead of simply SimpleResolverTest, it should resolve too. Any reason not to include it (the trailing |)?

It will probably look bad in VS Code then, which is the point of this change :-)

Currently, the structure looks like this on Testing tab:

The generated would show here, too, as well as |.

On the Test Results tab:

Edit: With the base test package.

We seem to not include generated group though.

Ah, well spotted, I'll look into that (and ensure there are tests).

With separators it was a tiny bit easier to read, but this is very small, and probably should be solved the way it was, with combining names, but in the test reporter instead.

Hmm, I'm not sure how we could do this without affecting other uses of package:test. We just map this to code like:

group('foo', () {
  test('bar', () {

And package test joins them automatically with spaces. I think usually they are written in a way that this makes sense, but I agree it seems odd here.

@jakemac53 do you know if there are any existing ways in package:test to control how the names would be combined, without inserting characters into the name that would show up in the JSON (and therefore the VS Code tree)?

@FMorschel

The generated would show here, too, as well as |.

Actually, I'm not sure about this, I'll have to do some testing. The generated name is in the test_all.dart script which @scheglov is using to run from the terminal, but in VS Code we're going to be using dart test to run the test file directly, and therefore we will only include the names that are in the test file directly (the hierarchy will come from the test files being in the tree, not from the test_all heirarchies).

Sure, the default behavior is to join with spaces as separators, and not everyone wants or likes using something different. My guess is that we might theoretically have a separate Reporter implementation in the analyzer that will join differently. I'm 99% sure that we will not do this, not just for adding | :-)

@jakemac53 do you know if there are any existing ways in package:test to control how the names would be combined, without inserting characters into the name that would show up in the JSON (and therefore the VS Code tree)?

Not that I know of, no.

My guess is that we might theoretically have a separate Reporter implementation in the analyzer that will join differently.

I have some recollection that you can't control the reporter unless running from dart test, although I can't find an issue describing this in the test repo.

Although, if you're likely to continue to run from the terminal with a patched version of test, I guess you could substitute the spaces in the same patch. Maybe one day we can get you onto VS Code and make the test runner more convenient though 😄

I'll tidy this PR up and add some tests (and fix the name issue) - thanks!

It turns out that I only handled name for the top level of suites, it was just lost for any nested suite.

I've pushed changes to handle this - essentially we know create our own hierarchy of group/tests now, and then walk that tree in _addTestsIfTopLevelSuite to pass it to test_package. I did try just calling test_package.group/test() inside defineReflectiveSuite and defineReflectiveTests which simplified the code quite a lot but it resulted in failures in analyzer because of some test that call test() themselves and produced a Cannot call test() after tests have started error - I think it's because it was called after an await. So I went back to the current model of keeping our own structure and then sending them to package:test at the end.

I diffed the output versus the current version for analyzer, and everything seemed the same (besides pipes, and some differences in the printed timestamps).

produced a Cannot call test() after tests have started error - I think it's because it was called after an await.

This happens if test is called after the future returned from main completes.

This happens if test is called after the future returned from main completes.

Ah, interesting. One of the failures was verify_docs_test.dart in the analyzer, but looking at it now, it seems like it should've made those calls before main() returned. I will try making this change again locally and debug a bit more.

Oh, I see the problem... It's these test_all.dart files:

main() {
  defineReflectiveSuite(() {
    dart.main();
    error.main();
    file_system.main();
    generated.main();
    instrumentation.main();
    source.main();
    src.main();
    utilities.main();
    verify_diagnostics.main();
    verify_docs.main();
    verify_tests.main();
  }, name: 'analyzer');
}

(some of those tests like verify_docs are calling test() themselves, and not using methods as is usually done for reflective tests)

They do not await anything so the tests that have await before then calling test() will call it after main completed.

I don't know why it works the other way though (not calling any group or test until after that all completes, and then doing it all).. If no tests have been registered by the time main() completes, are the rules different?

With the latest changes I see spamming on the console when I have a single solo_test

00:00 +0: ClassElementTest_keepLinking test_class_abstract                                         
  Skip: does not have "solo"
00:00 +0 ~1: ClassElementTest_keepLinking test_class_base                                          
  Skip: does not have "solo"
<cut>
00:00 +2 ~719: ClassElementTest_fromBytes test_unused_type_parameter                               
  Skip: does not have "solo"
00:00 +2 ~720: UpdateNodeTextExpectations                                                          
  Skip: does not have "solo"
00:00 +2 ~721: All tests passed!                                                                   

With the latest changes I see spamming on the console when I have a single solo_test

Oh, good spot. I think this is package:test behaviour... as part of this change, I switched from handling solo directly in test_reflective_loader to just using solo: from package:test (since it seemed like unnecessary duplication).

This isn't good though - if there's no way to suppress this (@jakemac53?) then we might have to switch back to handling it locally and just not calling group/test for those things.

This isn't good though - if there's no way to suppress this (@jakemac53?) then we might have to switch back to handling it locally and just not calling group/test for those things.

It is imo good for it to have the same behavior as regular package:test. If we want to change the default way package:test handles solo, then we should address it upstream as opposed to duplicating the feature but with a different UX just to avoid the spam.

"Just to avoid spam" is a quite important reason for me.
When I run tests and one fails, I don't want to dig through 1000+ lines of spam to find the failure message :-P

When I run tests and one fails, I don't want to dig through 1000+ lines of spam to find the failure message :-P

I am not saying it shouldn't spam, its just that we should address that issue in package:test itself, its not an issue specific to this use case.

When I run tests and one fails, I don't want to dig through 1000+ lines of spam to find the failure message :-P

I am not saying it shouldn't spam, its just that we should address that issue in package:test itself, its not an issue specific to this use case.

Ah, that way of solving this problem I do like. If package:test would stop spamming about (1) running tests with success; (2) skipping tests because of presence of solo... then I would not need to continue patching it locally :-)

We do have dart test -r silent, what does it do for solo I'm unsure, but it should skip printing about passed tests, IIRC.

I agree that changing this in package:test would be better. If @jakemac53 doesn't know if there's a strong reason for it to do this (it already indicates skips with numbers), I will see if it's a simple change and send a PR there (and we can always bump package:test here so that no versions of test_reflective_loader have this difference in output).

@scheglov do you know if you are using the expanded reporter also? If you are seeing spam for passed tests it indicates you are probably not getting the compact reporter.

Update: I did just confirm that skipped tests always spam even in compact mode, we should just fix that imo.

Do I have choice? I'm running using test_reflective_loader that uses test().
And AFAIK package:test is hardcoded to use ExpandedReporter.watch(engine, PrintSink(),.

diff --git a/pkgs/test_api/lib/src/backend/invoker.dart b/pkgs/test_api/lib/src/backend/invoker.dart
index 58f1001d..57201df8 100644
--- a/pkgs/test_api/lib/src/backend/invoker.dart
+++ b/pkgs/test_api/lib/src/backend/invoker.dart
@@ -378,7 +378,7 @@ class Invoker {
     _controller.setState(const State(Status.running, Result.success));
 
     _runCount++;
-    Chain.capture(() {
+    // Chain.capture(() {
       _guardIfGuarded(() {
         runZoned(() async {
           // Run the test asynchronously so that the "running" state change
@@ -415,7 +415,7 @@ class Invoker {
             zoneSpecification:
                 ZoneSpecification(print: (_, __, ___, line) => _print(line)));
       });
-    }, when: liveTest.test.metadata.chainStackTraces, errorZone: false);
+    // }, when: liveTest.test.metadata.chainStackTraces, errorZone: false);
   }
 
   /// Runs [callback], in a [Invoker.guard] context if [_guarded] is `true`.
diff --git a/pkgs/test_core/lib/src/scaffolding.dart b/pkgs/test_core/lib/src/scaffolding.dart
index ffcb0083..d060623b 100644
--- a/pkgs/test_core/lib/src/scaffolding.dart
+++ b/pkgs/test_core/lib/src/scaffolding.dart
@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:async';
+import 'dart:io';
 
 import 'package:meta/meta.dart' show isTest, isTestGroup;
 import 'package:path/path.dart' as p;
@@ -13,6 +14,7 @@ import 'package:test_api/src/backend/invoker.dart'; // ignore: implementation_im
 
 import 'runner/engine.dart';
 import 'runner/plugin/environment.dart';
+import 'runner/reporter/compact.dart';
 import 'runner/reporter/expanded.dart';
 import 'runner/runner_suite.dart';
 import 'runner/suite.dart';
@@ -60,7 +62,8 @@ Declarer get _declarer {
     var engine = Engine();
     engine.suiteSink.add(suite);
     engine.suiteSink.close();
-    ExpandedReporter.watch(engine, PrintSink(),
+//    ExpandedReporter.watch(engine, PrintSink(),
+    CompactReporter.watch(engine, stdout,
         color: true, printPath: false, printPlatform: false);
 
     var success = await runZoned(() => Invoker.guard(engine.run),
diff --git a/pkgs/test_core/lib/src/util/io.dart b/pkgs/test_core/lib/src/util/io.dart
index 98bb23b2..39259b29 100644
--- a/pkgs/test_core/lib/src/util/io.dart
+++ b/pkgs/test_core/lib/src/util/io.dart
@@ -20,7 +20,7 @@ import 'pretty_print.dart';
 
 /// The default line length for output when there isn't a terminal attached to
 /// stdout.
-const _defaultLineLength = 200;
+const _defaultLineLength = 100;
 
 /// Whether the test runner is running on Google-internal infrastructure.
 final bool inGoogle = Platform.version.contains('(google3)');

The default reporter is the compact reporter when using the test runner, if you are just directly invoking the test files though then maybe that is the code path you are editing? All kinds of things won't work doing that though and you should move to just using dart test <file>, this should work in the SDK just fine now (it didn't previously).

Using dart test means all of the test_all.dart files become redundant. A subtle difference is that the hierarchy added by the calls to defineReflectiveSuite() in those files will be lost. I don't know how important this is though. As a bonus, I think if you use dart test the tests will run concurrently and complete faster :-)

If using dart test doesn't work, would removing the Skip: does not have "solo" text for every skipped test in the expanded reporter be a reasonable change? I don't know how useful others may find it (I also think it's a bit spammy and unnecessary, but if it's removed, if you don't want to know which tests were skipped it's a bit more difficult).

@scheglov interested in your thoughts on the above... is using dart test to run tests (instead of test_all.dart) reasonable?

It does remove the hierarchy that is currently added by the test_all.dart files calling each other recursively though (and does make the test_all files somewhat redundant).
(cc @bwilkerson @srawlins because these questions also apply to the analysis server)

TIL!

Friendly ping for the comment above:

@scheglov interested in your thoughts on the above... is using dart test to run tests (instead of test_all.dart) reasonable?

It does remove the hierarchy that is currently added by the test_all.dart files calling each other recursively though (and does make the test_all files somewhat redundant). (cc @bwilkerson @srawlins because these questions also apply to the analysis server)

FYI @scheglov, @jakemac53

Noting that dart-lang/test#2542 has been completed so new versions of test should not show solo skipped tests on the compact (default) reporter.

So landing this would not be as much of a problem anymore.

I rebased these changes and fixed the conflicts (with Gemini's help :) they were quite awkward and it apparently is good at this!). When I run analyzer tests locally with dart test there is no skip-spam:

However, if I run without test then there is still skip-spam:

I think this is because the fix was in the compact runner but as @scheglov noted above:

And AFAIK package:test is hardcoded to use ExpandedReporter.watch(engine, PrintSink()

So I'm not sure where that leaves this change. Do we need to have the skip-spam in the expanded reporter (@jakemac53)? Is it reasonable to say you should run these tests with dart test (@scheglov)?

I don't run analyzer tests with dart test, because it uses all my CPU cores, and still takes 2x longer to run than running dart test_all.dart. But I also have patched package:test to avoid spam. So, probably ideally this patch should be merged to upstream. Or a way to specify that dart test_all.dart should use compact runner, not extended that spams.

But it is probably fair that if dart test works well, it is good to go.

Package publishing

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

PR Health

// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Or a way to specify that dart test_all.dart should use compact runner, not extended that spams.

I feel like we discussed being able to override the test reporter somewhere, but I'm unable to find any related issue now. Maybe @jakemac53 remembers?

it uses all my CPU cores, and still takes 2x longer to run

Presumably there is an overhead to compiling/running each suite individually compared to test_all.dart. However, I noticed that you can use dart test test/test_all.dart to go through the test runner but still get the benefit of a single fast compilation. There was only a 2s difference in time for me.

However, it requires https://dart-review.googlesource.com/c/sdk/+/478663 because there was a test with awaits that could call test() after the tests had started.

I've pushed what I hope to fix the failing test - it turns out pkg:test uses a different reporter on GitHub that outputs ticks instead of numbers. I've changed that test to set the reporter explicitly so it should behave consistently between GH and locally now. You'll likely have to trigger the bots again though, I don't think they'll re-run automatically.

At least in the past we have decided not to invest in supporting directly invoked tests without the test runner. There are many many features that are not supported when running that way, and fixing this one isn't going to really move the needle. cc @natebosch to confirm this is still accurate.

Really we should try and solve the issue of compilation taking longer than running the tests.

We do have some limited support for customizing the test behavior when executing without the test runner. It is not very fleshed out and currently not prioritized for changes. The only supported use is in Flutter integration test framework, but if it helps in the short term you can use it and hardcode the reporter to compact.

https://github.com/dart-lang/test/blob/5a2b40d8a5310ee37d5064e36b9b428f1596cc0f/pkgs/test_core/lib/src/direct_run.dart#L31

dart-lang/test#1310

I expect to spend time later this quarter on the test loading and compiling implementation in the test runner, and adding an option to combine test suites into a single compile is something we can try out.

Indeed, I can run my tests using directRunTests without patching package:test.
So, we probably could wrap it into a nice function and use in the analyzer.
OTOH, dart test file.dart does not work on it anymore.

import 'package:test_core/src/direct_run.dart';
import 'package:test_core/src/runner/reporter/compact.dart';

main() {
  directRunTests(
    () {
      defineReflectiveSuite(() {
        defineReflectiveTests(ClassDeclarationResolutionTest);
        defineReflectiveTests(ClassDeclarationResolutionTest_constructor_super);
        defineReflectiveTests(
          ClassDeclarationResolutionTest_primaryConstructor_super,
        );
        defineReflectiveTests(UpdateNodeTextExpectations);
      });
    },
    reporterFactory: (engine) => CompactReporter.watch(
      engine,
      stdout,
      color: true,
      printPath: true,
      printPlatform: false,
    ),
  );
  // defineReflectiveSuite(() {
  //   defineReflectiveTests(ClassDeclarationResolutionTest);
  //   defineReflectiveTests(ClassDeclarationResolutionTest_constructor_super);
  //   defineReflectiveTests(
  //     ClassDeclarationResolutionTest_primaryConstructor_super,
  //   );
  //   defineReflectiveTests(UpdateNodeTextExpectations);
  // });
}

OTOH, dart test file.dart does not work on it anymore.

Yes, this is a feature aimed at implementors of alternative test runners where breaking compatibility with dart test is expected.

The workaround is to keep test_all.dart as is, and generate another like

import 'test_all.dart' as tests;

void main() {
  directRunTests(tests.main);
}

This solution also handles async test declaring as long as the callback returns a Future, but if you start to rely on that it will break the current test_all.dart approach and we have no way to help you avoid that.

Apparently there's some difference in the test output between the Dart dev and Dart 3.5 bots too. I've tweaked the tests to only check for the combined test names (and not any +1 etc. at the start of the line), since that's all the test cares about and it shouldn't be tripped up by those differences.

@scheglov please could you trigger the bots again (hopefully for the last time 🤞); thanks!

@scheglov the bots are green now. Are you happy with this, or do you think we still need to resolve/change anything?

I don't have powers to merge so that's probably for you too, but if you still have concerns I'm happy to make changes.