feat(test): support sharding tests across CI runners by peter-trost · Pull Request #1707 · VeryGoodOpenSource/very_good_cli

peter-trost · 2026-08-28T15:32:13Z

Description

Adds --shard-index and --total-shards to very_good test and very_good dart test, so a test suite can be split across multiple CI runners with a strategy.matrix.

strategy:
  matrix:
    shard: [1, 2, 3]
steps:
  - run: very_good test --shard-index ${{ matrix.shard }} --total-shards 3

Opening as a draft because @ryzizub is assigned to #1538 — happy to hand this over, close it, or adapt to a different design if work is already underway or a different approach is preferred.

Why this approach

The test optimizer already discovers every test file, so sharding is a partition of that list rather than new machinery.

Sorted, then round-robin. Directory.listSync order is filesystem dependent, so without sorting two runners could disagree on the partition and either skip or duplicate tests. Sorting first makes the partition deterministic across machines; dealing out round-robin keeps shards balanced by file count.
Non-optimized tests are sharded too. Files tagged skip_very_good_optimization run as standalone files alongside the optimizer entrypoint. Leaving them unsharded would re-run all of them on every runner, partly defeating the purpose.
No shard-specific filenames. The issue proposed generating .test_optimizer_1_of_3.dart. That turned out to be unnecessary — each runner generates its own .test_optimizer.dart containing only its slice, and the file is cleaned up afterwards anyway. Same result, no filename plumbing.
Empty shards succeed. A shard with no test files (more shards than test files) would otherwise fail with No tests were found, breaking builds on oversized matrices. It now reports success, matching the existing "no test folder" behaviour.

Interaction with coverage

--min-coverage is rejected with a usage error when sharding. Each shard only exercises a subset of the codebase, so its coverage is not representative of the whole suite and would fail the build spuriously. The error explains the alternative: collect per-shard coverage with --coverage, merge the lcov reports, and enforce the threshold once in a separate job.

Sharding also requires the optimizer, so it is rejected with --no-optimization and with --platform (which disables the optimizer). Both cases exit with ExitCode.usage and an actionable message.

This is the part I'd most like a maintainer opinion on — a documented merge-then-check workflow is the other reasonable option, and #804 may change what's possible here.

Testing

Hook unit tests (11 new): complete/disjoint partition, non-optimized sharding, determinism, balance, empty shards, and the pass-through case.
Command tests (15 new): all validation paths and shard pass-through, for both test and dart test.
Runner tests (2 new): shard vars reach the generator; empty shard short-circuits without invoking the test runner.
Manual end-to-end against a real package (7 optimizable + 2 tagged test files): verified all tests run exactly once across 3 and 12 shards, that shards are stable across repeated runs, and that the Flutter path shards correctly. Also exhaustively verified the partition is complete, disjoint, and balanced for suite sizes 0–40 across 1–8 shards.

dart analyze --fatal-infos --fatal-warnings . is clean, and the touched lib/ files are at 100% line coverage.

Note

The full suite passes with -j 1 (529 tests). At -j 8 a handful of unrelated tests fail due to a pre-existing working-directory race between test files — I confirmed the same failures on a clean main checkout, so it is not introduced here.

Closes #1538

🤖 Generated with Claude Code

Add `--shard-index` and `--total-shards` to `very_good test` and `very_good dart test`, so a test suite can be split across multiple CI runners with a `strategy.matrix`. The test optimizer already discovers every test file, so sharding is a partition of that list rather than new machinery. Files are sorted and dealt out round-robin, which keeps shards balanced by file count and makes the partition deterministic across machines — `Directory.listSync` order is filesystem dependent, so without sorting two runners could disagree and either skip or duplicate tests. Tests tagged `skip_very_good_optimization` are sharded as well. They run as standalone files alongside the optimizer entrypoint, so leaving them unsharded would re-run all of them on every runner. Each runner generates its own `.test_optimizer.dart` containing only its slice, so no shard-specific filenames are needed. A shard with no test files succeeds instead of failing with "No tests were found", so an oversized matrix does not break the build. Sharding is rejected with a usage error when combined with `--min-coverage`, since each shard only exercises a subset of the codebase and its coverage is not representative of the whole suite, and when the optimizer is disabled, which sharding depends on. Closes VeryGoodOpenSource#1538 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Normalize path separators in getNotOptimizedTests so tagged tests in subdirectories are excluded from the optimized set on Windows. Without this the relative paths compare unequal to the forward-slash normalized paths built in run(), causing those tests to be both inlined into the optimizer entrypoint and run standalone. Parse the shard values inside validateSharding instead of passing both the raw and parsed forms, and shorten the --min-coverage error. Also cover the shard that only contains non optimized tests, and document how sharding interacts with --recursive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

