test: Db tests assert stored state instead of smoke-calling empty methods

[benaadams]

Changes

How to review this fast: four commits, one concern each; 5 files, all under Nethermind.Db.Test (no product code changed); net -23 lines.

• Deletes the six *_does_not_cause_trouble tests: MemDb.Flush/Dispose and the SnapshotableMemDb variants are empty method bodies (MemDb.cs:83,100), and SnapshotableMemColumnsDb.Flush/Dispose only loop over those empty bodies - the tests could not fail under any code change, because there is no behavior to cover.
• MemDbTests.Can_create_with_delays / Can_create_without_arguments had no assertions; they now verify the written value round-trips through Get and the batch indexer (grouped in Assert.EnterMultipleScope).
• ReadOnlyDbProviderTests.Can_clear called ClearTempChanges() on a provider with an empty registry - no db was ever registered, so the method body was a no-op loop. It now registers a real MemDb-backed ReadOnlyDb through a substituted IDbProvider and asserts: the in-memory overlay is dropped by ClearTempChanges while the wrapped db stays intact (localChanges: true), and Set throws InvalidOperationException when writes are not expected (localChanges: false).
• DbOnTheRocksDbTests.Can_get_all_on_empty discarded the enumeration; it now asserts Is.Empty on the per-test fresh db (both useColumnDb fixture variants).

