Skip to content

Allow specifying an explicit location for test/groups #2481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 28, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions pkgs/test/test/runner/json_reporter_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,70 @@ void customTest(String name, dynamic Function() testFn) => test(name, testFn);
''',
});
});

test('groups and tests with custom locations', () {
return _expectReport('''
group('group 1 inferred', () {
setUpAll(() {});
test('test 1 inferred', () {});
tearDownAll(() {});
});
group('group 2 custom', location: TestLocation('file:///foo/group', 123, 234), () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these URIs should be package root relative? Or at least support that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added test cases for org-dartlang-app:/// for the filtering, although it's not clear to me what supporting org-dartlang-app here would mean for the JSON... I think the clients of the JSON reporters would always expect file paths? If we supported org-dartlang-app here, would they need to be translated to real file paths somewhere?

Copy link
Contributor

@jakemac53 jakemac53 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I do think it will mess up some of these use cases - one thing we can try externally is running tests with build_web_compilers (build_runner test -- -p chrome), and see what all it takes to get things working.

Ultimately none of this works today anyways, so its probably ok if we just leave it in a state where it still doesn't work, as long as internally package:test does something sensible and we can leave any extra bits for later.

setUpAll(location: TestLocation('file:///foo/setUpAll', 345, 456), () {});
test('test 2 custom', location: TestLocation('file:///foo/test', 567, 789), () {});
tearDownAll(location: TestLocation('file:///foo/tearDownAll', 890, 901), () {});
});
''', [
[
suiteJson(0),
testStartJson(1, 'loading test.dart', groupIDs: []),
testDoneJson(1, hidden: true),
],
[
groupJson(2, testCount: 2),
groupJson(3,
name: 'group 1 inferred',
parentID: 2,
line: 6,
column: 7,
testCount: 1),
testStartJson(4, 'group 1 inferred (setUpAll)',
groupIDs: [2, 3], line: 7, column: 9),
testDoneJson(4, hidden: true),
testStartJson(5, 'group 1 inferred test 1 inferred',
groupIDs: [2, 3], line: 8, column: 9),
testDoneJson(5),
testStartJson(6, 'group 1 inferred (tearDownAll)',
groupIDs: [2, 3], line: 9, column: 9),
testDoneJson(6, hidden: true),
groupJson(7,
name: 'group 2 custom',
parentID: 2,
url: 'file:///foo/group',
line: 123,
column: 234,
testCount: 1),
testStartJson(8, 'group 2 custom (setUpAll)',
url: 'file:///foo/setUpAll',
groupIDs: [2, 7],
line: 345,
column: 456),
testDoneJson(8, hidden: true),
testStartJson(9, 'group 2 custom test 2 custom',
url: 'file:///foo/test',
groupIDs: [2, 7],
line: 567,
column: 789),
testDoneJson(9),
testStartJson(10, 'group 2 custom (tearDownAll)',
url: 'file:///foo/tearDownAll',
groupIDs: [2, 7],
line: 890,
column: 901),
testDoneJson(10, hidden: true),
]
], doneJson());
});
});

test(
Expand Down
9 changes: 5 additions & 4 deletions pkgs/test/test/runner/json_reporter_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,16 @@ Map<String, Object> groupJson(int id,
int? parentID,
Object? skip,
int? testCount,
String? url,
int? line,
int? column}) {
if ((line == null) != (column == null)) {
throw ArgumentError(
'line and column must either both be null or both be passed');
}

url ??=
line == null ? null : p.toUri(p.join(d.sandbox, 'test.dart')).toString();
return {
'type': 'group',
'group': {
Expand All @@ -107,17 +110,15 @@ Map<String, Object> groupJson(int id,
'testCount': testCount ?? 1,
'line': line,
'column': column,
'url': line == null
? null
: p.toUri(p.join(d.sandbox, 'test.dart')).toString()
'url': url
}
};
}

/// Returns the event emitted by the JSON reporter indicating that a test has
/// begun running.
///
/// If [parentIDs] is passed, it's the IDs of groups containing this test. If
/// If [groupIDs] is passed, it's the IDs of groups containing this test. If
/// [skip] is `true`, the test is expected to be marked as skipped without a
/// reason. If it's a [String], the test is expected to be marked as skipped
/// with that reason.
Expand Down
78 changes: 77 additions & 1 deletion pkgs/test/test/runner/line_and_col_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,43 @@ void main() {
await test.shouldExit(0);
});

test('additionally selects test with matching custom location', () async {
var testFileUri = Uri.file(d.file('test.dart').io.path);
var notTestFileUri = Uri.file(d.file('not_test.dart').io.path);
await d.file('test.dart', '''
import 'package:test/test.dart';