ryzizub · 2026-08-31T07:54:27Z

Thanks for the contribution @peter-trost ! ❤️ You can undraft it, since I actually havent started much on the sharding tests and this actually on first glance does it how i would do it.

I will give it a test and bigger review later today/tomorrow but so far it looks good to me

peter-trost · 2026-08-31T08:01:47Z

@ryzizub nice, glad to hear! Let me know if I can support in any way :)

peter-trost · 2026-08-31T10:18:29Z

the flame e2e failing seems unrelated 🤔

ryzizub · 2026-08-31T11:32:40Z

the flame e2e failing seems unrelated 🤔

Correct, there seems to be issue unrelated to this PR

Sharding the optimized and skip_very_good_optimization lists separately restarted the round-robin at shard 1 for each list, so the first shards received a file from both while later shards could stay empty. Deal out the single sorted list once and split it afterwards, which also drops one shardOf call. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

peter-trost · 2026-09-02T19:37:53Z

a505315 adds a fix to put optimized and skip_very_good_optimization files into one sorted list instead of two. Sharding them separately restarted at shard 1 for each list, so early shards got a file from both while later ones sat empty.

The bundle also embeds the hook test file, which gained a case in the previous commit after the bundle had last been generated, so the verify_bundle check failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

ryzizub

Overall LGTM, great job! Just a small findings

Route the raw --shard-index and --total-shards values through the test options classes instead of reading argResults in run(), make shardOf private now that the sharding behaviour is covered through run(), and name the flag values (<index>, <count>) while stating the default in their help text. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

peter-trost · 2026-09-03T09:32:31Z

Thanks for the review @ryzizub! Latest commit addresses the feedback.

# Conflicts: # lib/src/cli/dart_cli.dart # lib/src/cli/flutter_cli.dart # lib/src/cli/templates/test_optimizer_bundle.dart # lib/src/cli/test_cli_runner.dart # test/src/commands/dart/commands/dart_test_test.dart Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

peter-trost · 2026-09-07T19:29:31Z

@ryzizub merge conflicts fixed.

ryzizub · 2026-09-08T13:25:50Z

Adding @marcossevilla as additional reviewer for this PR

marcossevilla · 2026-09-08T13:56:27Z

