test: give vacuous size-extension and hint-cache tests real expectations

[benaadams]

Changes

Part of the test-hygiene series (#12689, #12690 - merged, #12693, #12694, #12696 - merged, #12699, #12705): tests that cannot meaningfully fail get real expectations. Rule for this chunk: numeric and cache assertions that hold for almost any implementation (>= 0, bare Is.Not.Null) become exact expectations and boundary checks. Three commits, one concern each; two test files; no product code changed (verified).

• SizeExtensionsTests: CheckOverflow_long/CheckOverflow_int asserted testCase.GB >= 0 on inputs that can never wrap, so no multiplier change could fail them. They now assert exact products, including the true long boundary (9_223_372_036 = Int64.MaxValue / 1e9) and a pin of the unchecked wrap one past it. The int test documents that an int input cannot overflow (it widens to long first). Expected values hand-derived and cross-checked in Python (9223372037 * 10^9 - 2^64 = -9223372036709551616).
• HintBasedCacheTests: five tests asserted only Is.Not.Null on hinted epochs - true of any non-null-returning stub. Each now also pins the boundary (Get outside the hinted epoch range returns null), Sync_hint_and_get pins CachedEpochsCount == 1 (the count increments synchronously inside Hint), and Many_threads now covers the last hinted epoch, which the old loop bound silently skipped. A new Get_returns_the_cached_instance test pins the cache identity contract (Is.Not.Null.And.SameAs - guarded against the SameAs(null) vacuous pass). Multi-assert blocks use Assert.EnterMultipleScope with per-iteration messages.

Triage note: the chunk originally included ProcessingStatsTests (census: 7x >= 0 on stopwatch fields), but that file has since been rewritten with real assertions; the remaining #if DEBUG Assert.Ignore is a legitimate guard (ProcessingStats's constructor forces SetDebugMode() under #if DEBUG, so the windowed-aggregation assertion cannot hold in Debug builds) and is left as is.

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

Every strengthened assertion was mutation-checked (deliberate product break, confirm red, revert):

• Dropping a zero from the GB multiplier fails 6/8 of the new SizeExtensions cases; the old >= 0 tests fail 0/6 under the same mutation.
• An off-by-one in HintBasedCache.Hint's epoch loop fails 8/11 HintBasedCache tests.
• Get rebuilding a data set instead of returning the cached one fails the identity test; a no-op Hint also fails it (it passed before the Is.Not.Null.And guard - SameAs(null) vs null passes in NUnit).
• One expectation flip per file confirmed red.

Full local suites green (windows-x64, release): Core.Test 5987 total 0 failed, Mining.Test 69 total 0 failed. HintBasedCacheTests repeated 10x (plus 3x after the review fixes) with no flakes.

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

[claude]

Claude finished @benaadams's task in 3m 20s —— View job

---

Review: test hygiene for SizeExtensions / HintBasedCache

• Read the diff (git diff origin/master...HEAD)
• Verify SizeExtensions.GB semantics and the hand-derived constants
• Verify every new HintBasedCache boundary expectation against HintBasedCache.Hint/Get
• Check the Many_threads loop-bound change and count consistency
• Post review

Verdict: no Critical / High / Medium findings. Two Low nits, both posted inline. Good to merge.

Test-only change, no product code touched — confirmed, the diff is two files under *.Test.

---

What I verified

SizeExtensionsTests — every expected value re-derived independently:

input	expected in test	derivation
9_223_372_036	9_223_372_036_000_000_000	< Int64.MaxValue (9_223_372_036_854_775_807) — largest non-wrapping input ✓
9_223_372_037	-9_223_372_036_709_551_616	9_223_372_037e9 − 2^64 ✓
int.MaxValue	2_147_483_647_000_000_000	widens to long first, cannot wrap ✓

The wrap test only holds if the multiply is unchecked. I grepped for CheckForOverflowUnderflow across all .props/.targets/.csproj — nothing sets it, so Nethermind.Core compiles with the C# default (unchecked) and GB_long_wraps_past_boundary is sound rather than throwing OverflowException. Worth knowing that this test now pins the wrap as contract: if anyone later makes these saturate or throw, this reads as a regression rather than a fix. The inline comment on line 12 of the source (callers must range-check first) makes that intent explicit, so I'm fine with it.

[TestCase(MaxGbLong, ...)] using a private const long in an attribute is legal (compile-time constant), and the int/long argument pairs match the parameter types, so no NUnit1001 from the analyzers.

HintBasedCacheTests — Ethash.EpochLength = 30000, GetEpoch(n) = n / 30000. Every new boundary assertion checks out:

• With_hint_returns_value / Different_users_reuse_cached_epochs: Hint(0, 200000) → epochs 0..6; Get(7) genuinely never hinted ✓
• Sync_hint_and_get: Hint(200000, 200000) → epoch 6 only, so Get(5)/Get(7) null ✓. CachedEpochsCount == 1 is safe — Interlocked.Increment happens inside the [MethodImpl(Synchronized)] Hint body before it returns, so no race with the assertion ✓
• Different_users_can_use_cache: epochs 0 and 1, Get(2) null ✓
• Can_hint_far: 1000000000 / 30000 = 33333; 33332/33334 never hinted ✓
• Many_threads — the substantive fix. range = 10_000_000, final hint window per guid is [10_000_000, 10_120_000] → epochs 333..337, i.e. exactly the 5 the pre-existing CachedEpochsCount == 5 asserts. The old i < lastEpoch bound stopped at 336, so epoch 337 — the one most likely to be affected by an off-by-one in Hint's for (uint i = startEpoch; i <= endEpoch; i++) — was never checked. <= is correct. Get(332) is null because the terminal Hint for every guid drops its ref (_cachedSets.Remove, moved into _recent, which Get doesn't read); Get(338) would need i >= 10_020_000 > range, so it's unreachable ✓. All hinting tasks are awaited before any Get, so the unsynchronized _cachedSets read in Get isn't a new race here.
• Get_returns_the_cached_instance: both Get(6) calls resolve the same Task<IEthashDataSet> via .Result, and _createDataSet runs once per epoch, so SameAs is a real identity contract. The Is.Not.Null.And guard is the right call — NUnit's SameAs(null) against null passes, which would have made this vacuous.

