Skip to content

ci: run Hardhat unit tests in GitHub Actions - #370

Open
huklaa wants to merge 2 commits into
circlefin:mainfrom
huklaa:fix-369-hardhat-unit-ci
Open

ci: run Hardhat unit tests in GitHub Actions#370
huklaa wants to merge 2 commits into
circlefin:mainfrom
huklaa:fix-369-hardhat-unit-ci

Conversation

@huklaa

@huklaa huklaa commented Sep 8, 2026

Copy link
Copy Markdown

Summary
add a dedicated TypeScript Unit Tests CI job
install Node 22 and the repo-pinned Foundry version
run the existing make test-unit-hardhat target
intentionally skip contract submodule initialization and compilation because this unit suite does not require them
This ensures the existing Hardhat unit suite is exercised on pull requests instead of remaining outside CI.
Fixes #369

@osr21

osr21 commented Sep 8, 2026

Copy link
Copy Markdown

I filed #369, so I re-ran my reproduction against this patch. It is correct, and it matches the repo's existing CI conventions exactly. Notes below, including one subtlety I got wrong in my own first draft of the issue that is worth recording so it doesn't get "optimized" away later.

The Foundry step is load-bearing — please don't drop it

The non-obvious part of this patch is installing Foundry for a job that never compiles a contract. My initial draft of #369 proposed a Node-only job and asserted no Foundry was needed. That was wrong, and stripping forge from PATH is what proved it:

Couldn't run `forge`

hardhat.config.ts imports @nomicfoundation/hardhat-foundry, which shells out to forge during config resolution — before any test executes, and regardless of --no-compile. So the dependency is invisible from the Makefile target alone. This PR handles it, and reuses the exact Read Foundry versionInstall Foundry pair already used by the four contract jobs (ci.yml lines ~119-125, ~184-190, ~214-220, ~236-242), reading the pinned v1.4.4 from .foundry-version rather than hardcoding.

Worth a one-line comment in the workflow saying why Foundry is there, since a future reader will reasonably assume a TypeScript unit job doesn't need it.

Skipping submodule init is correct

The PR description calls this out and it holds up: I ran the suite with all three contracts/lib/* directories empty and no typechain-types/ present, and it passed. The four contract jobs need git submodule update --init; this one genuinely does not.

Convention checks

  • node-version: 22 matches both existing setup-node steps (lines ~110, ~175). No .nvmrc or engines field exists to contradict it.
  • foundry-rs/foundry-toolchain@v1 unpinned initially looked like a supply-chain regression next to the SHA-pinned checkout/setup-node, but all four existing Foundry installs use the same bare @v1, as do taiki-e/install-action@v2 and actions-rust-lang/setup-rust-toolchain@v1. This PR introduces no new inconsistency; partial pinning is a pre-existing repo-wide choice.
  • npm ci + cache: npm is satisfied — package-lock.json is present at root.
  • Step ordering is safe: package.json declares only compile/test/clean, with no postinstall or prepare hook, so npm ci running before the Foundry install cannot trigger a hardhat invocation while forge is still absent.

Expected result

When it runs, maintainers should see 37 passing across 4 fileshelpers/matchers/index.test.ts 15, unit/deployer-nonce.test.ts 14, unit/protocol-config-genesis.test.ts 4, unit/validator-manager-genesis-validation.test.ts 4 — in roughly 58ms of test time, a few seconds wall clock with a warm npm cache. (My local run used forge 1.1.0-dev rather than the pinned 1.4.4; the suite doesn't touch the compiler, but the CI number is the authoritative one.)

Two things this patch cannot fix

  1. This PR's own CI hasn't executed. Public CI for a87718a is sitting at action_required — the head is a fork (huklaa/arc-node, author_association: NONE), so a maintainer has to approve the run before the new job produces any evidence. Right now nobody has seen it go green.
  2. Branch protection is a separate, admin-only step. Adding the job to ci.yml makes it run; it does not make it block. Unless TypeScript Unit Tests is added to the required status checks, a future PR that breaks these 37 tests still merges cleanly, and ci: the hardhat unit test suite is never run — 37 tests across 4 files are inert #369 is only half-closed.

Non-blocking: the glob can silently exclude new tests

make test-unit-hardhat targets ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts. That covers all four files that exist today, but a new file at tests/helpers/** other than that one hardcoded path, or anything in a subdirectory under tests/unit/, joins the repo already excluded from CI — the exact failure mode #369 describes, just one level down. A recursive pattern would close that off. Out of scope for this PR, but it's the reason the gap existed in the first place.

For the record, tests/localdev/ (19 files) and tests/simulation/ (9 files) are correctly left alone here — they need a running node and belong with the separate gaps in #248/#247/#249.


Disclosure: I am external to Circle — an unaffiliated community contributor with no write access to this repository. This is advisory review only, not an approval, and the measurements above are from my own local runs and should be confirmed by CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: the hardhat unit test suite is never run — 37 tests across 4 files are inert

2 participants