From 1e75249e1ad27f25eaed886f05db2f51850106ec Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Thu, 5 Jun 2025 15:45:03 +0100 Subject: [PATCH 1/3] [test_reflective_loader] Use test groups instead of combining names See https://github.com/dart-lang/tools/issues/2104 --- .../lib/test_reflective_loader.dart | 63 +++++++++---------- 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart index 9c3a103ce..c65c10b61 100644 --- a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart +++ b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart @@ -27,7 +27,6 @@ const Object soloTest = _SoloTest(); final List<_Group> _currentGroups = <_Group>[]; int _currentSuiteLevel = 0; -String _currentSuiteName = ''; /// Is `true` the application is running in the checked mode. final bool _isCheckedMode = () { @@ -43,17 +42,14 @@ final bool _isCheckedMode = () { /// add normal and "solo" tests, and also calls [defineReflectiveSuite] to /// create embedded suites. If the current suite is the top-level one, perform /// check for "solo" groups and tests, and run all or only "solo" items. -void defineReflectiveSuite(void Function() define, {String name = ''}) { - var groupName = _currentSuiteName; +void defineReflectiveSuite(void Function() define, {String? name}) { _currentSuiteLevel++; try { - _currentSuiteName = _combineNames(_currentSuiteName, name); define(); } finally { - _currentSuiteName = groupName; _currentSuiteLevel--; } - _addTestsIfTopLevelSuite(); + _addTestsIfTopLevelSuite(name); } /// Runs test methods existing in the given [type]. @@ -87,8 +83,7 @@ void defineReflectiveTests(Type type) { { var isSolo = _hasAnnotationInstance(classMirror, soloTest); var className = MirrorSystem.getName(classMirror.simpleName); - group = _Group(isSolo, _combineNames(_currentSuiteName, className), - classMirror.testLocation); + group = _Group(isSolo, className, classMirror.testLocation); _currentGroups.add(group); } @@ -147,21 +142,33 @@ void defineReflectiveTests(Type type) { } /// If the current suite is the top-level one, add tests to the `test` package. -void _addTestsIfTopLevelSuite() { +void _addTestsIfTopLevelSuite([String? name]) { if (_currentSuiteLevel == 0) { void runTests({required bool allGroups, required bool allTests}) { - 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); - } + /// A helper to run the tests that may optionally be wrapped in a group + /// if defineReflectiveSuite was given an explicit name. + void runTestsImpl() { + for (var group in _currentGroups) { + if (allGroups || group.isSolo) { + test_package.group(group.name, () { + 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); + } + } + }); } } } + + if (name != null) { + test_package.group(name, runTestsImpl); + } else { + runTestsImpl(); + } } if (_currentGroups.any((g) => g.hasSoloTest)) { @@ -175,18 +182,6 @@ void _addTestsIfTopLevelSuite() { } } -/// Return the combination of the [base] and [addition] names. -/// If any other two is `null`, then the other one is returned. -String _combineNames(String base, String addition) { - if (base.isEmpty) { - return addition; - } else if (addition.isEmpty) { - return base; - } else { - return '$base | $addition'; - } -} - Object? _getAnnotationInstance(DeclarationMirror declaration, Type type) { for (var annotation in declaration.metadata) { if ((annotation.reflectee as Object).runtimeType == type) { @@ -315,17 +310,15 @@ class _Group { bool get hasSoloTest => tests.any((test) => test.isSolo); void addSkippedTest(String name, test_package.TestLocation? location) { - var fullName = _combineNames(this.name, name); - tests.add(_Test.skipped(isSolo, fullName, location)); + tests.add(_Test.skipped(isSolo, name, location)); } void addTest(bool isSolo, String name, MethodMirror memberMirror, _TestFunction function) { - var fullName = _combineNames(this.name, name); var timeout = _getAnnotationInstance(memberMirror, TestTimeout) as TestTimeout?; - tests.add(_Test(isSolo, fullName, function, timeout?._timeout, - memberMirror.testLocation)); + tests.add(_Test( + isSolo, name, function, timeout?._timeout, memberMirror.testLocation)); } } From 1840ae57f6f1246d0f1cede2d3d32840901404bb Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Thu, 5 Jun 2025 15:49:34 +0100 Subject: [PATCH 2/3] Fix missing group location --- pkgs/test_reflective_loader/lib/test_reflective_loader.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart index c65c10b61..621e67787 100644 --- a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart +++ b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart @@ -150,7 +150,7 @@ void _addTestsIfTopLevelSuite([String? name]) { void runTestsImpl() { for (var group in _currentGroups) { if (allGroups || group.isSolo) { - test_package.group(group.name, () { + test_package.group(group.name, location: group.location, () { for (var test in group.tests) { if (allTests || test.isSolo) { test_package.test(test.name, test.function, From b0bf6ac32bfae596b529177de3ff61ebdb2a17df Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 25 Jun 2025 15:55:10 +0100 Subject: [PATCH 3/3] Fix handling of nested suites with names + add tests + update changelog etc. --- pkgs/test_reflective_loader/CHANGELOG.md | 7 + .../lib/test_reflective_loader.dart | 277 +++++++++--------- pkgs/test_reflective_loader/pubspec.yaml | 2 +- .../test/hierarchy_test.dart | 34 +++ .../test/hierarchy_test.data.dart | 32 ++ .../test/location_test.dart | 2 +- 6 files changed, 214 insertions(+), 140 deletions(-) create mode 100644 pkgs/test_reflective_loader/test/hierarchy_test.dart create mode 100644 pkgs/test_reflective_loader/test/hierarchy_test.data.dart diff --git a/pkgs/test_reflective_loader/CHANGELOG.md b/pkgs/test_reflective_loader/CHANGELOG.md index 61ba81bde..a4270cf48 100644 --- a/pkgs/test_reflective_loader/CHANGELOG.md +++ b/pkgs/test_reflective_loader/CHANGELOG.md @@ -1,3 +1,10 @@ +## 0.4.0 + +- Test classes and suites with `name`s are now registered with `package:test`s + `group()` function to produce a hierarchy of groups/tests rather than a flat + set of tests with concatenated names. This may improve the display of tests + in IDEs test explorers. + ## 0.3.0 - Require Dart `^3.5.0`. diff --git a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart index 621e67787..65b3e1041 100644 --- a/pkgs/test_reflective_loader/lib/test_reflective_loader.dart +++ b/pkgs/test_reflective_loader/lib/test_reflective_loader.dart @@ -25,8 +25,12 @@ const Object skippedTest = SkippedTest(); /// A marker annotation used to annotate "solo" groups and tests. const Object soloTest = _SoloTest(); -final List<_Group> _currentGroups = <_Group>[]; -int _currentSuiteLevel = 0; +/// The current group stack of nested [defineReflectiveSuite] calls. +List<_Group> _currentGroupStack = []; + +/// The root groups or tests created by [defineReflectiveSuite] or +/// [defineReflectiveTests] calls. +List<_GroupEntry> _rootGroupEntries = []; /// Is `true` the application is running in the checked mode. final bool _isCheckedMode = () { @@ -43,13 +47,9 @@ final bool _isCheckedMode = () { /// create embedded suites. If the current suite is the top-level one, perform /// check for "solo" groups and tests, and run all or only "solo" items. void defineReflectiveSuite(void Function() define, {String? name}) { - _currentSuiteLevel++; - try { - define(); - } finally { - _currentSuiteLevel--; - } - _addTestsIfTopLevelSuite(name); + _addGroup(_Group(name), define); + + _addTestsIfTopLevelSuite(); } /// Runs test methods existing in the given [type]. @@ -79,107 +79,90 @@ void defineReflectiveTests(Type type) { 'in order to be run by runReflectiveTests.'); } - _Group group; - { - var isSolo = _hasAnnotationInstance(classMirror, soloTest); - var className = MirrorSystem.getName(classMirror.simpleName); - group = _Group(isSolo, className, classMirror.testLocation); - _currentGroups.add(group); - } + var isSolo = _hasAnnotationInstance(classMirror, soloTest); + var className = MirrorSystem.getName(classMirror.simpleName); - classMirror.instanceMembers - .forEach((Symbol symbol, MethodMirror memberMirror) { - // we need only methods - if (!memberMirror.isRegularMethod) { - return; - } - // prepare information about the method - var memberName = MirrorSystem.getName(symbol); - var isSolo = memberName.startsWith('solo_') || - _hasAnnotationInstance(memberMirror, soloTest); - // test_ - if (memberName.startsWith('test_')) { - if (_hasSkippedTestAnnotation(memberMirror)) { - group.addSkippedTest(memberName, memberMirror.testLocation); - } else { - group.addTest(isSolo, memberName, memberMirror, () { - if (_hasFailingTestAnnotation(memberMirror) || - _isCheckedMode && _hasAssertFailingTestAnnotation(memberMirror)) { - return _runFailingTest(classMirror, symbol); - } else { - return _runTest(classMirror, symbol); - } - }); + _addGroup(_Group(className, solo: isSolo, location: classMirror.testLocation), + () { + classMirror.instanceMembers + .forEach((Symbol symbol, MethodMirror memberMirror) { + // we need only methods + if (!memberMirror.isRegularMethod) { + return; } - return; - } - // solo_test_ - if (memberName.startsWith('solo_test_')) { - group.addTest(true, memberName, memberMirror, () { - return _runTest(classMirror, symbol); - }); - } - // fail_test_ - if (memberName.startsWith('fail_')) { - group.addTest(isSolo, memberName, memberMirror, () { - return _runFailingTest(classMirror, symbol); - }); - } - // solo_fail_test_ - if (memberName.startsWith('solo_fail_')) { - group.addTest(true, memberName, memberMirror, () { - return _runFailingTest(classMirror, symbol); - }); - } - // skip_test_ - if (memberName.startsWith('skip_test_')) { - group.addSkippedTest(memberName, memberMirror.testLocation); - } + // prepare information about the method + var memberName = MirrorSystem.getName(symbol); + var isTest = memberName.startsWith(RegExp('(solo_|fail_|skip_)*test_')); + if (isTest) { + var isSolo = memberName.startsWith('solo_') || + _hasAnnotationInstance(memberMirror, soloTest); + var isSkipped = memberName.startsWith('skip_') || + _hasSkippedTestAnnotation(memberMirror); + var expectFail = memberName.startsWith('fail_') || + memberName.startsWith('solo_fail_') || + _hasFailingTestAnnotation(memberMirror) || + _isCheckedMode && _hasAssertFailingTestAnnotation(memberMirror); + var timeout = + _getAnnotationInstance(memberMirror, TestTimeout) as TestTimeout?; + + _addTest( + _Test( + memberName, + timeout: timeout?._timeout, + location: memberMirror.testLocation, + solo: isSolo, + skip: isSkipped, + () => expectFail + ? _runFailingTest(classMirror, symbol) + : _runTest(classMirror, symbol)), + ); + } + }); }); - // Support for the case of missing enclosing [defineReflectiveSuite]. _addTestsIfTopLevelSuite(); } -/// If the current suite is the top-level one, add tests to the `test` package. -void _addTestsIfTopLevelSuite([String? name]) { - if (_currentSuiteLevel == 0) { - void runTests({required bool allGroups, required bool allTests}) { - /// A helper to run the tests that may optionally be wrapped in a group - /// if defineReflectiveSuite was given an explicit name. - void runTestsImpl() { - for (var group in _currentGroups) { - if (allGroups || group.isSolo) { - test_package.group(group.name, location: group.location, () { - 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); - } - } - }); +/// If we're back at the top level ([_currentGroupStack] is empty), registers +/// all known groups and tests by calling [test_package.group] and +/// [test_package.test] appropriately. +void _addTestsIfTopLevelSuite() { + if (_currentGroupStack.isNotEmpty) return; + + void addGroupsAndTests(List<_GroupEntry> entries) { + for (var entry in entries) { + switch (entry) { + case _Group group: + // Only add groups if they have names, otherwise just add their + // children directly. + if (group.name != null) { + test_package.group( + group.name, + location: group.location, + // ignore: deprecated_member_use, invalid_use_of_do_not_submit_member + solo: group.solo, + () => addGroupsAndTests(group.children), + ); + } else { + addGroupsAndTests(group.children); } - } + break; + case _Test test: + test_package.test( + test.name, + timeout: test.timeout, + location: test.location, + // ignore: deprecated_member_use, invalid_use_of_do_not_submit_member + solo: test.solo, + skip: test.skip, + test.function); + break; } - - if (name != null) { - test_package.group(name, runTestsImpl); - } else { - runTestsImpl(); - } - } - - if (_currentGroups.any((g) => g.hasSoloTest)) { - runTests(allGroups: true, allTests: false); - } else if (_currentGroups.any((g) => g.isSolo)) { - runTests(allGroups: false, allTests: true); - } else { - runTests(allGroups: true, allTests: true); } - _currentGroups.clear(); } + + addGroupsAndTests(_rootGroupEntries); + _rootGroupEntries.clear(); } Object? _getAnnotationInstance(DeclarationMirror declaration, Type type) { @@ -221,6 +204,28 @@ Future _invokeSymbolIfExists( return Future.value(invocationResult); } +/// Adds a group to the current stack and executes [define] for child group +/// or tests definitions. +void _addGroup(_Group group, void Function() define) { + var parentCollection = + _currentGroupStack.lastOrNull?.children ?? _rootGroupEntries; + parentCollection.add(group); + _currentGroupStack.add(group); + try { + define(); + } finally { + _currentGroupStack.removeLast(); + } +} + +/// Adds a test to the current group (or as a root test if there is no current +/// group). +void _addTest(_Test test) { + var parentCollection = + _currentGroupStack.lastOrNull?.children ?? _rootGroupEntries; + parentCollection.add(test); +} + /// Run a test that is expected to fail, and confirm that it fails. /// /// This properly handles the following cases: @@ -262,8 +267,6 @@ Future _runTest(ClassMirror classMirror, Symbol symbol) async { } } -typedef _TestFunction = dynamic Function(); - /// A marker annotation used to annotate test methods which are expected to /// fail. class FailingTest { @@ -298,30 +301,6 @@ class _AssertFailingTest { const _AssertFailingTest(); } -/// Information about a type based test group. -class _Group { - final bool isSolo; - final String name; - final test_package.TestLocation? location; - final List<_Test> tests = <_Test>[]; - - _Group(this.isSolo, this.name, this.location); - - bool get hasSoloTest => tests.any((test) => test.isSolo); - - void addSkippedTest(String name, test_package.TestLocation? location) { - tests.add(_Test.skipped(isSolo, name, location)); - } - - void addTest(bool isSolo, String name, MethodMirror memberMirror, - _TestFunction function) { - var timeout = - _getAnnotationInstance(memberMirror, TestTimeout) as TestTimeout?; - tests.add(_Test( - isSolo, name, function, timeout?._timeout, memberMirror.testLocation)); - } -} - /// A marker annotation used to instruct dart2js to keep reflection information /// for the annotated classes. class _ReflectiveTest { @@ -333,23 +312,45 @@ class _SoloTest { const _SoloTest(); } -/// Information about a test. -class _Test { - final bool isSolo; - final String name; - final _TestFunction function; - final test_package.Timeout? timeout; +abstract class _GroupEntry { + final String? name; final test_package.TestLocation? location; + final bool solo; - final bool isSkipped; + _GroupEntry( + this.name, { + this.location, + this.solo = false, + }); +} - _Test(this.isSolo, this.name, this.function, this.timeout, this.location) - : isSkipped = false; +/// Information about a test group which could be from a call to +/// [defineReflectiveSuite] with a `name`, or a test class itself. +class _Group extends _GroupEntry { + final List<_GroupEntry> children = []; - _Test.skipped(this.isSolo, this.name, this.location) - : isSkipped = true, - function = (() {}), - timeout = null; + _Group( + super.name, { + super.location, + super.solo, + }); +} + +/// Information about a test created for a method of a class with +/// [defineReflectiveTests]. +class _Test extends _GroupEntry { + final FutureOr? Function() function; + final bool skip; + final test_package.Timeout? timeout; + + _Test( + super.name, + this.function, { + required super.location, + required super.solo, + required this.skip, + required this.timeout, + }); } extension on DeclarationMirror { diff --git a/pkgs/test_reflective_loader/pubspec.yaml b/pkgs/test_reflective_loader/pubspec.yaml index 262a3498f..efdec89d1 100644 --- a/pkgs/test_reflective_loader/pubspec.yaml +++ b/pkgs/test_reflective_loader/pubspec.yaml @@ -1,5 +1,5 @@ name: test_reflective_loader -version: 0.3.0 +version: 0.4.0 description: Support for discovering tests and test suites using reflection. repository: https://github.com/dart-lang/tools/tree/main/pkgs/test_reflective_loader issue_tracker: https://github.com/dart-lang/tools/labels/package%3Atest_reflective_loader diff --git a/pkgs/test_reflective_loader/test/hierarchy_test.dart b/pkgs/test_reflective_loader/test/hierarchy_test.dart new file mode 100644 index 000000000..c052dd474 --- /dev/null +++ b/pkgs/test_reflective_loader/test/hierarchy_test.dart @@ -0,0 +1,34 @@ +// Copyright (c) 2025, 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. + +import 'dart:io'; +import 'dart:isolate'; + +import 'package:path/path.dart' as path; +import 'package:test/test.dart'; + +void main() { + test('builds the correct hierarchy of group names / test names', () async { + var testPackagePath = (await Isolate.resolvePackageUri( + Uri.parse('package:test_reflective_loader/')))! + .toFilePath(); + var testFilePath = path.normalize( + path.join(testPackagePath, '..', 'test', 'hierarchy_test.data.dart')); + var result = + await Process.run(Platform.resolvedExecutable, ['test', testFilePath]); + + var error = result.stderr.toString().trim(); + var output = result.stdout.toString().trim(); + + expect(error, isEmpty); + expect( + output, + allOf([ + contains('+0: SimpleTest test_foo'), + contains('+1: level_1.1 level_2.1 SimpleTest test_foo'), + contains('+2: level_1.1 level_2.2 SimpleTest test_foo'), + contains('+3: All tests passed!'), + ])); + }); +} diff --git a/pkgs/test_reflective_loader/test/hierarchy_test.data.dart b/pkgs/test_reflective_loader/test/hierarchy_test.data.dart new file mode 100644 index 000000000..ae3d96791 --- /dev/null +++ b/pkgs/test_reflective_loader/test/hierarchy_test.data.dart @@ -0,0 +1,32 @@ +// Copyright (c) 2025, 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. + +import 'package:test/test.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +/// This file is not intended to be run directly, but is run by +/// `hierarchy_test.dart`. +void main() { + defineReflectiveTests(SimpleTest); // Ungrouped tests + defineReflectiveSuite(() { + defineReflectiveSuite(() { + defineReflectiveSuite( + () { + defineReflectiveTests(SimpleTest); + }, /* unnamed */ + ); + }, name: 'level_2.1'); + defineReflectiveSuite(() { + defineReflectiveTests(SimpleTest); + }, name: 'level_2.2'); + }, name: 'level_1.1'); +} + +@reflectiveTest +class SimpleTest { + // ignore: non_constant_identifier_names + void test_foo() { + expect(1, 1); + } +} diff --git a/pkgs/test_reflective_loader/test/location_test.dart b/pkgs/test_reflective_loader/test/location_test.dart index 14984bb83..b66b13eda 100644 --- a/pkgs/test_reflective_loader/test/location_test.dart +++ b/pkgs/test_reflective_loader/test/location_test.dart @@ -38,7 +38,7 @@ void main() { // Split just the method name from the combined test so we can search // the source code to ensure the locations match up. - name = name.split('|').last.trim(); + name = name.split(' ').last.trim(); // Expect locations for all remaining fields. var url = test['url'] as String;