Assert.EnterMultipleScope() needs NUnit ≥ 4.2; the repo pins 4.6.1 ✓. No scope spans an await (all are entered after the Wait.ForCondition), so no AsyncLocal context leakage across the fixture's ParallelScope.Self.

Findings

#	Severity	Location	Issue
1	Low	SizeExtensionsTests.cs:23-25	GB_long_wraps_past_boundary has the same body as GB_long_multiplies_exactly and could be one more [TestCase] (AGENTS.md DRY rule). Counter-argument: the method name documents the wrap. Deliberate-choice flag, not a defect.
2	Low	HintBasedCacheTests.cs:64-65	Hardcoded epoch 6 where the adjacent test derives it via Ethash.GetEpoch(200000UL).

Both have suggestion blocks inline.

Non-blocking observations

• SizeExtensions also exposes MB/KB/GiB/MiB/KiB and a whole ulong block, none of which have any test. That gap predates this PR, but since the file is already being turned into exact-product cases, adding rows for the siblings is nearly free — the ulong overloads in particular wrap differently (to a huge positive) and have zero coverage. Out of scope if you'd rather keep the chunk tight.
• The i.ToString() failure messages render as a bare 3; $"epoch {i}" would read better in a failure report. This matches the pre-existing style at line 186, so consistency argues for leaving it.

Testing limitation

I could not run dotnet test — the command needed approval that wasn't granted in this environment, so I have no local green run to report. Everything above is static verification: arithmetic re-derived by hand, epoch math traced against HintBasedCache.Hint, and NUnit/build-property assumptions checked by grepping the repo. Your PR body reports Core.Test 5987/0 and Mining.Test 69/0 green plus 13 repeats of HintBasedCacheTests with no flakes; CI on this PR is the authoritative confirmation.
• test-hygiene/vacuous-numeric-and-cache-assertions

[Copilot]

Pull request overview

Strengthens previously vacuous unit tests by replacing “can’t fail” assertions (>= 0, Is.Not.Null) with exact expectations and boundary checks, improving test signal without changing production behavior.

Changes:

• HintBasedCacheTests: adds boundary assertions for hinted epoch ranges, pins CachedEpochsCount, and adds an identity/instance-reuse contract test for Get.
• SizeExtensionsTests: replaces non-informative overflow checks with exact GB multiplication expectations, including the long boundary and the wraparound case past it.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Nethermind/Nethermind.Mining.Test/HintBasedCacheTests.cs	Replaces null-only assertions with precise range/identity expectations for hinted epochs.
src/Nethermind/Nethermind.Core.Test/SizeExtensionsTests.cs	Pins exact GB multiplication results and verifies overflow wrap behavior at the long boundary.

---

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

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 3m 19s —— View job

---

Re-review: test hygiene for SizeExtensions / HintBasedCache

• Read the current diff, including the three commits pushed after my first review
• Re-verify the SizeExtensions constants and the unchecked-wrap assumption
• Re-verify every HintBasedCache boundary, the identity test, and the new multi-assert scopes
• Check the two prior Low nits
• Check CI

Verdict: no Critical / High / Medium findings. One Low nit (posted inline). Good to merge.

Still test-only — the diff is two files under *.Test, no product code touched.

---

Delta since my last review (e6b7a37 → 11fc84d)

