feat: define solidity test sources in config #5870

galargh · 2024-10-28T18:39:51Z

Because this PR includes a bug fix, relevant tests have been included.
Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
I didn't do anything of this.

This PR:

adds a solidity key to the paths.tests user config (defaults to paths.sources.solidity value)
removes the dependency on the compile task from the test:solidity task
adds compilation of paths.tests.solidity to the test:solidity task

To compile a subset of sources for solidity tests, I use the build function from the build system programmatically (and prepare required inputs in the solidity test task action).

To make the artifact discovery work properly, I had to add contractArtifactsGenerated property to the CacheHitFileBuildResult. This is because given a compilation job, we need to be able to tell what artifacts were generated, even if that job was cached.

One alternative/extension considered was to define the solidity test paths per build profile. This would have allowed greater level of configuration. We decided not to proceed with that, at least for now.

vercel · 2024-10-28T18:39:55Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hardhat	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 13, 2025 11:13am

changeset-bot · 2024-10-28T18:39:55Z

⚠️ No Changeset found

Latest commit: 1277665

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

…compilation

v-next/hardhat/src/internal/builtin-plugins/solidity-test/task-action.ts

v-next/hardhat/src/internal/builtin-plugins/solidity-test/helpers.ts

alcuadrado · 2025-01-03T15:29:10Z

v-next/hardhat/test/internal/core/hook-manager.ts

