tools icon indicating copy to clipboard operation
tools copied to clipboard

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

Open FMorschel opened this issue 7 months ago • 5 comments

After https://github.com/dart-lang/tools/issues/2081 got closed, we can now go to tests and make the specific test run by clicking on the gutter button on the left:

Image

https://github.com/user-attachments/assets/61eb63ec-f410-417c-aa29-3408cb05723c

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

FMorschel avatar May 28 '25 18:05 FMorschel

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

https://github.com/dart-lang/tools/blob/a0dda7e50bcd99fd3be8512cf546d872ba39e046/pkgs/test_reflective_loader/lib/test_reflective_loader.dart#L153-L164

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

DanTup avatar May 28 '25 18:05 DanTup

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 avatar May 28 '25 20:05 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)');

scheglov avatar May 28 '25 20:05 scheglov

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 avatar May 28 '25 21:05 DanTup

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

DanTup avatar Jun 05 '25 14:06 DanTup