commit	change	assessment
125dfdd	Is.Not.Null.And.SameAs(firstGet) guard; widened multi-assert scopes	Correct — NUnit's SameAs(null) vs null passes, so the Is.Not.Null.And prefix is what makes the identity test non-vacuous
a69a9d0	remaining multi-assert blocks wrapped in Assert.EnterMultipleScope	Fine; EnterMultipleScope is used in 268 other files here, and every scope is entered after the await, so no AsyncLocal leakage under this fixture's ParallelScope.Self
11fc84d	identity test derives the epoch via Ethash.GetEpoch(200000UL)	Prior nit #2 addressed ✅

Prior nit #1 (fold GB_long_wraps_past_boundary into a [TestCase]) was left as a separate [Test] — that was flagged as a deliberate-choice call and the method name does carry the "why", so no objection.

Re-verified from scratch

SizeExtensionsTests — SizeExtensions.GB is @this * 1_000_000_000L (SizeExtensions.cs:12), int overload widens first (:35).

input	expected	check
9_223_372_036	9_223_372_036_000_000_000	< Int64.MaxValue (…854_775_807) — largest non-wrapping input ✓
9_223_372_037	-9_223_372_036_709_551_616	9_223_372_037e9 − 2^64 ✓, and the literal is in range for long ✓
int.MaxValue	2_147_483_647_000_000_000	widens to long, cannot wrap ✓

Re-grepped the whole repo for CheckForOverflowUnderflow — no matches, so Nethermind.Core compiles unchecked and the wrap test won't throw. MaxGbLong + 1 is a compile-time constant but the multiply happens at runtime inside the property, so no constant-folding-in-checked-context surprise. MaxGbLong in a [TestCase] argument is legal (const), and the argument types match the parameters — no NUnit1001.

HintBasedCacheTests — Ethash.EpochLength = 30000, GetEpoch(n) = (uint)(n / 30000) (Ethash.cs:43,50). Every boundary traced against Hint/Get:

• With_hint_returns_value / Different_users_reuse_cached_epochs: Hint(0, 200000) → epochs 0..6; Get(7) genuinely never hinted ✓
• Sync_hint_and_get: epoch 6 only → Get(5)/Get(7) null ✓. CachedEpochsCount == 1 is race-free: Interlocked.Increment runs inside the [MethodImpl(Synchronized)] Hint body before it returns ✓
• Get_returns_the_cached_instance: both calls resolve the same Task<IEthashDataSet> from _cachedSets[6], and _createDataSet runs once per epoch (HintBasedCache.cs:118, guarded by _epochRefs[epoch] == 1), so SameAs pins a real contract ✓
• Different_users_can_use_cache (Get(2)), Different_users_can_use_disconnected_epochs, Can_hint_far (33333, ±1) ✓
• Many_threads — the substantive fix, re-derived: all three guids terminate on the same Hint(10_000_000, 10_120_000) → epochs 333..337, exactly the 5 the pre-existing count asserts. The old i < lastEpoch bound stopped at 336, silently skipping 337 — the epoch most sensitive to an off-by-one in Hint's for (uint i = startEpoch; i <= endEpoch; i++). Get(332) is null because each guid drops its ref once its window passes 9_960_000; at refcount 0 the set moves from _cachedSets to _recent (:80-81), and Get reads only _cachedSets (:130). Get(338) is unreachable — it would need a hint start > range. ✓

CI on head (11fc84d, pushed 00:01:16Z; runs created 00:01:23Z): 14/14 green, including Build solutions, Code formatting, Code Lint, and the Nethermind test suites.

Findings

#	Severity	Location	Issue
1	Low	HintBasedCacheTests.cs:77-78 (and :200-202)	Leftover magic epochs (5/7, 1000000000 / 30000) sitting next to a line that derives the same value via Ethash.GetEpoch. Cosmetic; values are correct. Suggestion posted inline.

Non-blocking observations

• The Many_threads null-boundary now pins "Get does not fall back to _recent" as contract. That's the current behaviour and worth pinning, but note the direction of the signal: if someone later made Get serve from _recent (a plausible optimisation, since the set is still alive there for 30s), this test goes red as a "regression" when it's arguably an improvement. Same shape as the unchecked-wrap contract in SizeExtensionsTests — both fine, just worth being deliberate about.
• SizeExtensions still has zero coverage for MB/KB/GiB/MiB/KiB and the entire ulong block (which wraps to a huge positive rather than negative). Pre-existing gap, out of scope if you want the chunk tight, but nearly free now that the file is exact-product cases.

Testing limitation

Same as last time: I could not run dotnet build/dotnet test — both needed approval that wasn't granted in this environment, and git fetch origin master was likewise blocked (I diffed against the merge base ffbb46f instead, which is equivalent here). Everything above is static verification plus the green CI run on the head commit; CI is the authoritative confirmation of your reported Core.Test 5987/0 and Mining.Test 69/0.

--- • test-hygiene/vacuous-numeric-and-cache-assertions