void main() {
test("a", () {}); // Line 4 from stack trace

// Custom line 4 match
test("b", location: TestLocation('$testFileUri', 4, 0), () {});

// Custom different line, same file
test("c", location: TestLocation('$testFileUri', 5, 0), () => throw TestFailure("oh no"));

// Custom line 4 match but different file (no match)
test("d", location: TestLocation('$notTestFileUri', 4, 0), () => throw TestFailure("oh no"));
}
''').create();

var test = await runTest(['test.dart?line=4']);

expect(
test.stdout,
emitsThrough(contains('+2: All tests passed!')),
);

await test.shouldExit(0);
});

test('selects groups with a matching line', () async {
await d.file('test.dart', '''
import 'package:test/test.dart';

void main() {
group("a", () {
test("b", () {});
test("a", () {});
});
group("b", () {
test("b", () => throw TestFailure("oh no"));
Expand All @@ -80,6 +110,52 @@ void main() {
await test.shouldExit(0);
});

test('additionally selects groups with a matching custom location',
() async {
// TODO(dantup): This fails because we don't ever look at a groups
// location.. instead, we only look at tests. The reason we can
// normally run groups by line/col is because they just happen to be
// in the stack trace for the test() call too.
//
// So maybe we need to look up the tree to match location (in
// Runner._loadSuites() filter)?
var testFileUri = Uri.file(d.file('test.dart').io.path);
var notTestFileUri = Uri.file(d.file('not_test.dart').io.path);
await d.file('test.dart', '''
import 'package:test/test.dart';

void main() {
group("a", () { // Line 4 from stack trace
test("a", () {});
});

// Custom line 4 match
group("b", location: TestLocation('$testFileUri', 4, 0), () {
test("b", () {});
});

// Custom different line, same file
group("c", location: TestLocation('$testFileUri', 5, 0), () {
test("c", () => throw TestFailure("oh no"));
});

// Custom line 4 match but different file (no match)
group("d", location: TestLocation('$notTestFileUri', 4, 0), () {
test("d", () => throw TestFailure("oh no"));
});
}
''').create();

var test = await runTest(['test.dart?line=4']);

expect(
test.stdout,
emitsThrough(contains('+2: All tests passed!')),
);

await test.shouldExit(0);
});

test('No matching tests', () async {
await d.file('test.dart', '''
import 'package:test/test.dart';
Expand Down
1 change: 1 addition & 0 deletions pkgs/test_api/lib/backend.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ export 'src/backend/runtime.dart' show Runtime;
export 'src/backend/stack_trace_formatter.dart' show StackTraceFormatter;
export 'src/backend/stack_trace_mapper.dart' show StackTraceMapper;
export 'src/backend/suite_platform.dart' show SuitePlatform;
export 'src/backend/test_location.dart' show TestLocation;
1 change: 1 addition & 0 deletions pkgs/test_api/lib/scaffolding.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export 'src/backend/configuration/skip.dart' show Skip;
export 'src/backend/configuration/tags.dart' show Tags;
export 'src/backend/configuration/test_on.dart' show TestOn;
export 'src/backend/configuration/timeout.dart' show Timeout;
export 'src/backend/test_location.dart' show TestLocation;
export 'src/scaffolding/spawn_hybrid.dart' show spawnHybridCode, spawnHybridUri;
export 'src/scaffolding/test_structure.dart'
show addTearDown, group, setUp, setUpAll, tearDown, tearDownAll, test;
Expand Down
39 changes: 34 additions & 5 deletions pkgs/test_api/lib/src/backend/declarer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'group_entry.dart';
import 'invoker.dart';
import 'metadata.dart';
import 'test.dart';
import 'test_location.dart';

/// A class that manages the state of tests as they're declared.
///
Expand Down Expand Up @@ -44,6 +45,9 @@ class Declarer {
/// This is `null` for the root (implicit) group.
final Trace? _trace;

/// The optional location override for this group.
final TestLocation? _location;

/// Whether to collect stack traces for [GroupEntry]s.
final bool _collectTraces;

Expand All @@ -69,6 +73,9 @@ class Declarer {
/// [setUpAll] is always run and the rest are only run if that one succeeds.
Trace? _setUpAllTrace;

/// The optional location override for [setUpAll].
TestLocation? _setUpAllLocation;

/// The tear-down functions to run once for this group.
final _tearDownAlls = <void Function()>[];

Expand All @@ -78,6 +85,9 @@ class Declarer {
/// one trace. The first trace matches [_setUpAllTrace].
Trace? _tearDownAllTrace;

/// The optional location override for [tearDownAll].
TestLocation? _tearDownAllLocation;

/// The children of this group, either tests or sub-groups.
///
/// All modifications to this must go through [_addEntry].
Expand Down Expand Up @@ -157,6 +167,7 @@ class Declarer {
platformVariables ?? const UnmodifiableSetView.empty(),
collectTraces,
null,
null,
noRetry,
fullTestName,
allowDuplicateTestNames ? null : <String>{},
Expand All @@ -170,6 +181,7 @@ class Declarer {
this._platformVariables,
this._collectTraces,
this._trace,
this._location,
this._noRetry,
this._fullTestName,
this._seenNames,
Expand All @@ -189,6 +201,7 @@ class Declarer {
Object? skip,
Map<String, dynamic>? onPlatform,
Object? tags,
TestLocation? location,
int? retry,
bool solo = false}) {
_checkNotBuilt('test');
Expand Down Expand Up @@ -231,7 +244,10 @@ class Declarer {
// Make the declarer visible to running tests so that they'll throw
// useful errors when calling `test()` and `group()` within a test.
zoneValues: {#test.declarer: this});
}, trace: _collectTraces ? Trace.current(2) : null, guarded: false));
},
trace: _collectTraces ? Trace.current(2) : null,
location: location,
guarded: false));

if (solo) {
_soloEntries.add(_entries.last);
Expand All @@ -245,6 +261,7 @@ class Declarer {
Object? skip,
Map<String, dynamic>? onPlatform,
Object? tags,
TestLocation? location,
int? retry,
bool solo = false}) {
_checkNotBuilt('group');
Expand Down Expand Up @@ -272,6 +289,7 @@ class Declarer {
_platformVariables,
_collectTraces,
trace,
location,
_noRetry,
_fullTestName,
_seenNames,
Expand Down Expand Up @@ -307,16 +325,18 @@ class Declarer {
}

/// Registers a function to be run once before all tests.
void setUpAll(dynamic Function() callback) {
void setUpAll(dynamic Function() callback, {TestLocation? location}) {
_checkNotBuilt('setUpAll');
if (_collectTraces) _setUpAllTrace ??= Trace.current(2);
_setUpAllLocation ??= location;
_setUpAlls.add(callback);
}

/// Registers a function to be run once after all tests.
void tearDownAll(dynamic Function() callback) {
void tearDownAll(dynamic Function() callback, {TestLocation? location}) {
_checkNotBuilt('tearDownAll');
if (_collectTraces) _tearDownAllTrace ??= Trace.current(2);
_tearDownAllLocation ??= location;
_tearDownAlls.add(callback);
}

Expand Down Expand Up @@ -347,6 +367,7 @@ class Declarer {
return Group(_name ?? '', entries,
metadata: _metadata,
trace: _trace,
location: _location,
setUpAll: _setUpAll,
tearDownAll: _tearDownAll);
}
Expand Down Expand Up @@ -393,7 +414,11 @@ class Declarer {
// Make the declarer visible to running scaffolds so they can add to
// the declarer's `tearDownAll()` list.
zoneValues: {#test.declarer: this});
}, trace: _setUpAllTrace, guarded: false, isScaffoldAll: true);
},
trace: _setUpAllTrace,
location: _setUpAllLocation,
guarded: false,
isScaffoldAll: true);
}

/// Returns a [Test] that runs the callbacks in [_tearDownAll], or `null`.
Expand All @@ -408,7 +433,11 @@ class Declarer {
// Make the declarer visible to running scaffolds so they can add to
// the declarer's `tearDownAll()` list.
zoneValues: {#test.declarer: this});
}, trace: _tearDownAllTrace, guarded: false, isScaffoldAll: true);
},
trace: _tearDownAllTrace,
location: _tearDownAllLocation,
guarded: false,
isScaffoldAll: true);
}

void _addEntry(GroupEntry entry) {
Expand Down
Loading