diff --git a/crates/io-metrics/src/adaptive_ttl.rs b/crates/io-metrics/src/adaptive_ttl.rs index f544515eb..c07a10987 100644 --- a/crates/io-metrics/src/adaptive_ttl.rs +++ b/crates/io-metrics/src/adaptive_ttl.rs @@ -315,6 +315,44 @@ impl Default for AccessTracker { mod tests { use super::*; + /// Replaces the per-helper smoke tests that called the record_* helpers + /// and asserted nothing: the calls (same literals) now run against a local + /// DebuggingRecorder and every metric name the helpers own must actually + /// be emitted (rustfs/backlog#1836 PR3). + #[test] + fn record_helpers_emit_their_metrics() { + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + record_ttl_adjustment("test-key", 100, 150); + record_ttl_adjustment("test-key", 100, 50); + record_ttl_expiration(); + record_early_eviction("cold"); + record_early_eviction("low_priority"); + record_access_pattern_change("sequential", "random"); + record_access_pattern_change("random", "sequential"); + }); + + let emitted: std::collections::HashSet = snapshotter + .snapshot() + .into_vec() + .into_iter() + .map(|(composite, _, _, _)| composite.key().name().to_string()) + .collect(); + for expected in [ + "rustfs_cache_ttl_adjustments", + "rustfs_cache_ttl_base", + "rustfs_cache_ttl_adjusted", + "rustfs_cache_ttl_extensions", + "rustfs_cache_ttl_reductions", + "rustfs_cache_ttl_expirations", + "rustfs_cache_evictions_early", + "rustfs_cache_access_pattern_changes", + ] { + assert!(emitted.contains(expected), "{expected} must be emitted by its record helper"); + } + } + #[test] fn test_adaptive_ttl_stats() { let mut stats = AdaptiveTTLStats::new(); @@ -335,30 +373,6 @@ mod tests { assert!((stats.reduction_rate() - 0.3333333333333333).abs() < 0.01); } - #[test] - fn test_record_ttl_adjustment() { - // This test verifies the function compiles and runs - record_ttl_adjustment("test-key", 100, 150); - record_ttl_adjustment("test-key", 100, 50); - } - - #[test] - fn test_record_ttl_expiration() { - record_ttl_expiration(); - } - - #[test] - fn test_record_early_eviction() { - record_early_eviction("cold"); - record_early_eviction("low_priority"); - } - - #[test] - fn test_record_access_pattern_change() { - record_access_pattern_change("sequential", "random"); - record_access_pattern_change("random", "sequential"); - } - #[test] fn test_access_record() { let mut record = AccessRecord::new(); diff --git a/crates/io-metrics/src/backpressure_metrics.rs b/crates/io-metrics/src/backpressure_metrics.rs index f519ee846..2391e8dc2 100644 --- a/crates/io-metrics/src/backpressure_metrics.rs +++ b/crates/io-metrics/src/backpressure_metrics.rs @@ -53,30 +53,38 @@ pub fn record_backpressure_deactivation() { mod tests { use super::*; + /// Replaces the per-helper smoke tests that called the record_* helpers + /// and asserted nothing: the calls (same literals) now run against a local + /// DebuggingRecorder and every metric name the helpers own must actually + /// be emitted (rustfs/backlog#1836 PR3). #[test] - fn test_record_backpressure_state_change() { - record_backpressure_state_change("normal", "warning"); - record_backpressure_state_change("warning", "critical"); - } + fn record_helpers_emit_their_metrics() { + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + record_backpressure_state_change("normal", "warning"); + record_backpressure_state_change("warning", "critical"); + record_backpressure_rejection(); + record_concurrent_operations(10); + record_concurrent_operations(32); + record_backpressure_activation(); + record_backpressure_deactivation(); + }); - #[test] - fn test_record_backpressure_rejection() { - record_backpressure_rejection(); - } - - #[test] - fn test_record_concurrent_operations() { - record_concurrent_operations(10); - record_concurrent_operations(32); - } - - #[test] - fn test_record_backpressure_activation() { - record_backpressure_activation(); - } - - #[test] - fn test_record_backpressure_deactivation() { - record_backpressure_deactivation(); + let emitted: std::collections::HashSet = snapshotter + .snapshot() + .into_vec() + .into_iter() + .map(|(composite, _, _, _)| composite.key().name().to_string()) + .collect(); + for expected in [ + "rustfs_backpressure_state_changes", + "rustfs_backpressure_rejections", + "rustfs_backpressure_concurrent", + "rustfs_backpressure_activations", + "rustfs_backpressure_deactivations", + ] { + assert!(emitted.contains(expected), "{expected} must be emitted by its record helper"); + } } } diff --git a/crates/io-metrics/src/deadlock_metrics.rs b/crates/io-metrics/src/deadlock_metrics.rs index b79d08227..c8dfbe9b0 100644 --- a/crates/io-metrics/src/deadlock_metrics.rs +++ b/crates/io-metrics/src/deadlock_metrics.rs @@ -72,39 +72,48 @@ pub fn record_wait_edge_removed() { mod tests { use super::*; + /// Replaces the per-helper smoke tests that called the record_* helpers + /// and asserted nothing: the calls (same literals) now run against a local + /// DebuggingRecorder and every metric name the helpers own must actually + /// be emitted (rustfs/backlog#1836 PR3). #[test] - fn test_record_deadlock_detected() { - record_deadlock_detected(3); - record_deadlock_detected(5); - } + fn record_helpers_emit_their_metrics() { + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + record_deadlock_detected(3); + record_deadlock_detected(5); + record_long_held_lock(1, Duration::from_secs(30)); + record_long_held_lock(2, Duration::from_secs(60)); + record_lock_acquisition("mutex"); + record_lock_acquisition("rwlock"); + record_lock_release("mutex", Duration::from_millis(10)); + record_lock_release("rwlock", Duration::from_millis(5)); + record_lock_contention("mutex"); + record_lock_contention("rwlock"); + record_wait_edge_added(); + record_wait_edge_removed(); + }); - #[test] - fn test_record_long_held_lock() { - record_long_held_lock(1, Duration::from_secs(30)); - record_long_held_lock(2, Duration::from_secs(60)); - } - - #[test] - fn test_record_lock_acquisition() { - record_lock_acquisition("mutex"); - record_lock_acquisition("rwlock"); - } - - #[test] - fn test_record_lock_release() { - record_lock_release("mutex", Duration::from_millis(10)); - record_lock_release("rwlock", Duration::from_millis(5)); - } - - #[test] - fn test_record_lock_contention() { - record_lock_contention("mutex"); - record_lock_contention("rwlock"); - } - - #[test] - fn test_record_wait_edge() { - record_wait_edge_added(); - record_wait_edge_removed(); + let emitted: std::collections::HashSet = snapshotter + .snapshot() + .into_vec() + .into_iter() + .map(|(composite, _, _, _)| composite.key().name().to_string()) + .collect(); + for expected in [ + "rustfs_deadlock_detected_total", + "rustfs_deadlock_cycle_length", + "rustfs_deadlock_long_held", + "rustfs_deadlock_hold_time_secs", + "rustfs_lock_acquisitions", + "rustfs_lock_releases", + "rustfs_lock_hold_time_secs", + "rustfs_lock_contentions", + "rustfs_deadlock_wait_edges_added", + "rustfs_deadlock_wait_edges_removed", + ] { + assert!(emitted.contains(expected), "{expected} must be emitted by its record helper"); + } } } diff --git a/crates/io-metrics/src/io_metrics.rs b/crates/io-metrics/src/io_metrics.rs index 54ed9561b..ec61b59dd 100644 --- a/crates/io-metrics/src/io_metrics.rs +++ b/crates/io-metrics/src/io_metrics.rs @@ -169,46 +169,58 @@ impl IoSchedulerStats { mod tests { use super::*; + /// Replaces the per-helper smoke tests that called the record_* helpers + /// and asserted nothing: the calls (same literals) now run against a local + /// DebuggingRecorder and every metric name the helpers own must actually + /// be emitted (rustfs/backlog#1836 PR3). #[test] - fn test_record_io_scheduler_decision() { - record_io_scheduler_decision(128 * 1024, "low", "sequential"); - record_io_scheduler_decision(64 * 1024, "high", "random"); - } + fn record_helpers_emit_their_metrics() { + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + record_io_scheduler_decision(128 * 1024, "low", "sequential"); + record_io_scheduler_decision(64 * 1024, "high", "random"); + record_io_priority_decision("high", 1024); + record_io_priority_decision("normal", 1024 * 1024); + record_io_priority_decision("low", 10 * 1024 * 1024); + record_load_level_change("low", "medium"); + record_load_level_change("medium", "high"); + record_bandwidth_observation(100 * 1024 * 1024); + record_bandwidth_observation(500 * 1024 * 1024); + record_buffer_size_adjustment(128 * 1024, 64 * 1024, "concurrency"); + record_buffer_size_adjustment(128 * 1024, 256 * 1024, "sequential"); + record_queue_operation("enqueue", "high", 10); + record_queue_operation("dequeue", "high", 9); + record_starvation_event("low"); + }); - #[test] - fn test_record_io_priority_decision() { - record_io_priority_decision("high", 1024); - record_io_priority_decision("normal", 1024 * 1024); - record_io_priority_decision("low", 10 * 1024 * 1024); - } - - #[test] - fn test_record_load_level_change() { - record_load_level_change("low", "medium"); - record_load_level_change("medium", "high"); - } - - #[test] - fn test_record_bandwidth_observation() { - record_bandwidth_observation(100 * 1024 * 1024); - record_bandwidth_observation(500 * 1024 * 1024); - } - - #[test] - fn test_record_buffer_size_adjustment() { - record_buffer_size_adjustment(128 * 1024, 64 * 1024, "concurrency"); - record_buffer_size_adjustment(128 * 1024, 256 * 1024, "sequential"); - } - - #[test] - fn test_record_queue_operation() { - record_queue_operation("enqueue", "high", 10); - record_queue_operation("dequeue", "high", 9); - } - - #[test] - fn test_record_starvation_event() { - record_starvation_event("low"); + let emitted: std::collections::HashSet = snapshotter + .snapshot() + .into_vec() + .into_iter() + .map(|(composite, _, _, _)| composite.key().name().to_string()) + .collect(); + for expected in [ + "rustfs_io_scheduler_decisions", + "rustfs_io_scheduler_buffer_size", + "rustfs_io_scheduler_load", + "rustfs_io_scheduler_strategy", + "rustfs_io_scheduler_buffer_size_histogram", + "rustfs_io_priority_decisions", + "rustfs_io_priority_by_level", + "rustfs_io_priority_request_size", + "rustfs_io_load_changes", + "rustfs_io_bandwidth_bps", + "rustfs_io_bandwidth_histogram", + "rustfs_io_buffer_adjustments", + "rustfs_io_buffer_original", + "rustfs_io_buffer_adjusted", + "rustfs_io_queue_operations", + "rustfs_io_queue_size", + "rustfs_io_starvation_events", + ] { + assert!(emitted.contains(expected), "{expected} must be emitted by its record helper"); + } } #[test] diff --git a/crates/io-metrics/src/lock_metrics.rs b/crates/io-metrics/src/lock_metrics.rs index 663510b06..ae517ac75 100644 --- a/crates/io-metrics/src/lock_metrics.rs +++ b/crates/io-metrics/src/lock_metrics.rs @@ -163,6 +163,46 @@ impl LockMetricsSummary { #[cfg(test)] mod tests { use super::*; + + /// Replaces the per-helper smoke tests that called the record_* helpers + /// and asserted nothing: the calls (same literals) now run against a local + /// DebuggingRecorder and every metric name the helpers own must actually + /// be emitted (rustfs/backlog#1836 PR3). + #[test] + fn record_helpers_emit_their_metrics() { + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + record_lock_optimization_enabled(true); + record_lock_optimization_enabled(false); + record_spin_attempt(true); + record_spin_attempt(false); + record_spin_count_change(100); + record_spin_count_change(200); + record_lock_hold_time(Duration::from_millis(10)); + record_lock_hold_time(Duration::from_millis(100)); + record_early_release(); + record_contention_event(); + }); + + let emitted: std::collections::HashSet = snapshotter + .snapshot() + .into_vec() + .into_iter() + .map(|(composite, _, _, _)| composite.key().name().to_string()) + .collect(); + for expected in [ + "rustfs_lock_optimization_enabled", + "rustfs_lock_spin_successes", + "rustfs_lock_spin_failures", + "rustfs_lock_spin_count", + "rustfs_lock_hold_time_secs", + "rustfs_lock_early_releases", + "rustfs_lock_contentions", + ] { + assert!(emitted.contains(expected), "{expected} must be emitted by its record helper"); + } + } use metrics::{Counter, CounterFn, Gauge, GaugeFn, Histogram, HistogramFn, Key, KeyName, Metadata, SharedString, Unit}; use std::sync::{Arc, Mutex}; @@ -255,40 +295,6 @@ mod tests { fn record(&self, _value: f64) {} } - #[test] - fn test_record_lock_optimization_enabled() { - record_lock_optimization_enabled(true); - record_lock_optimization_enabled(false); - } - - #[test] - fn test_record_spin_attempt() { - record_spin_attempt(true); - record_spin_attempt(false); - } - - #[test] - fn test_record_spin_count_change() { - record_spin_count_change(100); - record_spin_count_change(200); - } - - #[test] - fn test_record_lock_hold_time() { - record_lock_hold_time(Duration::from_millis(10)); - record_lock_hold_time(Duration::from_millis(100)); - } - - #[test] - fn test_record_early_release() { - record_early_release(); - } - - #[test] - fn test_record_contention_event() { - record_contention_event(); - } - #[test] fn test_record_object_lock_diag_enabled() { let recorder = SeenMetricsRecorder::default(); diff --git a/crates/io-metrics/src/timeout_metrics.rs b/crates/io-metrics/src/timeout_metrics.rs index 436616f46..bddbf2edb 100644 --- a/crates/io-metrics/src/timeout_metrics.rs +++ b/crates/io-metrics/src/timeout_metrics.rs @@ -114,39 +114,46 @@ impl TimeoutMetricsSummary { mod tests { use super::*; + /// Replaces the per-helper smoke tests that called the record_* helpers + /// and asserted nothing: the calls (same literals) now run against a local + /// DebuggingRecorder and every metric name the helpers own must actually + /// be emitted (rustfs/backlog#1836 PR3). #[test] - fn test_record_timeout_event() { - record_timeout_event("get_object"); - record_timeout_event("put_object"); - } + fn record_helpers_emit_their_metrics() { + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + metrics::with_local_recorder(&recorder, || { + record_timeout_event("get_object"); + record_timeout_event("put_object"); + record_operation_duration("get_object", Duration::from_millis(100)); + record_operation_duration("put_object", Duration::from_millis(500)); + record_dynamic_timeout(1024 * 1024, Duration::from_secs(10)); + record_dynamic_timeout(100 * 1024 * 1024, Duration::from_secs(30)); + record_operation_progress("get_object", 50.0); + record_operation_progress("get_object", 100.0); + record_stalled_operation("get_object"); + record_operation_completion("get_object", true); + record_operation_completion("get_object", false); + }); - #[test] - fn test_record_operation_duration() { - record_operation_duration("get_object", Duration::from_millis(100)); - record_operation_duration("put_object", Duration::from_millis(500)); - } - - #[test] - fn test_record_dynamic_timeout() { - record_dynamic_timeout(1024 * 1024, Duration::from_secs(10)); - record_dynamic_timeout(100 * 1024 * 1024, Duration::from_secs(30)); - } - - #[test] - fn test_record_operation_progress() { - record_operation_progress("get_object", 50.0); - record_operation_progress("get_object", 100.0); - } - - #[test] - fn test_record_stalled_operation() { - record_stalled_operation("get_object"); - } - - #[test] - fn test_record_operation_completion() { - record_operation_completion("get_object", true); - record_operation_completion("get_object", false); + let emitted: std::collections::HashSet = snapshotter + .snapshot() + .into_vec() + .into_iter() + .map(|(composite, _, _, _)| composite.key().name().to_string()) + .collect(); + for expected in [ + "rustfs_io_timeout_events_total", + "rustfs_io_operation_duration_seconds", + "rustfs_timeout_dynamic_size", + "rustfs_timeout_dynamic_secs", + "rustfs_timeout_dynamic_size_histogram", + "rustfs_operation_progress", + "rustfs_operation_stalled", + "rustfs_operation_completions", + ] { + assert!(emitted.contains(expected), "{expected} must be emitted by its record helper"); + } } #[test] diff --git a/scripts/find_assertless_tests.py b/scripts/find_assertless_tests.py new file mode 100755 index 000000000..90e52a21f --- /dev/null +++ b/scripts/find_assertless_tests.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 +# Copyright 2024 RustFS Team +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Census of assertion-less tests (rustfs/backlog#1836 PR3). + +Flags `#[test]` / `#[tokio::test]` functions whose bodies contain no +verification signal: no assert!/assert_eq!/assert_ne!/panic! macro, no +`.expect(`/`.unwrap(`, no `?` operator, no `#[should_panic]`, and no +`insta` snapshot / proptest / matches! usage. Such a test is green no +matter what the code under test does. + +This is a heuristic REVIEW QUEUE, not a lint: a hit still needs human +reading before it is fixed or deleted, because assertions may live in a +called helper. Known false-positive classes are excluded up front: + +- `#[test_case(...)]`-driven functions (the values are the assertion's + parameters; the assert lives in the shared body — still scanned, but a + body that asserts is not flagged anyway; the exclusion covers wrappers + that only delegate to a suite runner). +- Functions whose body calls a helper with `assert`, `verify`, `check`, + `expect`, `run_` or `_case` in its name (suite-delegation pattern). + +Usage: + scripts/find_assertless_tests.py [path ...] # default: crates rustfs/src + +Exit code is always 0; the output is the queue. +""" + +import re +import sys +from pathlib import Path + +VERIFY_SIGNALS = re.compile( + r"assert!|assert_eq!|assert_ne!|debug_assert|panic!\(|\.expect\(|\.unwrap\(|" + r"unreachable!|matches!\(|insta::|proptest!|\.await\?|\)\?|\?;|should_panic" +) +DELEGATION = re.compile(r"\b[a-z0-9_]*(?:assert|verify|check|expect|run_case|_case|harness|round_trip|roundtrip)[a-z0-9_]*\s*\(") +TEST_ATTR = re.compile(r"#\[(?:tokio::)?test[\](]") +TEST_CASE_ATTR = re.compile(r"#\[test_case") +FN_LINE = re.compile(r"^\s*(?:pub\s+)?(?:async\s+)?fn\s+([a-zA-Z0-9_]+)") + + +def scan_file(path: Path): + try: + lines = path.read_text(encoding="utf-8").split("\n") + except (UnicodeDecodeError, OSError): + return + i = 0 + while i < len(lines): + if not TEST_ATTR.search(lines[i]): + i += 1 + continue + # collect the whole attribute block (may include #[serial], #[test_case], ...) + attrs = [] + j = i + while j < len(lines) and (lines[j].strip().startswith("#[") or lines[j].strip().startswith("//")): + attrs.append(lines[j]) + j += 1 + if j >= len(lines): + break + m = FN_LINE.match(lines[j]) + if not m: + i = j + 1 + continue + name = m.group(1) + if any(TEST_CASE_ATTR.search(a) for a in attrs): + i = j + 1 + continue + # brace-match the body + depth = 0 + begun = False + body = [] + k = j + while k < len(lines): + for ch in lines[k]: + if ch == "{": + depth += 1 + begun = True + elif ch == "}": + depth -= 1 + body.append(lines[k]) + if begun and depth <= 0: + break + k += 1 + text = "\n".join(body) + if not VERIFY_SIGNALS.search(text) and not DELEGATION.search(text): + print(f"{path}:{j + 1}: {name}") + i = k + 1 + + +def main(): + roots = [Path(p) for p in (sys.argv[1:] or ["crates", "rustfs/src"])] + for root in roots: + for path in sorted(root.rglob("*.rs")): + if "target" in path.parts: + continue + scan_file(path) + + +if __name__ == "__main__": + main()