Add splitTestsCompilation solidity setting: main PR#8127
Conversation
🦋 Changeset detectedLatest commit: ff3109b The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Adds a standalone specification document describing the planned introduction of a new top-level Solidity config field, splitTestsCompilation, to control whether Solidity tests are compiled separately or together with contracts (defaulting to unified compilation).
Changes:
- Add
SPLIT_TESTS_COMPILATION_SPEC.mddocumenting intended behavior changes across Hardhat build system, tasks, cache, and related plugins. - Outline a phased implementation plan (config/plumbing → build system semantics → cache → tasks/runners/plugins → docs/migration).
…be already adapted.
…r not in the CompileCacheEntry interface
…th unified compilations
…-phase-6 Add `splitTestsCompilation` solidity setting (6): Solidity test runner updates
…-phase-7 Add `splitTestsCompilation` solidity setting (7): Typechain updates
…-phase-8 Add `splitTestsCompilation` solidity setting (8): Mocha updates
…-phase-9 Add `splitTestsCompilation` solidity setting (9): `node:test` updates
…-phase-10 Add `splitTestsCompilation` solidity setting (10): `hardhat-ignition` updates
Co-authored-by: Luis Schaab <schaable@gmail.com>
Co-authored-by: Luis Schaab <schaable@gmail.com>
…-phase-11 Add `splitTestsCompilation` solidity setting (11): closing PR
| (suiteResult) => { | ||
| stream.push({ | ||
| type: "suite:done", | ||
| data: suiteResult, | ||
| } satisfies TestEvent); | ||
| remainingSuites.delete( | ||
| formatArtifactId(suiteResult.id, sourceNameToUserSourceName), | ||
| ); | ||
| if (remainingSuites.size === 0) { | ||
| if (runCompleted) { | ||
| stream.push(null); | ||
| } | ||
| } | ||
| }, | ||
| ); | ||
| stream.push({ | ||
| type: "run:done", | ||
| data: solidityTestResult, | ||
| } satisfies TestEvent); | ||
| runCompleted = true; | ||
|
|
||
| controller.error( | ||
| new HardhatError( | ||
| HardhatError.ERRORS.CORE.SOLIDITY_TESTS.UNHANDLED_EDR_ERROR_SOLIDITY_TESTS, | ||
| { | ||
| error: error.message, | ||
| }, | ||
| ), | ||
| ); | ||
| if (remainingSuites.size === 0) { | ||
| stream.push(null); | ||
| } |
There was a problem hiding this comment.
stream.push(null) can be called from both the suite callback (when runCompleted is true) and after runSolidityTests resolves. If the EDR callback can fire after runSolidityTests resolves, this risks pushing EOF twice, which can trigger ERR_STREAM_PUSH_AFTER_EOF. Consider centralizing stream termination in a helper (e.g. endOnce()) or guarding with stream.readableEnded/stream.destroyed before pushing null.
| let scope = scopeBySource.get(sourceName); | ||
| if (scope === undefined) { | ||
| const fsPath = path.resolve(projectRoot, sourceName); | ||
|
|
||
| // npm files will be classified as "contracts" because their sourceName is | ||
| // not an existing file, and "contracts" is the default. | ||
| // | ||
| // If the package name clashed with | ||
| // ```ts | ||
| // path.relative( | ||
| // context.config.paths.root, | ||
| // context.config.paths.tests.solidity | ||
| // ) | ||
| // ``` | ||
| // | ||
| // They could be misclassified as test files. This is highly improbable, | ||
| // so we don't check it. You could read the artifact and see if the | ||
| // inputSourceName starts with `npm/` to rule this out. | ||
| scope = await context.solidity.getScope(fsPath); | ||
| scopeBySource.set(sourceName, scope); | ||
| } |
There was a problem hiding this comment.
In unified mode, getContractArtifactPaths resolves each artifact sourceName to path.resolve(projectRoot, sourceName) and then calls context.solidity.getScope(fsPath). For npm-dependency artifacts, sourceName is not an on-disk project file, and this can be misclassified as a test if the package name matches the configured Solidity tests directory (e.g. test/... with paths.tests.solidity = "test"), causing types to be skipped incorrectly. Consider treating non-existent paths as npm sources (defaulting to "contracts"), or detecting npm artifacts via artifact metadata (e.g. input/source name prefix) instead of relying on getScope for paths that don't exist.
| @@ -0,0 +1 @@ | |||
| // Empty file to trigger the full CI | |||
There was a problem hiding this comment.
The PR description says this PR only adds a spec file and isn't meant to be reviewed, but the diff includes substantial production and test changes across multiple packages (build system behavior, errors, plugin integration tests, fixture projects, etc.). Please update the PR description to reflect the actual scope (or split/retitle the PR) to avoid confusion during review and release notes.
| @@ -0,0 +1 @@ | |||
| // Empty file to trigger the full CI | |||
There was a problem hiding this comment.
This placeholder source file under src/internal/ looks like it’s only meant to force CI to run. Please remove it (or move the CI trigger to a non-shipping location) before merging, so it doesn’t accidentally get released/published.
| if (hre.config.solidity.splitTestsCompilation) { | ||
| if (noCompile !== true) { | ||
| await hre.tasks.getTask("build").run({ | ||
| noTests: true, | ||
| }); | ||
| } | ||
|
|
||
| // EDR needs all artifacts (contracts + tests) | ||
| const edrArtifactsWithMetadata: EdrArtifactWithMetadata[] = []; | ||
| const allBuildInfosAndOutputs: BuildInfoAndOutput[] = []; | ||
| for (const scope of ["contracts", "tests"] as const) { | ||
| const artifactsDir = await hre.solidity.getArtifactsDirectory(scope); | ||
| const artifactManager = new ArtifactManagerImplementation(artifactsDir); | ||
| edrArtifactsWithMetadata.push( | ||
| ...(await buildEdrArtifactsWithMetadata(artifactManager)), | ||
| ); | ||
| allBuildInfosAndOutputs.push( | ||
| ...(await getBuildInfosAndOutputs(artifactManager)), | ||
| ); | ||
| ({ testRootPaths: testRootPathsToRun } = await hre.tasks | ||
| .getTask("build") | ||
| .run({ | ||
| files: testFiles, | ||
| noContracts: true, | ||
| })); | ||
| console.log(); |
There was a problem hiding this comment.
In splitTestsCompilation mode, the --no-compile flag is only skipping the contracts build, but still always runs hre.tasks.getTask("build").run({ files: testFiles, noContracts: true }), which compiles the project despite the flag (the task option description says it should not compile the project). Consider skipping both build invocations when noCompile === true, and instead: (1) compute testRootPathsToRun from testFiles (resolved) or solidity.getRootFilePaths({ scope: "tests" }), (2) load artifacts from both scopes, and (3) validate that the selected test roots have compiled artifacts (similar to the unified-mode branch).
| // | ||
| // They could be misclassified as test files. This is highly improbable, | ||
| // so we don't check it. You could read the artifact and see if the | ||
| // inputSourceName starts with `npm/` to rule this out. |
There was a problem hiding this comment.
This comment refers to npm/ as the prefix for npm sources, but the Solidity build system root-path format uses the npm: prefix (e.g. npm:<package>/<file>). Updating the comment to match the actual prefix would avoid confusion for future maintainers.
| // inputSourceName starts with `npm/` to rule this out. | |
| // inputSourceName starts with `npm:` to rule this out. |
| "hardhat": minor | ||
| --- | ||
|
|
||
| Make the split of contracts and solidity tests compilation optional, and controlled with a new `splitTestsCompilation` config field. |
There was a problem hiding this comment.
PR description says this PR “only introduces a spec file” and is “not meant to be reviewed”, but this changeset (and the diff overall) indicates the feature is being implemented and released (new config field, error descriptors, build-system behavior, multiple tests/fixtures). Please update the PR description to match the actual scope of changes, or split out the spec-only PR if that’s still the intent.
Adds a
splitTestsCompilationSolidity setting that controls if tests are built independently or not.Docs PR: NomicFoundation/hardhat-website#255