logaritmiskandClaude Opus 5
507894dae7
Three unrelated small defects, all requiring signature changes: - `Game::one_v_one` hardcoded `GameOptions::default()`, so a 1v1 could never set `p_draw` or convergence options — and a drawn 1v1 was therefore unreachable through it, since the default `p_draw` is zero. It now takes `&GameOptions` like every other constructor. - `Observer::on_batch_processed` was declared on the trait and never called from anywhere: implementors wired up a callback that could not fire. It is now called after each slice sweep, and renamed `on_slice_processed` to match the vocabulary the codebase adopted in T2 — the unit of work is a `TimeSlice`, not a batch. A slice is swept once travelling backward and once forward, so a multi-slice history fires it twice per slice per iteration; the doc comment says so. - `pub mod factors` sat beside `pub(crate) mod factor`, two module paths differing by one character with only one of them importable. The public facade is now `graph`. Tests cover each as a behaviour rather than a compile check: a drawn 1v1 succeeds only when p_draw is supplied, and the observer tests fail if any callback stops firing. BREAKING CHANGE: `Game::one_v_one` takes a fourth `&GameOptions` argument; `Observer::on_batch_processed` is renamed `on_slice_processed`; the `factors` module is renamed `graph`. Closes #21 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
106 lines
3.3 KiB
Rust
106 lines
3.3 KiB
Rust
//! `Observer` callbacks must actually fire.
|
|
//!
|
|
//! `on_slice_processed` (formerly `on_batch_processed`) was declared on the
|
|
//! trait and never called from anywhere, so implementors wired up a callback
|
|
//! that could not run. These tests exist so that cannot silently recur.
|
|
|
|
use std::sync::{Arc, Mutex};
|
|
|
|
use trueskill_tt::{History, Observer};
|
|
|
|
/// `History` takes its observer by value and never hands it back, so a test
|
|
/// that wants to read what was recorded shares the storage rather than the
|
|
/// observer: the handles are cloned, the buffers are not.
|
|
#[derive(Clone, Default)]
|
|
struct Recorder {
|
|
iterations: Arc<Mutex<Vec<usize>>>,
|
|
slices: Arc<Mutex<Vec<(i64, usize, usize)>>>,
|
|
converged: Arc<Mutex<Vec<(usize, bool)>>>,
|
|
}
|
|
|
|
impl Observer<i64> for Recorder {
|
|
fn on_iteration_end(&self, iter: usize, _max_step: (f64, f64)) {
|
|
self.iterations.lock().unwrap().push(iter);
|
|
}
|
|
|
|
fn on_slice_processed(&self, time: &i64, slice_idx: usize, n_events: usize) {
|
|
self.slices
|
|
.lock()
|
|
.unwrap()
|
|
.push((*time, slice_idx, n_events));
|
|
}
|
|
|
|
fn on_converged(&self, iters: usize, _final_step: (f64, f64), converged: bool) {
|
|
self.converged.lock().unwrap().push((iters, converged));
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn every_observer_callback_fires() {
|
|
let recorder = Recorder::default();
|
|
let mut h = History::builder().observer(recorder.clone()).build();
|
|
|
|
h.record_winner(&"a", &"b", 1).unwrap();
|
|
h.record_winner(&"b", &"c", 2).unwrap();
|
|
h.record_winner(&"c", &"a", 3).unwrap();
|
|
h.converge().unwrap();
|
|
|
|
assert!(
|
|
!recorder.iterations.lock().unwrap().is_empty(),
|
|
"on_iteration_end never fired"
|
|
);
|
|
assert!(
|
|
!recorder.converged.lock().unwrap().is_empty(),
|
|
"on_converged never fired"
|
|
);
|
|
assert!(
|
|
!recorder.slices.lock().unwrap().is_empty(),
|
|
"on_slice_processed never fired — the defect this test exists for"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn slice_callbacks_report_the_slice_they_swept() {
|
|
let recorder = Recorder::default();
|
|
let mut h = History::builder().observer(recorder.clone()).build();
|
|
|
|
h.record_winner(&"a", &"b", 10).unwrap();
|
|
h.record_winner(&"a", &"b", 20).unwrap();
|
|
h.converge().unwrap();
|
|
|
|
let slices = recorder.slices.lock().unwrap();
|
|
|
|
// Only the times actually in the history, and each with its own events.
|
|
for &(time, idx, events) in slices.iter() {
|
|
assert!(time == 10 || time == 20, "unexpected slice time {time}");
|
|
assert!(idx < 2, "slice index {idx} out of range");
|
|
assert_eq!(events, 1, "each slice holds exactly one event");
|
|
}
|
|
|
|
// Both slices must be reported, not just one end of the sweep.
|
|
assert!(
|
|
slices.iter().any(|&(t, ..)| t == 10),
|
|
"slice 10 never reported"
|
|
);
|
|
assert!(
|
|
slices.iter().any(|&(t, ..)| t == 20),
|
|
"slice 20 never reported"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn a_single_slice_history_still_reports_its_sweep() {
|
|
let recorder = Recorder::default();
|
|
let mut h = History::builder().observer(recorder.clone()).build();
|
|
|
|
h.record_winner(&"a", &"b", 1).unwrap();
|
|
h.converge().unwrap();
|
|
|
|
let slices = recorder.slices.lock().unwrap();
|
|
assert!(
|
|
!slices.is_empty(),
|
|
"the single-slice path must report its sweep too"
|
|
);
|
|
assert!(slices.iter().all(|&(t, idx, _)| t == 1 && idx == 0));
|
|
}
|