@@ -257,8 +257,9 @@ describe("HookManager", () => {
                      _context: HookContext,
                      givenHre: HardhatRuntimeEnvironment,
                    ): Promise<void> => {
-                      givenHre.config.paths.tests =
-                        "./test-folder-from-plugin1";
+                      givenHre.config.paths.tests = {


This is a bit confusing, maybe not needed.

givenHre.config.paths.tests is of type TestPathsConfig which is an empty interface to begin with. In this PR, it gets extended by a built-in solidity-test plugin with a solidity key. After this, ts starts flagging this assignment as an issue so we can either use something it knows about here or ignore the error.

Oh, I see! the only reason this issue was hit before is because the other plugins that extend TestPathsConfig are non-builtin.

alcuadrado · 2025-01-03T15:29:25Z

We chatted about this on a call: we'd treat solidity files as solidity test file if they either end in .t.sol, or they are inside config.paths.tests.

(defaults to paths.sources.solidity value)

Let's default to test/.

galargh

As discussed in a call and on Slack, I made the semantics of paths.tests.solidity equal to that of mocha and nodeTest.

I also removed the noCompile flag from the solidity test task, moved the common helpers from the test plugin to solidity one, and, most importantly, changed the way in which the tests are discovered (.t.sol from solidity sources and all .sol files from solidity test sources).

galargh · 2025-01-07T13:37:46Z

v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/build-system.ts

+          getAllFilesMatching(
+            dir,
+            (f) => f.endsWith(".sol") && !f.endsWith(".t.sol"),
+          ),


I'm saying here that we should NOT compile .t.sol files present in solidity sources in the default compile task.

galargh · 2025-01-07T13:41:03Z

v-next/hardhat/test/internal/core/hook-manager.ts

@@ -257,8 +257,9 @@ describe("HookManager", () => {
                      _context: HookContext,
                      givenHre: HardhatRuntimeEnvironment,
                    ): Promise<void> => {
-                      givenHre.config.paths.tests =
-                        "./test-folder-from-plugin1";
+                      givenHre.config.paths.tests = {


givenHre.config.paths.tests is of type TestPathsConfig which is an empty interface to begin with. In this PR, it gets extended by a built-in solidity-test plugin with a solidity key. After this, ts starts flagging this assignment as an issue so we can either use something it knows about here or ignore the error.

galargh · 2025-01-07T13:49:22Z

v-next/hardhat/src/internal/builtin-plugins/solidity-test/task-action.ts

-  { timeout, noCompile },
+  { timeout, force, quiet },


I decided to remove the noCompile flag from here until we figure out the right semantics for it.

I also added force and quiet flags as they are present in the compile task and make sense here as well.

alcuadrado · 2025-01-12T20:13:36Z

v-next/hardhat/src/internal/builtin-plugins/solidity-test/index.ts

      .addFlag({
-        name: "noCompile",
-        description: "Don't compile the project before running the tests",
+        name: "force",
+        description: "Force compilation even if no files have changed",
+      })
+      .addFlag({
+        name: "quiet",
+        description: "Makes the compilation process less verbose",
      })


Agreed with removing noCompile.

Now, I'm not sure if these new flags are valuable tbh. And with this names they are definitely confusing, as they are flags to the compilation process, but belong to a test task. I think we should do the same and remove them, at least for now.

OK, no worries. I'll remove them for now.

I think removing noCompile broke the main test task, which always sets that param to true for all test subtasks.

Ay! You're right! Sorry about that. I'm on it!

alcuadrado · 2025-01-12T20:14:47Z

v-next/hardhat/src/internal/builtin-plugins/solidity-test/task-action.ts

+    mergeCompilationJobs: shouldMergeCompilationJobs(
+      hre.globalOptions.buildProfile,
+    ),
+    quiet,


quiet should always be true

alcuadrado · 2025-01-12T20:14:53Z

v-next/hardhat/src/internal/builtin-plugins/solidity-test/task-action.ts

+  ).flat(1);
+
+  const buildOptions: BuildOptions = {
+    force,


force should always be false.

alcuadrado · 2025-01-12T20:16:12Z

v-next/hardhat/src/internal/builtin-plugins/solidity-test/task-action.ts

-    return;
-  }
+  // NOTE: We compile all the sources together with the tests
+  const rootFilePaths = [...rootSourceFilePaths, ...rootTestFilePaths];


No need to compile non-test files, the test files include any necessary non-test file as a dependency already. They aren't acting as root in this usecase.

I would have sworn that didn't work for me before, but I cannot replicate what I vaguely remember now so I'll remove the rootSourceFilePaths compilation.

alcuadrado

Looking good. I left two small comments. Feel about the root files and the task's flags. Feel free to merge after approaching them.

…compilation

github-actions bot added the status:ready This issue is ready to be worked on label Oct 28, 2024

alcuadrado added the v-next A Hardhat v3 development task label Oct 28, 2024

feat: define solidity test sources in config

6dea780

galargh force-pushed the feat/solidity-test-compilation branch from b0bdb71 to 8d671af Compare November 6, 2024 11:29

vercel bot deployed to Preview November 6, 2024 11:30 View deployment

galargh force-pushed the feat/solidity-test-compilation branch 2 times, most recently from 980d558 to 9721ff4 Compare November 7, 2024 00:14

vercel bot deployed to Preview November 7, 2024 00:15 View deployment

galargh force-pushed the feat/solidity-test-compilation branch from 9721ff4 to c6e7ae9 Compare November 7, 2024 00:47

vercel bot deployed to Preview November 7, 2024 00:48 View deployment

galargh force-pushed the feat/solidity-test-compilation branch from c6e7ae9 to c03c9f7 Compare November 7, 2024 00:50

vercel bot deployed to Preview November 7, 2024 00:51 View deployment

galargh force-pushed the feat/solidity-test-compilation branch from c03c9f7 to c829cb3 Compare November 7, 2024 00:51

vercel bot deployed to Preview November 7, 2024 00:52 View deployment

galargh force-pushed the feat/solidity-test-compilation branch from c829cb3 to 4958af2 Compare November 7, 2024 00:53

vercel bot deployed to Preview November 7, 2024 00:54 View deployment

galargh force-pushed the feat/solidity-test-compilation branch from 4958af2 to 3ce0176 Compare November 7, 2024 01:08

feat: compile solidity test sources only

0ebcfbf

vercel bot had a problem deploying to Preview November 7, 2024 01:09 Failure

galargh force-pushed the feat/solidity-test-compilation branch from 3ce0176 to 0ebcfbf Compare November 7, 2024 01:09

vercel bot deployed to Preview November 7, 2024 01:10 View deployment

galargh added 2 commits December 4, 2024 12:17

docs: add inline comments to solidity test helpers functions

63e0215

Merge remote-tracking branch 'origin/v-next' into feat/solidity-test-…

4fca1da

…compilation

vercel bot deployed to Preview December 4, 2024 11:18 View deployment

fix: ran pnpm prettier --write to fix linting error

a4a5b61

galargh changed the title ~~[WIP] feat: define solidity test sources in config~~ feat: define solidity test sources in config Dec 4, 2024

galargh requested a review from alcuadrado December 4, 2024 11:24

galargh marked this pull request as ready for review December 4, 2024 11:24

alcuadrado reviewed Jan 3, 2025

View reviewed changes

v-next/hardhat/src/internal/builtin-plugins/solidity-test/task-action.ts Outdated Show resolved Hide resolved

alcuadrado reviewed Jan 3, 2025

View reviewed changes

v-next/hardhat/src/internal/builtin-plugins/solidity-test/helpers.ts Outdated Show resolved Hide resolved

alcuadrado reviewed Jan 3, 2025

View reviewed changes

Merge branch 'v-next' into feat/solidity-test-compilation

67daa57

vercel bot deployed to Preview January 3, 2025 22:08 View deployment

chore: restore the semantics of paths.tests.solidity

18dca31

vercel bot deployed to Preview January 3, 2025 23:20 View deployment

chore: move common helpers from solidity-test to solidity plugin

c882e69

vercel bot deployed to Preview January 7, 2025 13:30 View deployment

feat: add force and quiet flags to solidity test task

f222cd9

vercel bot deployed to Preview January 7, 2025 13:49 View deployment

galargh commented Jan 7, 2025

View reviewed changes

galargh requested a review from alcuadrado January 7, 2025 13:55

alcuadrado reviewed Jan 12, 2025

View reviewed changes

alcuadrado approved these changes Jan 12, 2025

View reviewed changes

galargh added 2 commits January 13, 2025 10:56

Merge remote-tracking branch 'origin/v-next' into feat/solidity-test-…

5314589

…compilation

chore: apply review comments

9a32725

vercel bot deployed to Preview January 13, 2025 10:23 View deployment

chore: fix eslint

1277665

galargh enabled auto-merge January 13, 2025 11:13

vercel bot deployed to Preview January 13, 2025 11:13 View deployment

galargh added this pull request to the merge queue Jan 13, 2025

Merged via the queue into v-next with commit 34e9feb Jan 13, 2025
55 checks passed

galargh deleted the feat/solidity-test-compilation branch January 13, 2025 11:28

galargh mentioned this pull request Jan 14, 2025

chore: check test task options before passing on noCompile flag #6143

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: define solidity test sources in config #5870

feat: define solidity test sources in config #5870

galargh commented Oct 28, 2024 •

edited

Loading

vercel bot commented Oct 28, 2024 •

edited

Loading

changeset-bot bot commented Oct 28, 2024 •

edited

Loading

alcuadrado Jan 3, 2025

galargh Jan 7, 2025

alcuadrado Jan 12, 2025

alcuadrado commented Jan 3, 2025

galargh left a comment

galargh Jan 7, 2025

alcuadrado Jan 12, 2025

galargh Jan 7, 2025

galargh Jan 7, 2025

alcuadrado Jan 12, 2025

galargh Jan 13, 2025

fvictorio Jan 14, 2025

galargh Jan 14, 2025

alcuadrado Jan 12, 2025

alcuadrado Jan 12, 2025 •

edited

Loading

alcuadrado Jan 12, 2025 •

edited

Loading

galargh Jan 13, 2025

alcuadrado left a comment

feat: define solidity test sources in config #5870

feat: define solidity test sources in config #5870

Conversation

galargh commented Oct 28, 2024 • edited Loading

vercel bot commented Oct 28, 2024 • edited Loading

changeset-bot bot commented Oct 28, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcuadrado commented Jan 3, 2025

galargh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcuadrado Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

alcuadrado Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcuadrado left a comment

Choose a reason for hiding this comment

galargh commented Oct 28, 2024 •

edited

Loading

vercel bot commented Oct 28, 2024 •

edited

Loading

changeset-bot bot commented Oct 28, 2024 •

edited

Loading

alcuadrado Jan 12, 2025 •

edited

Loading

alcuadrado Jan 12, 2025 •

edited

Loading