mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-19 11:06:17 +00:00
test(ecstore): assert the error conversions, and stop the census over-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
This commit is contained in:
@@ -853,13 +853,32 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_error_conversions() {
|
fn test_error_conversions() {
|
||||||
// Test From implementations
|
// A plain io::Error carries no typed payload to recover, so it lands in
|
||||||
|
// `Io` rather than being guessed at from its kind — `NotFound` here must
|
||||||
|
// not silently become `FileNotFound`, which quorum aggregation counts as
|
||||||
|
// a different error (rustfs/backlog#1836).
|
||||||
let io_error = std::io::Error::new(std::io::ErrorKind::NotFound, "test");
|
let io_error = std::io::Error::new(std::io::ErrorKind::NotFound, "test");
|
||||||
let _disk_error: DiskError = io_error.into();
|
let disk_error: DiskError = io_error.into();
|
||||||
|
match &disk_error {
|
||||||
|
DiskError::Io(inner) => assert_eq!(inner.kind(), std::io::ErrorKind::NotFound),
|
||||||
|
other => panic!("a plain io::Error must stay typed as Io, got {other:?}"),
|
||||||
|
}
|
||||||
|
|
||||||
let json_str = r#"{"invalid": json}"#; // Invalid JSON
|
// A typed DiskError boxed through io::Error round-trips back to itself
|
||||||
|
// instead of degrading to `Io`.
|
||||||
|
let boxed: std::io::Error = std::io::Error::other(DiskError::VolumeNotFound);
|
||||||
|
assert_eq!(DiskError::from(boxed), DiskError::VolumeNotFound);
|
||||||
|
|
||||||
|
// serde_json errors have no dedicated variant and fold into `other`,
|
||||||
|
// keeping the original message.
|
||||||
|
let json_str = r#"{"invalid": json}"#;
|
||||||
let json_error = serde_json::from_str::<serde_json::Value>(json_str).unwrap_err();
|
let json_error = serde_json::from_str::<serde_json::Value>(json_str).unwrap_err();
|
||||||
let _disk_error: DiskError = json_error.into();
|
let json_message = json_error.to_string();
|
||||||
|
let disk_error: DiskError = json_error.into();
|
||||||
|
assert!(
|
||||||
|
disk_error.to_string().contains(&json_message),
|
||||||
|
"the json error message must survive the conversion: {disk_error}"
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -53,15 +53,38 @@ VERIFY_SIGNALS = re.compile(
|
|||||||
r"unreachable!|matches!\(|insta::|proptest!|\.await\?|\)\?|\?;|should_panic"
|
r"unreachable!|matches!\(|insta::|proptest!|\.await\?|\)\?|\?;|should_panic"
|
||||||
)
|
)
|
||||||
DELEGATION = re.compile(
|
DELEGATION = re.compile(
|
||||||
r"\b(?:assert|verify|check|expect|ensure|run)_[a-z0-9_]*\s*\(|"
|
r"\b(?:assert|verify|check|expect|ensure|run)_[a-z0-9_]*(?:::<[^>]*>)?\s*\(|"
|
||||||
r"\b[a-z0-9_]+_(?:case|cases|harness|roundtrip|round_trip)\s*\("
|
r"\b[a-z0-9_]+_(?:case|cases|harness|roundtrip|round_trip)(?:::<[^>]*>)?\s*\("
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# A body whose whole content is one call delegates by construction, whatever the
|
||||||
|
# callee is named: `run(DurabilityMode::Strict).await` and
|
||||||
|
# `aborting_encode_drops_blocked_producer(EncodePipeline::Vec).await` both hand
|
||||||
|
# every assertion to a shared harness.
|
||||||
|
SINGLE_CALL_BODY = re.compile(
|
||||||
|
r"\A\s*[a-zA-Z_][a-zA-Z0-9_:]*(?:::<[^>]*>)?\s*\([^;]*\)\s*(?:\.await\s*)?;?\s*\Z",
|
||||||
|
re.S,
|
||||||
|
)
|
||||||
|
|
||||||
|
# A nested `fn` that is only bound and discarded is a signature guard: the type
|
||||||
|
# system is the assertion, exactly like the `fn _name()` form below.
|
||||||
|
SIGNATURE_GUARD = re.compile(r"\bfn\s+[a-zA-Z0-9_]+\s*(?:<[^>]*>)?\s*\([^;]*\)[^;]*\{", re.S)
|
||||||
|
DISCARDED_BINDING = re.compile(r"\blet\s+_\s*=\s*[a-zA-Z_][a-zA-Z0-9_]*\s*;")
|
||||||
COMPILE_TIME_CHECK = re.compile(r"\bfn\s+_[a-zA-Z0-9_]*\s*(?:<[^>]*>)?\s*\(")
|
COMPILE_TIME_CHECK = re.compile(r"\bfn\s+_[a-zA-Z0-9_]*\s*(?:<[^>]*>)?\s*\(")
|
||||||
TEST_ATTR = re.compile(r"#\[(?:tokio::)?test[\](]")
|
TEST_ATTR = re.compile(r"#\[(?:tokio::)?test[\](]")
|
||||||
TEST_CASE_ATTR = re.compile(r"#\[test_case")
|
TEST_CASE_ATTR = re.compile(r"#\[test_case")
|
||||||
FN_LINE = re.compile(r"^\s*(?:pub\s+)?(?:async\s+)?fn\s+([a-zA-Z0-9_]+)")
|
FN_LINE = re.compile(r"^\s*(?:pub\s+)?(?:async\s+)?fn\s+([a-zA-Z0-9_]+)")
|
||||||
|
|
||||||
|
|
||||||
|
def extract_body(text: str) -> str:
|
||||||
|
"""Return what is between the outermost braces of a scanned function."""
|
||||||
|
start = text.find("{")
|
||||||
|
end = text.rfind("}")
|
||||||
|
if start == -1 or end <= start:
|
||||||
|
return text
|
||||||
|
return text[start + 1 : end]
|
||||||
|
|
||||||
|
|
||||||
def scan_file(path: Path):
|
def scan_file(path: Path):
|
||||||
try:
|
try:
|
||||||
lines = path.read_text(encoding="utf-8").split("\n")
|
lines = path.read_text(encoding="utf-8").split("\n")
|
||||||
@@ -105,7 +128,16 @@ def scan_file(path: Path):
|
|||||||
break
|
break
|
||||||
k += 1
|
k += 1
|
||||||
text = "\n".join(body)
|
text = "\n".join(body)
|
||||||
if not VERIFY_SIGNALS.search(text) and not DELEGATION.search(text) and not COMPILE_TIME_CHECK.search(text):
|
# The attribute block carries verification too: `#[should_panic(expected
|
||||||
|
# = "...")]` makes the panic message the assertion.
|
||||||
|
attr_text = "\n".join(attrs)
|
||||||
|
inner = extract_body(text)
|
||||||
|
delegates = (
|
||||||
|
DELEGATION.search(text)
|
||||||
|
or SINGLE_CALL_BODY.match(inner)
|
||||||
|
or (SIGNATURE_GUARD.search(inner) and DISCARDED_BINDING.search(inner))
|
||||||
|
)
|
||||||
|
if not VERIFY_SIGNALS.search(text) and not VERIFY_SIGNALS.search(attr_text) and not delegates and not COMPILE_TIME_CHECK.search(text):
|
||||||
print(f"{path}:{j + 1}: {name}")
|
print(f"{path}:{j + 1}: {name}")
|
||||||
i = k + 1
|
i = k + 1
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user