New flag --shard-by-suite

[sigurdm]

Running all setupAll hooks is expensive when what we are trying to do is to minimize the time spent for each shard.

This allows choosing a less fine-grained strategy of sharding by individual test files.

Fixes #2614

[github-actions]

PR Health

Changelog Entry ✔️

Package	Changed Files

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

This check can be disabled by tagging the PR with skip-changelog-check.

[jakemac53]

Meta comment, but we call test "files" test "suites". So it might be best to make this --shard-by=suite. Although it may not be as intuitive for users, but it would match our general terminology.

I am curious what @natebosch thinks though.

[sigurdm]

Meta comment, but we call test "files" test "suites". So it might be best to make this --shard-by=suite. Although it may not be as intuitive for users, but it would match our general terminology.

I think as this is raw sharding by file, I like to keep the terminology there. Also I don't really see a reason for package:test to invent its own name for a file. I added a parenthetical explanation in the help-string and readme for people who know what a suite is, but don't know what a file is.

[jakemac53]

I think as this is raw sharding by file, I like to keep the terminology there. Also I don't really see a reason for package:test to invent its own name for a file. I added a parenthetical explanation in the help-string and readme for people who know what a suite is, but don't know what a file is.

I don't disagree with you but the decision was made a long time ago to call test files suites. I couldn't tell you why. This does leak out to users via --suite-load-timeout, but I don't see a lot of other places. cc @natebosch for thoughts on this

[natebosch]

Sorry for the slow response. I spent some time thinking through alternative sharding designs that can take into account at least some of the differences between test suites but I came up short. I think I lean towards landing something very close to this design. Some thoughts:

RE --shard-by-file vs --shard-by-suite

I think as this is raw sharding by file

With the "file" naming, as a user I might expect that a pattern like all_tests_test.dart which imports and runs the main of a bunch files with test cases would still shard based on which file the test() is actually written in. "Suite" might cause a reader to need to look for a definition of the term, but is unlikely to result in an incorrect interpretation like "file" might.

RE shard before or after reading top level @TestOn annotations:

If we do top-level @TestOn, tags, it seems to me asymetrical that we don't look at the single-test annotations

It feels quite natural to me that we would honor suite-level configuration when we are doing suite-level sharding. We should expect that test suites which are entirely configured for one platform would do that at the suite level instead of the test or group level

The fact that we do sorting by file path could make a situation where a big run of tests for a single platform end up in a single shard that ends up doing no testing because they were all filtered out. We should maybe consider doing a stable shuffle then sublists, or sort and round-robin distribute suites to shards so that similarly named tests are more likely to end up on different shards.

[natebosch]

[nit] This usage of "suites" is wrong - replace with maybe "packages" or "test invocations"?

[sigurdm]

Fixed.

[natebosch]

I think we should make top level test annotations work for this strategy.

I think the way we should talk about the test-case level filtering is a little bit more general. Something maybe like

Test filters which apply to individual test cases or groups (as opposed to suite level annotations at the top of a file) will be reflected in the distribution when sharding by test case, but not when sharding by test suite. Test invocations with uneven number of tests cases to run between test suites can cause uneven workloads across the shards.

[natebosch]

I had gemini take a try at handling the filtering before sharding. At a skim the solution it found looks fine to me - add a cache of the MetaData to the Loader so that we can read metadata earlier than we used to without doubling up. The Runner where the sharding is implemented already has a reference to the Loader so it's feasible to introduce suite filtering eagerly when using suite sharding.

[sigurdm]

Fair, I can try to give it a go next week to have top-level filtering before splitting.

[sigurdm]

Now suites are filtered by top-level annotations before being sharded.
The flag is now --shard-by-suite, and the phrasing has been updated everywhere.

[jakemac53]

LGTM from me

[natebosch]

[nit] I prefer being a bit more explicit than Metadat/ not Metadat. WDYT about using Result from package:async (even though we're only using it in a synchronous way)

Suggested change

	/// Cache of metadata parsed from test files.
	final _metadataCache = <String, Object>{};
	/// Parses and returns the suite metadata for [path], caching the result.
	Metadata parseSuiteMetadata(String path) {
	final cached = _metadataCache.putIfAbsent(path, () {
	if (!File(path).existsSync()) return Metadata.empty;
	try {
	return parseMetadata(
	path,
	File(path).readAsStringSync(),
	_runtimeVariables.toSet(),
	);
	} catch (e) {
	return e;
	}
	});
	if (cached is Metadata) return cached;
	throw cached;
	}
	/// Cache of metadata parsed from test files.
	final _metadataCache = <String, Result<Metadata>>{};
	/// Parses and returns the suite metadata for [path], caching the result.
	Metadata parseSuiteMetadata(String path) {
	final cached = _metadataCache.putIfAbsent(path, () {
	if (!File(path).existsSync()) return .value(Metadata.empty);
	try {
	return .value(
	parseMetadata(
	path,
	File(path).readAsStringSync(),
	_runtimeVariables.toSet(),
	),
	);
	} catch (e, stackTrace) {
	return .error(e, stackTrace);
	}
	});
	return switch (cached) {
	ValueResult(:var value) => value,
	ErrorResult(:var error, :var stackTrace) => Error.throwWithStackTrace(
	error,
	stackTrace,
	),
	_ => throw StateError('Unexpected result type: $cached'),
	};
	}

[natebosch]

I would have expected false. Is this a code path we'll never hit in practice? If this branch does get executed I think we should describe it in the doc.

[natebosch]

I think this is somewhat duplicative. I think we could pull out a List<SuitePlatform> _matchingPlatforms(SuiteConfiguration suiteConfig) method if the throw behavior is OK for both.

Would it cause any problem if we use the exception behavior here instead of silently ignoring unknown platforms? I think we wouldn't expect them here any more than we would when loading would we?