Part of the test-hygiene series (#12689, #12690): tests that cannot meaningfully fail get real assertions or are removed where nothing exists to cover.

Types of changes

What types of changes does your code introduce?

• Refactoring

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

• Full Nethermind.Db.Test suite green locally (windows-x64, release): 1091 total, 0 failed (544 skipped are pre-existing fixture-mode guards).
• Mutation-checked: making ReadOnlyDb.ClearTempChanges a no-op fails Can_clear(true); perturbed expectations fail the strengthened MemDb test and both Can_get_all_on_empty fixture variants. All mutations reverted.
• The deleted tests' subjects were independently verified as empty method bodies; the columns variants remain exercised indirectly via MemDbFactory.CreateColumnsDb and PseudoNethermindModule.

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

[claude]

Claude encountered an error after 3m 18s —— View job

---

I'll analyze this and get back to you.

[Copilot]

Pull request overview

This PR strengthens Nethermind.Db.Test by removing tests that could not meaningfully fail (smoke-calling empty methods) and replacing assertion-free tests with checks that validate stored state and observable behavior.

Changes:

• Removed *_does_not_cause_trouble tests for Flush/Dispose on in-memory DB implementations where the methods are effectively no-ops.
• Added assertions to MemDbTests and ReadOnlyDbProviderTests to verify round-trips, overlay clearing, and expected write behavior.
• Made DbOnTheRocksDbTests.Can_get_all_on_empty assert the empty enumeration rather than discarding it.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Nethermind/Nethermind.Db.Test/SnapshotableMemDbTests.cs	Removes no-op Dispose/Flush smoke tests.
src/Nethermind/Nethermind.Db.Test/SnapshotableMemColumnsDbTests.cs	Removes no-op Dispose/Flush smoke tests for columns DB.
src/Nethermind/Nethermind.Db.Test/ReadOnlyDbProviderTests.cs	Makes Can_clear assert overlay clearing / wrapped DB persistence and write expectations.
src/Nethermind/Nethermind.Db.Test/MemDbTests.cs	Adds assertions to previously assertion-free construction tests to verify round-trip reads.
src/Nethermind/Nethermind.Db.Test/DbOnTheRocksTests.cs	Ensures empty DB GetAll() is asserted as empty for both fixture variants.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[benaadams]

@claude re-review

[claude]

Claude encountered an error after 3m 59s —— View job

---

I'll analyze this and get back to you.

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 4m 6s —— View job

---

Re-review complete — no blocking findings

• Read PR diff (336260a, 7856531, 75044b8)
• Verified ReadOnlyDbProvider / ReadOnlyDb semantics against the new assertions
• Checked the unused-using claim from the Copilot review
• Checked MemDb / DbOnTheRocks assertions and fixture isolation
• Checked repo rules (.agents/rules/test-infrastructure.md, coding-style.md, robustness.md) and CI

---

Verification performed

Deletions are justified. Confirmed against source, not just the PR description:

• MemDb.Flush (Nethermind.Db/MemDb.cs:83) and MemDb.Dispose (:98) are { }.
• SnapshotableMemDb.Flush (:98) / Dispose (:202) are { }; the type holds only managed state (SortedSet, HashSet, Lock) — no unmanaged/pooled resource that a disposal test would guard.
• SnapshotableMemColumnsDb.Flush/Dispose (:70-83) only loop over those empty bodies.

ReadOnlyDbProviderTests.Can_clear now exercises real code. ReadOnlyDbProvider.GetDb<T> (ReadOnlyDbProvider.cs:44-47) resolves through the substituted provider and wraps in ReadOnlyDb via IDb.CreateReadOnly (IDb.cs:18), so:

• localChanges: true → Set lands in ReadOnlyDb._memDb; ClearTempChanges → _memDb.Clear() (ReadOnlyDb.cs:69) makes Get(KeccakB) fall through to the wrapped db and return null. The assertion genuinely fails if ClearTempChanges becomes a no-op.
• localChanges: false → Set throws InvalidOperationException (ReadOnlyDb.cs:22-27), and the KeccakA assertion still proves the wrapped db is reachable and untouched. Correctly guarded so the overlay-drop assert only runs where a write could land in the overlay.

DbOnTheRocksDbTests.Can_get_all_on_empty is deterministic. DbOnTheRocksDbTests (:390) has a per-test DbPath keyed on TestContext.CurrentContext.Test.ID, deleted and recreated in [SetUp] (:398-423), with [Parallelizable(ParallelScope.None)] — so Is.Empty cannot be polluted by sibling tests, for both useColumnDb variants. NUnit's empty-collection check enumerates with foreach, so the RocksDB iterator is still disposed exactly as the old .ToList() did.

Assert.EnterMultipleScope usage matches the repo rule (test-infrastructure.md:67-99): assertion blocks sit after arrange/act, contain no state mutation, and no null-guard-then-dereference chains.

---

On the Copilot review comment (ReadOnlyDbProviderTests.cs:6)

using Nethermind.Core; is unused in this test file…

That is incorrect — removing it breaks the build. Set(Hash256, byte[]) and Get(Hash256) are extension members declared in Nethermind.Core.KeyValueStoreExtensions (Nethermind.Core/KeyValueStoreExtensions.cs:12, :25, :98), and Nethermind.Core is not a parent of the Nethermind.Db.Test namespace, so it must be imported explicitly — same as MemDbTests.cs:6 already does. Code Lint / Build solutions are green on this branch, which corroborates it. No action needed.

---

Findings

No Critical, High, or Medium findings. Three Low nits, all optional:

Low — ReadOnlyDbProvider is IDisposable and is not disposed (ReadOnlyDbProviderTests.cs:24). Previously nothing was registered so it didn't matter; now a real ReadOnlyDb is registered, and robustness.md says IDisposable should be using-wrapped. Harmless in practice (ReadOnlyDb.Dispose → MemDb.Dispose is a no-op), but using ReadOnlyDbProvider dbProvider = new(...) costs nothing and keeps the file exemplary. Fix this →

Low — repeated new byte[] { 1, 2, 3 } literal (MemDbTests.cs:33-39, :56-57). The fixture already has private readonly byte[] _sampleValue = { 1, 2, 3 }; (:27); reusing it would trim the duplication the rest of the file already avoids.

Low — Can_create_with_delays still doesn't assert anything about the delays. The round-trip assertions are a strict improvement, but the test name promises coverage of the (10, 10) read/write delay ctor and nothing in the body would fail if the delay parameters were dropped. MemDb exposes ReadsCount / WritesCount, so Assert.That(memDb.ReadsCount, Is.EqualTo(2)) (one Get, one batch key) plus WritesCount == 1 would at least pin the delayed paths that increment them. Fine to leave for a follow-up given this PR's stated scope.

---

Verdict

Test-only change (no product code touched), net −23 lines, each commit one concern. All three strengthened tests fail under the mutations described in the PR body, which I verified is consistent with the source semantics. CI is green: Build solutions, Code Lint, Code formatting, Nethermind tests (Flat DB), Nethermind extra test variants, Integration tests (E2E) all passing; 0 failures across 30 runs. Mergeable — the Copilot comment is a false positive and should not be acted on.
· branch test-hygiene/db-tests-assert-stored-state