Allow specifying an explicit location for test/groups

[DanTup]

Usually test locations (which are used both to report the location of tests in the json reporter, and also for filtering tests by line/col) are inferred from the call stack when test() is called.

This changes adds a new location parameter to group/test to allow this information to be provided explicitly. This can be useful where the call to test() doesn't contain the actual location of the declaration in the call stack (for example when using pkg:test_reflective_loader).

See #2029
See Dart-Code/Dart-Code#5480

[DanTup]

@jakemac53 this doesn't quite work yet - see comments on the test about filtering groups.

I'm also not sure if the API changes I've made are reasonable, so interested in feedback on that too.

[jakemac53]

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

[DanTup]

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?

[jakemac53]

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.

[DanTup]

@jakemac53 I think this all works as required now, but I'm unsure about:

• the public API changes (lots of them are in src, but I think many of these classes are exported from lib?)
• the mutable parent on GroupEntry
• whether this will affect how Flutter uses this (however once you're happy with it here, I can try updating in Flutter locally to verify before we commit)

[jakemac53]

This needs a pubspec/changelog update as well

[DanTup]

@jakemac53

This needs a pubspec/changelog update as well

I've added this to all three changelogs that this affected. I used the same description but let me know if these should be more specific (the changes only really work together, and I presume these are always shipped/updated together).

I've also tested with Flutter locally and this doesn't seem to affect anything there (at least, no analysis errors, and no failures from the tests I ran).

[jakemac53]

This is a new feature (new apis) so lets release it as such.

Suggested change

	## 1.25.16-wip
	## 1.26.0-wip

[DanTup]

hmm, I'm having a hard time making this work (although I'm bumping the minor part for all three packages - is that what you meant?)

By bumping these versions, the matcher constraint doesn't match. So I tried bumping that, but test depends back on matcher, so I end up with:

[test] dart pub get --no-example
Resolving dependencies...
Because matcher >=0.12.16+1 depends on test_api >=0.5.0 <0.8.0 and matcher >=0.12.16 <0.12.16+1 depends on test_api >=0.5.0 <0.7.0, matcher >=0.12.16 requires test_api >=0.5.0 <0.8.0.
So, because test depends on matcher >=0.12.16 <0.12.18 and test_workspace depends on test_api, version solving failed.
exit code 1

Do I need to also bump the major versions on matcher/checks and increase their constraints to match the new versions of everything else?

[jakemac53]

For test_api and test_core, since they are on the 0 major version - all the semantic versions are shifted to the right so they were actually fine (already feature releases). Only package:test needed to be updated

[DanTup]

Oh yeah!

I've done that, though I now see this (fairly unspecific) error:

[test] dart pub get --no-example
Resolving dependencies...
Because test_core depends on test_api 0.7.4 and test_workspace depends on test_api, version solving failed.
exit code 1

However, I'm also seeing that when I revert all of my changes - so I've pushed here so you can kick the bots while I figure out if this is just a me problem.

[DanTup]

ok, it was my change and it was because I bumped the test_api version (because the current version had already been published), but there were still references to the old one - see 888a9e8.

I'm not sure if having -wip on the end of those versions is ideal, but they're not yet published - let me know if there's something better I should do there.

[jakemac53]

That change lgtm

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

[DanTup]

@jakemac53 looks like the asserts I added caused a lot of failures. This is because in forPlatform() we create new groups, but we reuse the same child entries, so we try to reassign their parents to the new group:

test/pkgs/test_api/lib/src/backend/group.dart

Lines 61 to 67 in 3ac991f

	var filtered = _map((entry) => entry.forPlatform(platform));
	if (filtered.isEmpty && entries.isNotEmpty) return null;
	return Group(name, filtered,
	metadata: newMetadata,
	trace: trace,
	setUpAll: setUpAll,
	tearDownAll: tearDownAll);

I'm not sure how to handle this.. should we keep the parent pointing at the original group (not the new platform-scoped one), or do we need to clone the child entries?

[jakemac53]

Looksl ike some legit test failures in the non-null asserts @DanTup

[DanTup]

Oh, ignore me - we are cloning tests already, but not setup/teardown. I think it's an easy fix, I'll have a go :)

[DanTup]

@jakemac53 could you check whether 990f13f seems reasonable (and if so, kick the bots off again)?

Edit: hang on, I think there are other issues - lemme take another look.

[DanTup]

@jakemac53 I think some of this might be test issues, but I'm not sure how to handle it. For example this test here:

test/pkgs/test/test/runner/engine_test.dart

Lines 16 to 32 in 3ac991f

	test('runs each test in each suite in order', () async {
	var testsRun = 0;
	var tests = declare(() {
	for (var i = 0; i < 4; i++) {
	test(
	'test ${i + 1}',
	expectAsync0(() {
	expect(testsRun, equals(i));
	testsRun++;
	}, max: 1));
	}
	});
	var engine = Engine.withSuites([
	runnerSuite(Group.root(tests.take(2))),
	runnerSuite(Group.root(tests.skip(2)))
	]);

It creates some tests with declare(), which puts them into a root group (with an empty name). Then it strips those tests out and passed them to a new Group.root() to run subsets of them. The new Group.root() constructor tries to "own" these tests, but they already have a parent that was assigned during declaration.

So I don't think that's a bug, but rather just a consequence of how the test works. Cloning the tests here might work, but it also feels like a burden on tests (I think there are quite a lot that do similar).

[jakemac53]

Hmmm, I do think the asserts are generally helpful, bashing over a parent is never intended, so ideally the tests would be updated to copy tests before reparenting them.

We could possibly file a TODO for this, but it makes me a bit uneasy to remove the asserts.

[DanTup]

@jakemac53

it makes me a bit uneasy to remove the asserts

Yep, I agree - but is there something nicer we could do than changing all of these tests from something like:

runnerSuite(Group.root(tests.take(2))), 

to:

runnerSuite(Group.root(tests.take(2).map(_clone)), 

Maybe a version of runnerSuite that takes a list of tests, and creates the root group itself using clones of the tests?

runnerSuiteTests(tests.take(2)), 

[jakemac53]

Either way sounds fine to me

[DanTup]

@jakemac53 assuming I'm running them all correctly, I believe the tests are all passing now. I made two further changes:

1. Ensure Test.filter() always returns a clone (because when Group.filter() is called it returns a new group, and therefore must return new tests to have the parent wired up correctly)
2. Update some tests to clone the tests when they're taking them from the declared group and producing a new group subset for testing (I did this with an private .asRootGroup() for that test file)

If these look reasonable, please can you trigger the bots again? Thanks!