Group classes tests with the `group` function from `test`

[FMorschel]

After #2081 got closed, we can now go to tests and make the specific test run by clicking on the gutter button on the left:

fix.mp4

I'd like to request that we internally use the group function from test to wrap all tests in the class (and then not include the class name in the test name but in the group), and then pass the group location (either the class definition or the defineReflectiveTests call that runs it) up so we can run all tests in one class at a time.

CC @DanTup

[DanTup]

@scheglov it's been a while, but do you recall if there was a reason not to use a group() call for the test class here:

tools/pkgs/test_reflective_loader/lib/test_reflective_loader.dart

Lines 153 to 164 in a0dda7e

	for (var group in _currentGroups) {
	if (allGroups || group.isSolo) {
	for (var test in group.tests) {
	if (allTests || test.isSolo) {
	test_package.test(test.name, test.function,
	timeout: test.timeout,
	skip: test.isSkipped,
	location: test.location);
	}
	}
	}
	}

Currently the tests just have the class name prefixed to the test names (even though internally test_reflective_loader represents them with its own Group type).

[scheglov]

I did not use group() mostly because I did not need it.
And I also use a patched version of package:test to reduce output.
Here is all I see in the terminal:

scheglov@scheglov-macbookpro4:~/Source/Dart/sdk.git/sdk (elements-remove-PrefixElementImpl)$ dart /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/dart/ast/ast_test.dart
00:00 +157: All tests passed!

If a test fails, I get enough information: class name, and test name.

scheglov@scheglov-macbookpro4:~/Source/Dart/sdk.git/sdk (elements-remove-PrefixElementImpl)$ dart /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer/test/dart/ast/ast_test.dart
00:00 +124 -1: PropertyAccessTest | test_isNullAware_true [E]
  Manual failure
  package:matcher/src/expect/expect.dart 152:31                      fail
  pkg/analyzer/test/dart/ast/ast_test.dart 1240:15                   PropertyAccessTest.test_isNullAware_true
  dart:mirrors-patch/mirrors_impl.dart 363:70                        _InstanceMirror._invoke
  dart:mirrors-patch/mirrors_impl.dart 359:25                        _InstanceMirror.invoke
  package:test_reflective_loader/test_reflective_loader.dart 264:26  _runTest
  ===== asynchronous gap ===========================
  package:test_api/src/backend/declarer.dart 242:9                   Declarer.test.<fn>.<fn>
  ===== asynchronous gap ===========================
  package:test_api/src/backend/declarer.dart 240:7                   Declarer.test.<fn>
  ===== asynchronous gap ===========================
  package:test_api/src/backend/invoker.dart 282:9                    Invoker._waitForOutstandingCallbacks.<fn>


To run this test again: dart test . -p vm --plain-name 'PropertyAccessTest | test_isNullAware_true'
00:00 +156 -1: Some tests failed.

Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'dart test --chain-stack-traces'.

Unhandled exception:
Dummy exception to set exit code.

I don't have any specific arguments against using group(), as long as this does not break my workflow :-)

[scheglov]

My patch for package:test

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

[DanTup]

I don't have any specific arguments against using group(), as long as this does not break my workflow :-)

Great - then I might try changing this and confirm everything still works for me, and then let you review/test the patch to ensure it doesn't break anything for you.

And I also use a patched version of package:test to reduce output.

I had some terminal filters to do similar, though the recent changes (and the request here) are to improve integration with the test runner in VS Code, so it'll be easy to jump between failing tests and see their output instead of having to run from the terminal 🙂 (I guess this doesn't help you if you're using IntelliJ though.. I don't know what it's test runner - if it has one - is like).

[DanTup]

I made a draft PR at #2107 that includes as change to use groups. Not a priority, but would like your input/opinion when you have time :-)