`cargo clippy --all-targets` went from 51 warnings (23 in the lib) to zero. Most were mechanical — needless borrows, `assert_eq!` against a bool literal, `vec!` where an array does, `or_insert_with(Vec::new)`, a loop index used only to index — and were applied with `clippy --fix`, then reviewed line by line. That review caught one auto-fix that was *not* semantically neutral: dropping the redundant `use hostname;` left its `#[cfg(target_os = "linux")]` orphaned directly above `SERVICE_NAME`, which would have silently cfg'd the constant out of every non-Linux build. Removed the stray attribute with the import. Where a lint asked for a risky change rather than a better one, it is suppressed with a comment saying why: - `too_many_arguments` on five `#[tauri::command]` handlers and `ThumbnailCache::save_thumbnail` — most of the arity is `State<'_, _>` injection, and a parameter struct would change the IPC contract and the generated TypeScript for no readability gain. - `large_enum_variant` on `PlayerStatusEvent` and `AutoplayDecision` — both are serde + specta wire types emitted a handful of times a second, never bulk allocated; boxing would have to stay invisible to the generated bindings while every match arm gained a deref. - `await_holding_lock` on the `hybrid`/`offline` test modules — the guard is a test-only serialisation lock for the process-global `INCLUDE_CATALOG_BROWSE` flag, and the await it spans *is* the critical section. Each `#[tokio::test]` gets its own single-threaded runtime, so this is not the production deadlock class the lint targets; restructuring would reintroduce the flag race. Real fixes elsewhere: `JellyfinItem::to_media_item` takes `self` by value, so it is now `into_media_item`; the five-tuple episode row in the download commands has a named `EpisodeRow` alias; the mpv `PropertyChange` arm matches `name: "pause"` instead of guarding on it. Also converted the last 27 raw `.lock().unwrap()` call sites to `lock_safe()`, completing the `MutexSafe`/`RwLockSafe` convention. All of them turned out to be in test modules — production code was already clean — so this is consistency rather than a fix. The two raw locks in `utils/lock.rs` stay raw on purpose: those tests deliberately poison a mutex to prove the helpers recover from it. Pure refactoring: all 698 tests still pass.
352 lines
12 KiB
Rust
352 lines
12 KiB
Rust
//! Tests for MpvBackend to prevent regressions
|
|
//!
|
|
//! These tests are designed to catch common issues like:
|
|
//! - Tokio runtime panics when spawning async tasks from std::thread
|
|
//! - Position update thread failures
|
|
//! - Event emission issues
|
|
//!
|
|
//! TRACES: UR-003, UR-004 | IR-003 | IT-003, IT-004
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use crate::utils::lock::MutexSafe;
|
|
use std::sync::{Arc, Mutex};
|
|
use tokio::sync::Mutex as TokioMutex;
|
|
|
|
/// Test that simulates the position update thread spawning async tasks
|
|
/// without a Tokio runtime (the bug we just fixed)
|
|
#[test]
|
|
fn test_position_thread_handles_missing_tokio_runtime() {
|
|
use std::sync::atomic::{AtomicBool, Ordering};
|
|
|
|
let success = Arc::new(AtomicBool::new(false));
|
|
let success_clone = success.clone();
|
|
|
|
// Spawn a regular thread (no Tokio runtime)
|
|
let handle = std::thread::spawn(move || {
|
|
// This simulates what the position update thread does
|
|
// It should handle the case where there's no Tokio runtime
|
|
|
|
// Try to get the current Tokio runtime handle
|
|
if let Ok(handle) = tokio::runtime::Handle::try_current() {
|
|
// We have a runtime, use it
|
|
handle.spawn(async move {
|
|
// Async work here
|
|
});
|
|
} else {
|
|
// No runtime, spawn a new thread with its own runtime
|
|
// This is the fix we applied
|
|
std::thread::spawn(move || {
|
|
let rt = tokio::runtime::Runtime::new().unwrap();
|
|
rt.block_on(async move {
|
|
// Async work here
|
|
success_clone.store(true, Ordering::SeqCst);
|
|
});
|
|
});
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
// Give the spawned thread time to complete
|
|
std::thread::sleep(std::time::Duration::from_millis(100));
|
|
|
|
assert!(
|
|
success.load(Ordering::SeqCst),
|
|
"Should successfully execute async code from std::thread without panicking"
|
|
);
|
|
}
|
|
|
|
/// Test that the Tokio runtime fallback pattern works correctly
|
|
#[test]
|
|
fn test_tokio_runtime_fallback_pattern() {
|
|
let counter = Arc::new(Mutex::new(0));
|
|
let counter_clone = counter.clone();
|
|
|
|
// Spawn from a regular thread (no runtime)
|
|
let handle = std::thread::spawn(move || {
|
|
if let Ok(handle) = tokio::runtime::Handle::try_current() {
|
|
// Has runtime (shouldn't happen in this test)
|
|
handle.spawn(async move {
|
|
*counter_clone.lock_safe() += 1;
|
|
});
|
|
} else {
|
|
// No runtime - use fallback (should happen in this test)
|
|
std::thread::spawn(move || {
|
|
let rt = tokio::runtime::Runtime::new().unwrap();
|
|
rt.block_on(async move {
|
|
*counter_clone.lock_safe() += 1;
|
|
});
|
|
});
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
// Wait for async task to complete
|
|
std::thread::sleep(std::time::Duration::from_millis(100));
|
|
|
|
let count = *counter.lock_safe();
|
|
assert_eq!(
|
|
count, 1,
|
|
"Fallback pattern should execute async code successfully"
|
|
);
|
|
}
|
|
|
|
/// Test that position update logic works in a thread
|
|
#[test]
|
|
fn test_position_update_in_thread() {
|
|
use std::time::Duration;
|
|
|
|
let positions = Arc::new(Mutex::new(Vec::new()));
|
|
let positions_clone = positions.clone();
|
|
|
|
// Simulate the position update thread
|
|
let handle = std::thread::spawn(move || {
|
|
for i in 0..5 {
|
|
std::thread::sleep(Duration::from_millis(10));
|
|
|
|
// Simulate getting position from player
|
|
let position = i as f64 * 0.25;
|
|
|
|
// Store position (simulating event emission)
|
|
positions_clone.lock_safe().push(position);
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
let recorded_positions = positions.lock_safe();
|
|
assert_eq!(
|
|
recorded_positions.len(),
|
|
5,
|
|
"Should have recorded 5 position updates"
|
|
);
|
|
|
|
// Verify positions are increasing
|
|
for (i, pos) in recorded_positions.iter().enumerate() {
|
|
let expected = i as f64 * 0.25;
|
|
assert!(
|
|
(*pos - expected).abs() < 0.001,
|
|
"Position {} should be close to {}",
|
|
pos,
|
|
expected
|
|
);
|
|
}
|
|
}
|
|
|
|
/// Test async progress reporting pattern
|
|
#[tokio::test]
|
|
async fn test_progress_reporting_with_tokio_mutex() {
|
|
use crate::playback_reporting::{EventThrottler, PlaybackReporter};
|
|
|
|
// Create mock reporter (None for this test)
|
|
let reporter = Arc::new(TokioMutex::new(None::<PlaybackReporter>));
|
|
let throttler = Arc::new(EventThrottler::new());
|
|
|
|
// Simulate progress reporting
|
|
let item_id = "test_item_123".to_string();
|
|
|
|
// This should not panic even though reporter is None
|
|
let reporter_guard = reporter.lock().await;
|
|
if let Some(_reporter_instance) = reporter_guard.as_ref() {
|
|
// Would report here if reporter was configured
|
|
} else {
|
|
// Reporter not configured - this is OK
|
|
}
|
|
drop(reporter_guard);
|
|
|
|
// Verify throttler works
|
|
assert!(
|
|
throttler.should_report(&item_id),
|
|
"First report should be allowed"
|
|
);
|
|
|
|
throttler.mark_reported(&item_id);
|
|
|
|
// Immediate second report should be throttled
|
|
// (EventThrottler has internal logic for this)
|
|
}
|
|
|
|
/// Test that position updates are emitted even when paused (for scrubbing)
|
|
/// This is critical for UI responsiveness when seeking while paused
|
|
#[test]
|
|
fn test_position_updates_while_paused() {
|
|
use std::sync::atomic::{AtomicUsize, Ordering};
|
|
use std::time::Duration;
|
|
|
|
let update_count = Arc::new(AtomicUsize::new(0));
|
|
let update_count_clone = update_count.clone();
|
|
|
|
// Simulate a position update thread that runs regardless of pause state
|
|
let handle = std::thread::spawn(move || {
|
|
// Simulate 5 position updates
|
|
for _ in 0..5 {
|
|
std::thread::sleep(Duration::from_millis(50));
|
|
|
|
// In the real implementation, we check position from MPV
|
|
// and emit PositionUpdate events even when paused
|
|
// This simulates that behavior:
|
|
let _is_paused = true; // Simulating paused state
|
|
|
|
// Key: We DON'T skip the update when paused
|
|
// This allows scrubbing to work
|
|
update_count_clone.fetch_add(1, Ordering::SeqCst);
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
let final_count = update_count.load(Ordering::SeqCst);
|
|
assert_eq!(
|
|
final_count, 5,
|
|
"Position updates should be emitted even when paused (got {} updates)",
|
|
final_count
|
|
);
|
|
}
|
|
|
|
/// Test that progress reporting is skipped when paused
|
|
/// Progress reporting to the server should only happen during active playback
|
|
#[test]
|
|
fn test_progress_reporting_skipped_when_paused() {
|
|
use std::sync::atomic::{AtomicUsize, Ordering};
|
|
use std::time::Duration;
|
|
|
|
let report_count = Arc::new(AtomicUsize::new(0));
|
|
let report_count_clone = report_count.clone();
|
|
|
|
// Simulate the progress reporting logic
|
|
let handle = std::thread::spawn(move || {
|
|
// Simulate 5 update cycles
|
|
for i in 0..5 {
|
|
std::thread::sleep(Duration::from_millis(50));
|
|
|
|
// Position updates are emitted (tested separately)
|
|
// But progress reporting depends on pause state
|
|
let is_paused = i % 2 == 0; // Alternate between paused and playing
|
|
|
|
// Key: Only report when NOT paused
|
|
if !is_paused {
|
|
report_count_clone.fetch_add(1, Ordering::SeqCst);
|
|
}
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
let final_count = report_count.load(Ordering::SeqCst);
|
|
assert_eq!(
|
|
final_count, 2,
|
|
"Progress reporting should only happen when not paused (got {} reports)",
|
|
final_count
|
|
);
|
|
}
|
|
|
|
/// Test that position updates are suppressed briefly after a seek
|
|
/// This prevents "jumping to zero" visual glitches during seek operations
|
|
#[test]
|
|
fn test_position_updates_suppressed_after_seek() {
|
|
use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
|
|
use std::time::{Duration, SystemTime, UNIX_EPOCH};
|
|
|
|
let last_seek_time = Arc::new(AtomicU64::new(0));
|
|
let last_seek_time_clone = last_seek_time.clone();
|
|
let update_count = Arc::new(AtomicUsize::new(0));
|
|
let update_count_clone = update_count.clone();
|
|
|
|
// Simulate a seek happening
|
|
let seek_time = SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.unwrap()
|
|
.as_millis() as u64;
|
|
last_seek_time.store(seek_time, Ordering::Relaxed);
|
|
|
|
// Simulate position update thread
|
|
let handle = std::thread::spawn(move || {
|
|
// Try 2 position updates at 50ms intervals (well within the 150ms window)
|
|
for _ in 0..2 {
|
|
std::thread::sleep(Duration::from_millis(50));
|
|
|
|
// Check if we should suppress updates (within 150ms of seek)
|
|
let now = SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.unwrap()
|
|
.as_millis() as u64;
|
|
let last_seek = last_seek_time_clone.load(Ordering::Relaxed);
|
|
let time_since_seek = now.saturating_sub(last_seek);
|
|
|
|
if time_since_seek < 150 {
|
|
// Suppress update (don't increment counter)
|
|
continue;
|
|
}
|
|
|
|
// Emit update
|
|
update_count_clone.fetch_add(1, Ordering::SeqCst);
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
let final_count = update_count.load(Ordering::SeqCst);
|
|
// With 50ms intervals and 150ms suppression window, updates at 50ms and 100ms
|
|
// should both be suppressed
|
|
assert_eq!(
|
|
final_count, 0,
|
|
"Position updates should be suppressed within 150ms of seek (got {} updates)",
|
|
final_count
|
|
);
|
|
}
|
|
|
|
/// Test that position updates resume after seek suppression window
|
|
#[test]
|
|
fn test_position_updates_resume_after_seek_window() {
|
|
use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
|
|
use std::time::{Duration, SystemTime, UNIX_EPOCH};
|
|
|
|
let last_seek_time = Arc::new(AtomicU64::new(0));
|
|
let last_seek_time_clone = last_seek_time.clone();
|
|
let update_count = Arc::new(AtomicUsize::new(0));
|
|
let update_count_clone = update_count.clone();
|
|
|
|
// Simulate a seek that happened 200ms ago (past the suppression window)
|
|
let seek_time = SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.unwrap()
|
|
.as_millis() as u64
|
|
- 200; // 200ms ago
|
|
last_seek_time.store(seek_time, Ordering::Relaxed);
|
|
|
|
// Simulate position update thread
|
|
let handle = std::thread::spawn(move || {
|
|
// Try 3 position updates
|
|
for _ in 0..3 {
|
|
std::thread::sleep(Duration::from_millis(10));
|
|
|
|
// Check if we should suppress updates (within 150ms of seek)
|
|
let now = SystemTime::now()
|
|
.duration_since(UNIX_EPOCH)
|
|
.unwrap()
|
|
.as_millis() as u64;
|
|
let last_seek = last_seek_time_clone.load(Ordering::Relaxed);
|
|
let time_since_seek = now.saturating_sub(last_seek);
|
|
|
|
if time_since_seek < 150 {
|
|
continue; // Should not happen in this test
|
|
}
|
|
|
|
// Emit update
|
|
update_count_clone.fetch_add(1, Ordering::SeqCst);
|
|
}
|
|
});
|
|
|
|
handle.join().unwrap();
|
|
|
|
let final_count = update_count.load(Ordering::SeqCst);
|
|
assert_eq!(
|
|
final_count, 3,
|
|
"Position updates should resume after seek suppression window (got {} updates)",
|
|
final_count
|
|
);
|
|
}
|
|
}
|