+    final shardingError = validateSharding(
+      rawShardIndex: options.shardIndex,
+      rawTotalShards: options.totalShards,
+      optimizePerformance: options.optimizePerformance,


Blocking: this passes the raw --optimization flag, but the effective value at L564-570 is also switched off by --platform, --update-goldens, and an explicit test path.
So very_good test --platform chrome --shard-index 1 --total-shards 3 passes the check at L275-277, then runs with the optimizer off and every shard runs the whole suite.
Compute the effective value once above this call and pass it to both validateSharding and _flutterTest.

Fixed. The effective value is now computed once and shared.

marcossevilla · 2026-09-08T13:56:27Z

+    final shardingError = validateSharding(
+      rawShardIndex: options.shardIndex,
+      rawTotalShards: options.totalShards,
+      optimizePerformance: options.optimizePerformance,


Blocking: same as in test.dart. This is the raw flag, but L431-437 also switches it off for --platform and for explicit test paths.
very_good dart test --platform chrome --shard-index 1 --total-shards 4 then runs the full suite on all four shards instead of a quarter each.
Compute the effective value once and pass it to both.

Fixed. The effective value is now computed once and shared.

marcossevilla · 2026-09-08T13:56:27Z

+        // than test files. That is not a failure: the shard has nothing to do,
+        // so report success instead of letting the test runner exit with
+        // "No tests were found", which would fail the CI job.
+        if (shardIndex != null &&


Bug: L123-127 deletes coverage/lcov.info, and returning here skips the block that rewrites it, which ends by asserting the file exists (L266-267).
very_good test --coverage --shard-index 4 --total-shards 4 on a three-file suite then succeeds with no lcov file, so the merge step has nothing to read.
Write an empty lcov file before returning.

Fixed. An empty lcov.info is written.

marcossevilla · 2026-09-08T13:56:27Z

+  // Each shard only exercises a fraction of the codebase, so its coverage is
+  // not representative of the whole suite. Merge the lcov files from every
+  // shard and enforce the threshold in a separate job instead.
+  if (minCoverage != null) {


Bug: minCoverage is resolved from very_good.yaml (L56-60), so this rejects sharding in any repo that sets min_coverage:, even when the flag was never passed.
There is no --no-min-coverage to opt out, only --min-coverage="".
Check argResults.wasParsed('min-coverage') instead, the same way rawShardIndex and rawTotalShards stay strings so "not provided" can be detected.

Agreed on the problem. One tweak: wasParsed alone would let a config threshold through to per-shard enforcement and fail spuriously. So we reject only an explicit flag, and drop the config value while sharding.

marcossevilla · 2026-09-08T13:56:27Z

+/// Files are dealt out round-robin (index modulo [totalShards]) over the
+/// already sorted [paths], which keeps shards balanced in file count and makes
+/// the partition stable for a given test suite.
+List<String> _shardOf(


Bug: the loop below has no bounds checks. totalShards: 0 never advances i, so it appends paths[0] until memory runs out, and shardIndex: 0 reads paths[-1] and throws.
validateSharding blocks both from the CLI, but brick.yaml now prompts for these two vars, so mason make test_optimizer reaches this code directly.
Add a guard here, since this is the only place that can protect that caller.

marcossevilla · 2026-09-08T13:56:27Z

+    totalShards: totalShards,
+  );
+  final optimizedTestPaths = shardPaths
+      .where((p) => !notOptimizedTests.contains(p))


Nit: notOptimizedTests is a List, so each contains is a full scan, run once per path here and again on L73.
That is quadratic in the number of test files, on every run of the command.
Build a Set once before both where calls.

marcossevilla · 2026-09-08T13:56:27Z

+  /// equal to the ones built in [run] on Windows too.
  final relativePaths = testWithVeryGoodTest
-      .map((e) => path.relative(e, from: testDir))
+      .map((e) => path.relative(e, from: testDir).replaceAll(r'\', '/'))


Nit: this normalizes to forward slashes, but the consumer at test_cli_runner.dart L209 still calls p.join('test', e.toString()), so on Windows the argument becomes test\sub/foo_test.dart.
Windows accepts mixed separators, so it most likely works.
Use p.joinAll(p.posix.split(e)) there if you want the emitted path to stay native.

marcossevilla · 2026-09-08T13:56:27Z

 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 import 'package:very_good_cli/src/cli/cli.dart';
+import 'package:very_good_cli/src/commands/test/test.dart';


Nit: this import exists only to reach validateSharding, which sits next to FlutterTestOptions in the Flutter command.
CLAUDE.md rules out sibling dependencies in the same layer, and the function has no Flutter specifics.
Move it to lib/src/cli/, next to TestCLIRunner.

Validate sharding against the effective optimizer setting, so --platform, --update-goldens and explicit test paths are rejected instead of silently running the whole suite on every shard. Reject only an explicit --min-coverage and ignore a threshold inherited from very_good.yaml while sharding, since a single shard cannot meet it. Write an empty lcov.info for an empty shard so the merge step still finds a report per shard. Guard the hook against out of range shard values reached through `mason make`, look non optimized tests up in a set, keep the emitted test paths native, and move validateSharding next to TestCLIRunner so the dart command no longer depends on the flutter one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

peter-trost · 2026-09-08T19:47:57Z

Thanks for the thorough review @marcossevilla. Addressed all comments. 🙂

peter-trost and others added 2 commits August 28, 2026 17:31

ryzizub assigned peter-trost and ryzizub Aug 31, 2026

peter-trost marked this pull request as ready for review August 31, 2026 08:00

peter-trost requested a review from a team as a code owner August 31, 2026 08:00

peter-trost and others added 2 commits September 2, 2026 21:39

Merge branch 'main' into feat/test-sharding

f5b39fc

chore(test): regenerate test optimizer bundle

26357ed

The bundle also embeds the hook test file, which gained a case in the previous commit after the bundle had last been generated, so the verify_bundle check failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

ryzizub reviewed Sep 3, 2026

View reviewed changes

ryzizub requested a review from marcossevilla September 8, 2026 13:25

marcossevilla reviewed Sep 8, 2026

View reviewed changes

marcossevilla and others added 2 commits September 8, 2026 16:14

Merge branch 'main' into feat/test-sharding

1c52e95

Uh oh!

Conversation

peter-trost commented Aug 28, 2026

Description

Why this approach

Interaction with coverage

Testing

Uh oh!

ryzizub commented Aug 31, 2026

Uh oh!

peter-trost commented Aug 31, 2026

Uh oh!

peter-trost commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzizub commented Aug 31, 2026

Uh oh!

peter-trost commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzizub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peter-trost commented Sep 3, 2026

Uh oh!

peter-trost commented Sep 7, 2026

Uh oh!

ryzizub commented Sep 8, 2026

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcossevilla Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-trost commented Aug 31, 2026 •

edited

Loading

peter-trost commented Sep 2, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading

marcossevilla Sep 8, 2026 •

edited

Loading