[test_reflective_loader] Use test groups instead of combining names

[DanTup]

@scheglov this change switches to using package:test groups instead of just combining names (#2104). This would improve how things appear in the VS Code test runner because there would be groups instead of everything being flat at the top:

I don't know if it might negatively affect you - one difference when running from the terminal (without the test runner) is that there won't be pipe-separated names, they will just be space-separated (which is what package:test does for groups/tests:

If we want to go ahead, this will need a little more work (tests, versions/changelog updating etc.) but I wanted to get your input first.

[DanTup]

@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.

[scheglov]

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.

[FMorschel]

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 |)?

[scheglov]

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

[FMorschel]

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.

[DanTup]

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).

[scheglov]

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]

@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.

[DanTup]

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!

[DanTup]

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).

[jakemac53]

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.

[DanTup]

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.

[DanTup]

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?

[scheglov]

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!                                                                   

[DanTup]

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.

[jakemac53]

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.

[scheglov]

"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

[jakemac53]

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.

[scheglov]

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 :-)

[FMorschel]

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.

[DanTup]

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).

[jakemac53]

@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.

[scheglov]

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(),.

Patch

You can add a header

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)');

[jakemac53]

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).

[DanTup]

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).

[DanTup]

@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)

[mosuem]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the test discovery logic to use package:test's group() function, creating a more organized, hierarchical test structure. I've identified a critical syntax error in the _Test constructor call that needs to be addressed, as well as a medium-severity suggestion to improve performance by caching the regular expression. Addressing these points will enhance the code's correctness and efficiency.

[gemini-code-assist]

The _Test constructor's parameters are not in the correct order. In Dart, positional arguments must precede named arguments. The function literal, intended as the second positional argument, is placed after named arguments, leading to a syntax error.

        _addTest(
          _Test(
              memberName,
              () => expectFail
                  ? _runFailingTest(classMirror, symbol)
                  : _runTest(classMirror, symbol),
              location: memberMirror.testLocation,
              solo: isSolo,
              skip: isSkipped,
              timeout: timeout?._timeout
          ),
        );

[gemini-code-assist]

Consider defining the regular expression RegExp('(solo_|fail_|skip_)*test_') as a top-level constant for improved performance and readability. Caching the RegExp avoids re-compilation on each method evaluation, aligning with Effective Dart's performance guidelines.

final _testMethodRegExp = RegExp('(solo_|fail_|skip_)*test_');

      var isTest = _testMethodRegExp.hasMatch(memberName);

[mosuem]

@DanTup sorry for using your PR to test the Gemini code review feature ;)

[DanTup]

@mosuem heh, np - I've been using it lately too on Dart-Code. It's been generally giving me better comments than Copilot and has pointed out a few good bugs. Although the "critical" issue above is not so good 😄

[mosuem]

Although the "critical" issue above is not so good

But the signature is actually this - or what am I missing?

_Test(
    super.name,
    this.function, {
    required super.location,
    required super.solo,
    required this.skip,
    required this.timeout,
  });

[DanTup]

@mosuem yep, but "In Dart, positional arguments must precede named arguments" has not been the case for a long time (Dart 2.17 - see dart-lang/language#1072).

[mosuem]

TIL!