ci: run orphaned Nethermind test projects in the test matrix - #12689
ci: run orphaned Nethermind test projects in the test matrix#12689benaadams wants to merge 5 commits into
Conversation
Seven Nethermind.* test projects were absent from every test workflow, so their tests never ran in CI: Analyzers, BalRecorder, CensorshipDetector.Plugin, OpcodeTracing.Plugin, Serialization.SszGenerator, State.Flat.History and StateDiffsWriter. Adds them to the nethermind-tests.yml matrix. Nethermind.OpcodeTracing.Plugin.Test was also missing from Nethermind.slnx (only the plugin itself was listed), so the shared test-artifact build did not produce its binaries; adds the project to the solution. Nethermind.Precompiles.Benchmark.Test is intentionally not wired in: it lives in Benchmarks.slnx only, and the test-artifact build compiles only Nethermind.slnx and EthereumTests.slnx.
|
Claude finished @benaadams's task in 6m 41s —— View job Review:
|
| Check | Result |
|---|---|
| Directory name == csproj name == matrix entry for all 7 | ✅ — required, since the job does working-directory: src/Nethermind/${{ matrix.project }} + dotnet restore ${{ matrix.project }}.csproj |
All 7 in Nethermind.slnx (so --no-build finds binaries) |
✅ — 6 already were; OpcodeTracing.Plugin.Test is the one this PR adds. build-test-artifacts.yml builds Nethermind.slnx + EthereumTests.slnx, so the artifact now covers all 7 |
All 7 import ../tests.props |
✅ — EnableNUnitRunner, Microsoft.Testing.Extensions.CodeCoverage, NUnit adapter — so both the plain and --coverage paths work |
| Matrix stays ASCII-alphabetical | ✅ — Serialization.Ssz.Test < Serialization.SszGenerator.Test, State.Flat.History.Test < State.Flat.Test, State.Test.Runner.Test < StateDiffsWriter.Test (. = 0x2E < D) |
.slnx placement follows convention |
✅ — plugin tests sit beside their plugin (matches /Plugins/CensorshipDetector/, /Plugins/StateDiffsWriter/); non-plugin ones were already under /Tests/ |
| No submodule dependency | ✅ — the tests job doesn't run fetch-submodules; none of the 7 read src/tests |
| No zero-test risk | ✅ — this matters: Ethereum.Blockchain.Pyspec.Test/CiSentinelTests.cs documents that MTP exits non-zero when zero tests run. None of the 7 has a CiRunnerGuard-style per-runner filter, [Explicit], or [Ignore], so all tests run on both x64 and arm64 |
| Flake surface on arm64 | ✅ — no Thread.Sleep / Task.Delay / DateTime.Now / RuntimeInformation / hardcoded paths / TestBlockchain / SemaphoreSlim anywhere in the 7. These are fast unit-test projects |
| Coverage exclusions still hold | ✅ — codecoverage.json's (Ethereum|Nethermind)\..*\.Test(\.Runner)?\.dll$ exclude matches all 7 new assemblies, and ExcludeAssembliesWithoutSources: MissingAll keeps Roslyn out of Analyzers.Test's instrumentation |
| Completeness of "these are all the orphans" | ✅ — the only unwired Nethermind.* test directory left is Nethermind.Precompiles.Benchmark.Test, which the PR body acknowledges. Nethermind.IntegrationTests looks like a gap but is run by run-e2e-tests.yml:68 |
The Benchmarks.slnx-only rationale for excluding Precompiles.Benchmark.Test is sound — pulling a BenchmarkDotNet project into Nethermind.slnx would put it in build-solutions, code-lint, and the artifact build for one test.
Low
1. Nethermind.OpcodeTracing.Plugin.Test enters code-lint for the first time. Left inline — this project has never been compiled in CI, and code-lint.yml builds the slnx with EnforceCodeStyleInBuild=true, failing on any warning IDE\d+/CA\d+ (20 rules are promoted to warning in .editorconfig, IDE0005 unused-using being the likeliest to bite). --locked-mode in build-solutions.yml is fine (no lock file in this project's graph). Nothing to change — just needs Code Lint and Build solutions green before merge. Both were still in progress when I looked.
2. No guard against the next orphan. This PR fixes the seven instances; the class stays open. nethermind-tests-checked.yml already drifts independently of this PR — it runs 44 projects and is missing Nethermind.EraE.Test and Nethermind.State.Flat.Test, which have been in nethermind-tests.yml all along, and is now behind by 9. (nethermind-tests-flat.yml is a deliberate 5-project subset, so leaving it alone is correct.) A CI step that diffs ls -d src/Nethermind/Nethermind.*.Test against the matrix, with an explicit allowlist for the intentional exclusion, would make the next orphan fail CI instead of silently never running — the natural companion to a test-hygiene series. Fix this →
3. Ethereum.Ssz.Test is the same defect, still unwired. It's in EthereumTests.slnx (so the artifact job builds it) but absent from the tests-spec matrix — ~22 [Test]/[TestCase] attributes across 4 files that never execute. Outside the PR's stated Nethermind.* scope, but squarely inside "a test suite that never runs is the limiting case". One matrix line. Fix this →
4. Nethermind.Analyzers.Test brings a test-runtime network dependency into the matrix. Microsoft.CodeAnalysis.CSharp.Analyzer.Testing's ReferenceAssemblies.Net.Net80 / .Net100 (used across 7 of its test files) resolve Microsoft.NETCore.App.Ref from NuGet while the tests run — the job's dotnet restore can't prefetch these, since they're not PackageReferences. Fine on GitHub runners, but note the failure mode: a NuGet hiccup surfaces as rc=1, which the step treats as a genuine test failure and does not retry (only non-0/1 exit codes get the crash retry). Nothing to do now; worth remembering if this job is the one that goes red intermittently.
Nit
+7 projects × 2 Linux runners = 14 new jobs per PR (plus coverage on master pushes) for 340 tests total. Per-project jobs match the existing structure, so consistency probably wins over batching the small ones — noting it only because the arithmetic isn't free.
Nothing blocking. Verdict is contingent on this PR's own Code Lint, Build solutions, and the 14 new test jobs going green — which, as the PR body says, is the real verification.
· branch test-hygiene/wire-orphan-test-projects-into-ci
There was a problem hiding this comment.
Pull request overview
This PR improves CI coverage by ensuring previously “orphaned” Nethermind.* test projects are included in the primary test workflow matrix, and that Nethermind.OpcodeTracing.Plugin.Test is also built by the shared test-artifact solution build.
Changes:
- Adds 7 missing
Nethermind.*test projects to thetestsjob matrix in.github/workflows/nethermind-tests.ymlso they run in CI. - Adds
Nethermind.OpcodeTracing.Plugin.Testtosrc/Nethermind/Nethermind.slnxso the shared Linux test-artifact build produces binaries for it.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Nethermind/Nethermind.slnx |
Includes Nethermind.OpcodeTracing.Plugin.Test in the main solution so it’s built in shared test artifacts. |
.github/workflows/nethermind-tests.yml |
Extends the test matrix to run additional Nethermind.* test projects on the standard runners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The OpcodeTracing test project enables ImplicitUsings, so the remaining leading System.* usings are redundant - IDE0005 anchors the whole contiguous group at line 4, which is why removing only 'using System;' did not clear the warning. Also, per review: wire Ethereum.Ssz.Test into tests-spec (it is in EthereumTests.slnx but ran in no matrix), and add a matrix-guard job that fails when a *.Test project directory has no matrix entry, so the next orphaned test project fails CI instead of silently never running.
|
@claude re-review |
|
Claude encountered an error after 3m 20s —— View job I'll analyze this and get back to you. |
|
@claude re-review |
|
Claude finished @benaadams's task in 4m 51s —— View job Re-review:
|
| Prior note | Status |
|---|---|
Low 1 — first-time code-lint exposure for OpcodeTracing.Plugin.Test |
✅ Real: IDE0005 fired. ac78ff6/c51732a removed 7 now-implicit usings. Code Lint and Build solutions green |
| Low 2 — no guard against the next orphan | ✅ matrix-guard job added |
Low 3 — Ethereum.Ssz.Test unwired |
✅ Added to tests-spec. Verified in EthereumTests.slnx:69 (artifact has binaries for --no-build) and correctly not in FIXTURE_PROJECTS — it reads no submodule fixtures |
Low 4 — Analyzers.Test runtime NuGet dep |
Unchanged, still just a note |
CI on head c51732a: 14/14 workflow runs green, including Check test project matrix is exhaustive (job) and the new test jobs. That also settles the one thing I couldn't check statically: [[ … ]] && continue under Actions' bash -eo pipefail is not fatal on the allowlist branch (errexit is suppressed for the non-final command of an && list), so the loop doesn't abort on the first allowlisted project.
Medium
1. matrix-guard is in needs but not in the assertion — the guard doesn't gate. Inline. tests-summary is if: always() with an explicit four-way [[ ]] (L408–413) covering tests/tests-spec/tests-chunked/nethtest. matrix-guard was added to needs (L402) but not to the conjunction, so a guard failure leaves Tests summary green; the needs entry's only effect is to delay the summary. In a workflow that deliberately funnels everything into one aggregate check, that means the next orphan shows up as a red job inside a green required check — the same silent-pass this PR exists to close. One line in the assertion.
2. The Ethereum.Blockchain.Pyspec.Test allowlist rationale covers only part of the assembly. Inline. "superseded by the nethtest job" is true for the CiRunnerGuard-gated fixture fixtures in PyspecTestFixture.cs. It is not true for the plain NUnit unit tests sitting in the same project: ZkEvmFixtures/ZkEvmBlockchainTests.cs:89 StatelessSchemaTests — no guard, no [Explicit] — contributes ~20 cases (Revision_1_schema_roundtrips ×6, Request_struct_conversion_roundtrips ×5, the [TestCase(0x0f01)] / [TestCase(10UL, 20UL, true)] / BlockchainIds sets, one bare [Test] at L180), plus CiSentinelTests.cs:16. nethtest drives Nethermind.Test.Runner over fixture JSON and never loads this assembly, and the project is in no matrix in any of the three test workflows. So it is an orphan of exactly the class this PR fixes — and the allowlist is what will keep the new guard quiet about it. Adding it to tests-spec is enough: it's in EthereumTests.slnx:58, not a FIXTURE_PROJECTS member, the fixture fixtures self-skip via CiRunnerGuard, and CiSentinelTests keeps the run non-empty.
Low
3. nethermind-tests-checked.yml drift is now 10 projects, and the guard doesn't see it. The guard greps only nethermind-tests.yml. -checked lists 44 projects and is missing all 7 newly wired ones plus Ethereum.Ssz.Test, Nethermind.EraE.Test and Nethermind.State.Flat.Test (the last two predate this PR). Out of the PR's stated scope, and -flat is a deliberate 5-project subset — but the same class of defect remains unguarded one file over. If -checked is meant to be exhaustive, a one-line comment saying so plus extending the guard to it would close the loop; if it's a curated subset, saying that in the file is enough. Fix this →
4. The guard's glob only matches *.Test, so *.Tests names escape it silently. for dir in src/Nethermind/*.Test misses Nethermind.IntegrationTests (fine — it runs via run-e2e-tests.yml:68), but that's by accident of the glob rather than by the allowlist, so it isn't documented, and a future Nethermind.Foo.Tests would be invisible to the guard rather than caught. src/Nethermind/*.Test?(s) with shopt -s extglob, or *.Test* plus a couple more allowlist entries for Ethereum.Test.Base / Nethermind.Test.Runner / Nethermind.EthereumTests.Benchmark, would make the coverage boundary explicit. Same shape: [[ -f "$dir/$project.csproj" ]] || continue silently skips any project whose directory name ≠ csproj name — currently none, and such a project would fail the tests job anyway.
5. Flow-style exclude: entries would satisfy the guard. The second alternative project: ${esc}[,} ] matches inline mapping entries, and it can't tell an include: from an exclude:. No such case exists today — the only exclude: blocks (L109, L224) use block style, whose trailing newline the pattern correctly rejects — so a project that's excluded on every runner would currently be caught. Just noting the pattern doesn't guarantee that.
What I re-verified
- Guard produces no false positives on the real tree — confirmed by the green job, cross-checked against all 78
*Test*project files. Anchored^ +- <esc>$correctly distinguishesNethermind.State.TestfromNethermind.State.Test.Runner.Test, andEthereum.Legacy.Blockchain.Testmatches via thetests-chunkedflow-form alternative. - Comment lines can't produce false matches (
^ +-requires-immediately after the indent, so# - Foodoesn't match), and theallowlist=line itself matches neither alternative. - Matrix ordering still ASCII-alphabetical, including
Ethereum.Rlp.Test<Ethereum.Ssz.Test<Ethereum.Transaction.Test. - The 7 removed usings in
OpcodeTracingModuleTests.cs/OpcodeTraceRecorderTests.csare all implicit-usings duplicates; compilation is confirmed by green Build solutions, Code Lint and the test jobs themselves.
Neither Medium is a defect in what now runs — all 348-odd newly wired tests do execute and pass on both Linux arches. They're both about the guard's own effectiveness, which is the part that has to be right if it's going to prevent the next instance rather than document this one. Finding 1 is a one-line fix; finding 2 is one matrix line and a narrowed comment.
· branch test-hygiene/wire-orphan-test-projects-into-ci
| tests-summary: | ||
| name: Tests summary | ||
| needs: [tests, tests-spec, tests-chunked, nethtest] | ||
| needs: [tests, tests-spec, tests-chunked, nethtest, matrix-guard] |
There was a problem hiding this comment.
matrix-guard is added to needs but its result is never asserted, so the guard is not actually gating.
tests-summary runs if: always() and its check is an explicit four-way conjunction (lines 408–413) that still only covers tests, tests-spec, tests-chunked, nethtest. With matrix-guard in needs but absent from the [[ ]], a guard failure leaves Tests summary green — the only effect of the needs entry is to delay the summary until the guard finishes.
That matters because this workflow deliberately funnels every job into one aggregate check; if Tests summary is the required status check, a future orphaned test project produces a red Check test project matrix is exhaustive job inside an otherwise-green required check, which is precisely the "silently never runs" failure mode this PR exists to close. Adding it to needs shows the gating was intended, so this reads as an oversight rather than a choice.
| needs: [tests, tests-spec, tests-chunked, nethtest, matrix-guard] | |
| needs: [tests, tests-spec, tests-chunked, nethtest, matrix-guard] |
…plus the matching line in the assertion:
"${{ needs.matrix-guard.result }}" == "success" && \
(If the intent really is that the guard is advisory and gated only as its own check, dropping it from needs and saying so in a comment would be clearer than the current half-wired state.)
| # Intentional exclusions: | ||
| # - Nethermind.Precompiles.Benchmark.Test: lives in Benchmarks.slnx only, not built by the test artifact | ||
| # - Ethereum.Blockchain.Pyspec.Test: superseded by the nethtest job (see run-nethtest.yml) | ||
| allowlist="Nethermind.Precompiles.Benchmark.Test Ethereum.Blockchain.Pyspec.Test" |
There was a problem hiding this comment.
The Ethereum.Blockchain.Pyspec.Test allowlist rationale is only true for part of the project, and allowlisting it permanently hides a real orphan.
"superseded by the nethtest job" holds for the fixture-driven fixtures (PyspecTestFixture.cs — those are CiRunnerGuard-gated and their coverage did move to run-nethtest.yml). It does not hold for the plain unit tests that live in the same assembly:
ZkEvmFixtures/ZkEvmBlockchainTests.cs:89StatelessSchemaTests— noCiRunnerGuard, no[Explicit]:Revision_1_schema_roundtrips(6 cases),Request_struct_conversion_roundtrips(5 cases), plus the[TestCase(0x0f01)]/[TestCase(10UL, 20UL, true)]/[TestCase(BlockchainIds.Gnosis, true)]sets and one bare[Test]at line 180 — roughly 20 cases of stateless-payload schema andExecutionRequestsize assertions.CiSentinelTests.cs:16— the zero-tests-ran sentinel, which by construction cannot fire if the project is never run.
nethtest drives Nethermind.Test.Runner over fixture JSON; it does not execute this NUnit assembly, and Ethereum.Blockchain.Pyspec.Test appears in no matrix in any of the three test workflows. So these are never-run tests of exactly the class this PR is closing — and this allowlist entry is what will stop the new guard from ever reporting them.
Two options, either fine:
- Add
Ethereum.Blockchain.Pyspec.Testto thetests-specmatrix. It's inEthereumTests.slnx, so the artifact already contains its binaries, and it's not inFIXTURE_PROJECTS, so it needs no submodule. TheCiRunnerGuardcalls make the fixture fixtures self-skip,CiSentinelTestskeeps the run non-empty, and the ~20 unit cases start executing. - Or move
StatelessSchemaTestsinto a project that does run, and keep the allowlist entry with the rationale narrowed to "fixture shards only".
Either way the comment should stop implying the whole assembly is covered.
|
@claude re-review |
|
Claude finished @benaadams's task in 1m 50s —— View job Re-review:
|
The guard job sat in needs but not in the result conjunction, so a guard failure left the summary green. The Pyspec allowlist comment now states what stays unwired and why: test discovery loads the full fixture sets, so a tests-spec run exceeds the job timeout.
CiSentinelTests is scaffolding for filtered-out runs, not coverage.
Changes
How to review this fast: three commits (base change, then two review/lint follow-ups), +36/-8 across 4 files - new matrix entries and a guard job in
nethermind-tests.yml, one missing project entry inNethermind.slnx, and redundant usings removed from the newly built test project. No product code changed.Nethermind.*test projects were absent from every test workflow (nethermind-tests.yml,-flat,-checked), so their tests never ran in CI: Analyzers.Test, BalRecorder.Test, CensorshipDetector.Plugin.Test, OpcodeTracing.Plugin.Test, Serialization.SszGenerator.Test, State.Flat.History.Test, StateDiffsWriter.Test. This adds them to thetestsjob matrix.Nethermind.OpcodeTracing.Plugin.Testwas also missing fromNethermind.slnx(only the plugin itself was listed), so the shared test-artifact build produced no binaries for it; added to the solution.Nethermind.Precompiles.Benchmark.Testis deliberately not wired in: it lives inBenchmarks.slnxonly, and the test-artifact build compiles onlyNethermind.slnx+EthereumTests.slnx; pulling the BenchmarkDotNet-dependent benchmark project into the main solution for its single test seemed disproportionate.From review feedback:
matrix-guardjob now fails CI when asrc/Nethermind/*.Testdirectory has no matrix entry, so projects cannot silently fall out of CI again (allowlist:Nethermind.Precompiles.Benchmark.Test,Ethereum.Blockchain.Pyspec.Test). It is wired intotests-summaryso branch protection sees it.Ethereum.Ssz.Testwas inEthereumTests.slnxbut in no matrix; added totests-spec.ImplicitUsings(flagged by lint once the project actually built in the solution).This is the first PR of a test-hygiene effort (fixing tests that cannot meaningfully fail); a test suite that never runs is the limiting case, so it goes first.
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
All 7 projects were run locally on windows-x64 (release, sequential): Analyzers 96, BalRecorder 24, CensorshipDetector 7, OpcodeTracing 16, SszGenerator 61, State.Flat.History 100, StateDiffsWriter 36 - 340 tests, 0 failed, 0 skipped. The real verification is this PR's own checks: the 7 new matrix jobs must appear and pass on ubuntu x64/arm64.
Documentation
Requires documentation update
Requires explanation in Release Notes