test(ecstore): assert the error conversions, and stop the census over-reporting - #6241
Merged
Conversation
…-reporting The census listed 17 candidates in ecstore. Sixteen were false positives of three shapes, and reading them showed the heuristics rather than the tests were wrong: - `#[should_panic(expected = "...")]` (5). `should_panic` was already in the verification signals, but the check only ever ran against the function body — the attribute block was collected and then ignored, so the expected panic message, which *is* the assertion, was invisible. - Bodies that are a single call into a shared harness (9), like `run(DurabilityMode::Strict).await` and `aborting_encode_drops_blocked_producer(EncodePipeline::Vec).await`. The delegation rule keyed off callee names (`assert_`/`verify_`/`run_`/`_harness`), which these do not match, though a body that is nothing but one call delegates by construction whatever the callee is called. - Compile-time contracts (2): a turbofish between the callee and its parens (`assert_replication_config_ext::<T>()`) broke the delegation regex, and a nested `fn` that is only bound and discarded is the same signature guard as the already-recognised `fn _name()` form. The script now folds the attribute block into the verification text, allows a turbofish in the delegation patterns, and recognises both a single-call body and a discarded nested-fn binding. Tree-wide candidates drop from 53 to 33, ecstore from 17 to 1. The one that survives was real: `test_error_conversions` performed two conversions and discarded both results. It now pins what each conversion must produce — a plain `io::Error` stays `DiskError::Io` rather than being guessed at from its `NotFound` kind, a typed error boxed through `io::Error` round-trips back to itself instead of degrading to `Io`, and a serde_json error folds into `other` with its message intact. Refs backlog#1836
Contributor
|
CLA requirements are satisfied for this pull request. |
houseme
approved these changes
Aug 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fourth batch of backlog#1836, taking on the largest remaining cluster: 17 candidates in
ecstore. One was a real assertion-less test. The other sixteen were the census mis-reading three legitimate patterns, so this fixes the script rather than the tests — the same call batch two made when its delegation heuristic mis-fired.The sixteen false positives
#[should_panic(expected = "...")]— 5 tests.should_panicwas already listed inVERIFY_SIGNALS, but the check only ran against the function body. The attribute block was collected intoattrsand then never used, so the expected panic message — which is the assertion — was invisible to the scan.A body that is a single call — 9 tests.
run(DurabilityMode::Strict).await,aborting_encode_drops_blocked_producer(EncodePipeline::Vec).await. The delegation rule keyed off the callee's name (assert_,verify_,run_,_harness, …), which these do not match. But a body whose entire content is one call delegates by construction, whatever the callee happens to be called — that is now its own rule.Compile-time contracts — 2 tests.
assert_replication_config_ext::<ReplicationConfiguration>()does match theassert_prefix, but the turbofish sits between the name and the parens and broke\s*\(. Andtier.rs's signature guard declares a nestedfnand then discards it withlet _ = call_legacy_refresh;— the same "the type system is the assertion" shape as the already-recognisedfn _name()form.Tree-wide candidates drop from 53 to 33; ecstore from 17 to 1.
The one that was real
test_error_conversionsran two conversions and threw both results away:It now pins what each conversion owes its caller:
io::ErrorbecomesDiskError::Ioand keeps its kind — it must not be promoted toFileNotFoundon the strength ofErrorKind::NotFound, sincereduce_errscounts those as different errors during quorum aggregation;DiskErrorboxed throughio::Errorround-trips back to itself rather than degrading toIo, which is the behaviour theFromimpl's own comment calls out;serde_json::Errorfolds intootherwith its message intact.Verification
cargo nextest run -p rustfs-ecstore --lib -E 'test(test_error_conversions)'passes,cargo clippy -p rustfs-ecstore --all-targets -- -D warningsis clean,cargo fmt --allapplied. The script change was checked against the excluded tests individually — each of the sixteen is confirmed to carry verification the scan could not see.