Skip to content

test: strengthen mock-echo and vacuous tests in Abi, Optimism and Xdc test projects - #12720

Open
benaadams wants to merge 5 commits into
masterfrom
test-hygiene/optimism-xdc-abi-mock-echo
Open

test: strengthen mock-echo and vacuous tests in Abi, Optimism and Xdc test projects#12720
benaadams wants to merge 5 commits into
masterfrom
test-hygiene/optimism-xdc-abi-mock-echo

Conversation

@benaadams

Copy link
Copy Markdown
Member

Changes

Test-hygiene chunk: mock-echo and do-nothing tests in Nethermind.Abi.Test, Nethermind.Optimism.Test, and Nethermind.Xdc.Test. A scanner pass over the three projects found 43 candidates (no-verification bodies, Received-only tests, Not.Null-only asserts); after reading every hit, three fixes remained - the rest verify real behavior through local assert helpers or use Received as a legitimate oracle for routing/dedup contracts (details below). No product code changed (verified: diff touches only *.Test files).

  • Nethermind.Abi.Test/AbiEncoderExtensionsTests.cs: both tests called the extension and asserted only Received() on the inner call, ignoring the extension's return value. They now stub the exact unpacked arguments and assert the extension returns the stubbed instance. A misforwarded argument makes the substitute return null, so the identity assert pins the unpacking and the return propagation at once.
  • Nethermind.Xdc.Test/ModuleTests/XdcRocksDbConfigFactoryTests.cs: the single test asserted Is.Not.Null twice on values that cannot be null. The factory's contract is routing: Xdc databases (XdcSnapshots, XdcRewards) get a PerTableDbConfig without consulting the base factory (validation bypass - the reason the class exists), every other database delegates. Both branches are now pinned (InstanceOf + options read through the provided IDbConfig + DidNotReceive for the special names; Is.SameAs the base result for delegation). The delegation branch was previously untested, and the options assert also pins the forwarded database name (the Xdc names have no dedicated options, so the config must fall back to the unprefixed options; a wrong name appends prefixed options). Note: PerTableDbConfig validation is #if DEBUG only, so no throw-based assert is possible in release mode.
  • Nethermind.Xdc.Test/XdcProtocolHandlerTests.cs (one line): the timeout routing test used Arg.Any<Timeout>() while its vote sibling pins the routed value - a handler routing the wrong timeout passed. It now pins the routed timeout. Timeout equality is RLP-hash based (full wire content: round, signature, gap), which is the semantically right pin for routed message content - a content-equal copy is behaviorally identical, so reference identity is deliberately not asserted (matches the vote sibling's idiom).

Triage of the remaining scanner hits (all verified by file read, left unchanged)

  • Assert-helper false positives: ForkInfoTests (delegates to the asserting shared Network.Test.ForkInfoTests.Test helper), OptimismReceiptTests (AssertL1AndOperatorFees), CL/PayloadDecoderTests (ComparePayloads), Rpc/DepositTransactionForRpcTests (ValidateSchema), Xdc HobbitTests (HobbitTestsBase.Run asserts byte-identical round-trips), XdcSubnetHeaderDecoderTests (AssertRoundTrip with a field comparer).
  • Legitimate Received-based behavioral tests: XdcProtocolHandlerTests (conditional routing, send dedup, sync gating), VotesManagerTests (QC build timing and dedup, DidNotReceive on missing header), SyncInfoManagerTests (dispatch with exact certificates), SignTransactionManagerTests (60s signing-window boundary via Received(1/0)), OptimismEngineRpcModuleTest (version comparison branching via Received(1/0) with exact args).
  • Optimism.Test/RlpDecoderTests.cs is excluded: already strengthened on the open test: make decoder tests assert the decoded values #12711 branch.
  • Nethermind.Taiko.Test is out of scope for the whole effort.

Mutation evidence

Product mutations applied against committed state, run, and reverted:

Mutation New tests Old tests
Extension discards the inner result and returns [] 2 red 2/2 green (run empirically - Received cannot see the return value)
Factory special-DB branch dead-matched (everything delegates) 2 red (InstanceOf/DidNotReceive) 1/1 green (run empirically - in release the old test cannot tell whose config it got)
Factory forwards a hardcoded database name instead of the requested one 2 red (options assert) green (nothing pinned the name)
Timeout expectation flipped to a different timeout 1 red Arg.Any passes by construction

Verification

  • Full Nethermind.Abi.Test: 226/226 passed. Full Nethermind.Xdc.Test: 543/543 passed.
  • Touched fixtures ran 3x each with 0 failures.
  • dotnet format whitespace on both projects: no changes.
  • Dual-reviewed (Codex + Opus 4.8) to zero findings before push.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Tests are the subject of this PR. Each strengthened assertion was shown to fail under a product perturbation or an expectation flip, and the replaced assertions were shown empirically to stay green under mutations the new ones catch (table above).

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

The extension tests asserted only Received on the inner call and
ignored the extension return value. A stub on the exact unpacked
arguments plus an identity assert on the result covers both.
The single test asserted Is.Not.Null on a result that cannot be null.
The factory contract is routing: Xdc databases get a PerTableDbConfig
without consulting the base factory, and every other database delegates
to it. Both branches are now pinned; the delegation branch was
untested.
The vote routing test pins the exact vote, but the timeout test used
Arg.Any, so a handler that routes the wrong timeout passed.
Removes an unused using that fails lint CI. Pins the factory options
against the provided IDbConfig, so a wrong database name in the special
branch turns the test red. Moves the extension rationale to class level
and renames the Abi tests to the project snake_case convention.
NSubstitute returns an empty array, not null, for an unmatched call on
an array-returning member. The prefixed-options claim holds for only
half of the database prefixes, so the sentence is dropped.
Copilot AI review requested due to automatic review settings August 6, 2026 16:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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.

2 participants