Test Sharding #5075
Test Sharding #5075
Conversation
|
Marking as draft pending discussion in #4958. |
JoshuaKGoldberg
added
the
status: blocked
There was a problem hiding this comment.
Pull Request Overview
Adds file-based test sharding support, including a new CLI --shard option, parsing/validation utilities, and filtering of test files by shard.
Key changes:
- Introduce
parseShardStringandcreateInvalidShardErrorfor shard validation. - Extend
MochaAPI with.shard()and wire the option through CLI and file collection. - Add integration tests and a new JSON assertion for validating suite counts per shard.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/options/shard.spec.js | New integration test covering valid/invalid shard use |
| test/integration/fixtures/options/*.js | Fixture files defining suites for alpha, beta, theta |
| test/assertions.js | Add .to have suite count JSON assertion |
| lib/utils.js | Add parseShardString utility with error creation |
| lib/mocha.js | Add .shard() API and chain it into constructor |
| lib/errors.js | Define createInvalidShardError and INVALID_SHARD |
| lib/cli/run.js | Wire in --shard, validate mutual exclusion with parallel |
| lib/cli/run-option-metadata.js | Include shard in recognized option types |
| lib/cli/run-helpers.js | Add validateShardString and pass shard to runner |
| lib/cli/collect-files.js | Filter collected files by shard index |
Comments suppressed due to low confidence (3)
lib/mocha.js:175
- Duplicate JSDoc entry for
options.slow; remove one to avoid confusion.
* @param {number} [options.slow] - Slow threshold value.
lib/errors.js:106
- Typo in comment: change "A a shard value is invalid" to "A shard value is invalid" or similar.
* * A a shard value is invalid
lib/cli/run.js:235
- [nitpick] Consider adding
type: 'string'to theshardoption in yargs to ensure it’s parsed correctly as a string.
shard: {
| const desiredShard = parseInt(shardParts[0]); | ||
| const totalShards = parseInt(shardParts[1]); |
There was a problem hiding this comment.
Always specify a radix when using parseInt; e.g., use parseInt(shardParts[0], 10) and parseInt(shardParts[1], 10) to avoid unexpected behavior.
| const desiredShard = parseInt(shardParts[0]); | |
| const totalShards = parseInt(shardParts[1]); | |
| const desiredShard = parseInt(shardParts[0], 10); | |
| const totalShards = parseInt(shardParts[1], 10); |
Copilot uses AI. Check for mistakes.
| '--shard', | ||
| shard | ||
| ]; | ||
| // Test that come in through --file are always run first |
There was a problem hiding this comment.
[nitpick] Minor grammar: consider "Tests specified via --file are always run first" or "Test files coming in through --file are always run first".
| // Test that come in through --file are always run first | |
| // Tests specified via `--file` are always run first |
Copilot uses AI. Check for mistakes.
PR Checklist
status: accepting prsOverview
This strategy uses file-based sharding. The entire set of test files is divided into different groups or "shards". Each shard contains a subset of the test files. The shards use a round robin approach using a simple modulus calculation. The way this works in the code is it simply filters out the tests by the shard value before proceeding to the rest of the mocha code.
A shard option has been added to the command line interface for specifying which shard to run and how many shards there are in total. This value is validated to ensure that bad values aren't inputted.
Sharding does not work when parallel mode is enabled.