logaritmisk/trueskill-tt
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Activity

Test suite has no coverage for degenerate inputs, release-mode behaviour, or non-finite results #26

New Issue
Closed
opened 2026-08-04 19:21:40 +00:00 by logaritmisk · 4 comments
logaritmisk commented 2026-08-04 19:21:40 +00:00
Owner
Copy Link
Copy Source

The existing suite is strong where it aims: numerical goldens against TrueSkillThroughTime.py/.jl, an equivalence layer (tests/equivalence.rs), a thread-count determinism check (tests/determinism.rs), and a targeted NaN regression (tests/large_history_converges_finite.rs). Every bug filed in this audit sits outside that aim.

The shape of the gap

Every test exercises a valid, non-degenerate, two-or-three-team input in a debug build. So:

Bug Why no test caught it
#8 (NaN on draws with p_draw=0) All draw tests pass p_draw = 0.25; the guard is a debug_assert! never exercised in release
#9 (quality panics at 3+ groups) test_quality uses exactly 2 groups
#11 (empty history panics) No test converges an empty history
#19 (online mode inert) No test sets online(true)
#14 (quadratic incremental add) Benchmarks only measure batched construction
#17 (memory scales with index magnitude) No memory assertions anywhere

What to add

Degenerate and boundary inputs — currently untested end to end:

  • Empty history; single-event history; single-competitor event
  • Ties under every p_draw setting, including 0.0 (#8)
  • Outcome::winner(w, n) for n in 3..=5 (implicit ties among losers)
  • quality/predict_quality at 1, 3, and 10 groups; empty groups; unknown keys (#9)
  • Zero and negative weights; weights length mismatches
  • Events at identical timestamps; out-of-order timestamps; negative elapsed
  • Very large and very small beta/sigma/score_sigma

Release-mode runs. Everything protected by debug_assert! is untested in the configuration users ship. cargo test --release should be a first-class job (#24) — several existing tests would need to keep passing there, and the validation tests from #18 only mean anything there.

A finiteness invariant. Given #8 and the large_history_converges_finite.rs precedent, a shared helper asserting every posterior is finite and non-NaN — applied across the whole suite, not one regression file — would have caught #8, #12, and #19 at once.

Property-based tests. The engine has invariants that generate well: posteriors are finite for any valid input; log-evidence is ≤ 0 and finite; convergence is order-independent for disjoint events; skill totals are conserved under symmetric outcomes; color-group partitions never place two events sharing an index in the same color. proptest or quickcheck as a dev-dependency would cover a large input space cheaply.

Performance regressions. Benchmarks exist but nothing fails on regression — benches/baseline.txt and benches/scored_baseline.txt are committed and compared by hand via the Justfile. The incremental-add path has no benchmark at all (#14), which is why its quadratic went unnoticed.

Doc tests. Zero, since there are no doc examples (#25).

Acceptance

  • A degenerate-input test module covering the table above.
  • cargo test --release in CI, green.
  • A finiteness assertion helper used across the numerical tests.
  • Property tests for at least the finiteness and color-group-disjointness invariants.

Individual regression tests for each bug belong in that bug's own issue; this one is about the systematic gaps that let all of them through.

The existing suite is strong where it aims: numerical goldens against TrueSkillThroughTime.py/.jl, an equivalence layer (`tests/equivalence.rs`), a thread-count determinism check (`tests/determinism.rs`), and a targeted NaN regression (`tests/large_history_converges_finite.rs`). Every bug filed in this audit sits outside that aim. ## The shape of the gap Every test exercises a **valid, non-degenerate, two-or-three-team** input in a **debug** build. So: | Bug | Why no test caught it | |---|---| | #8 (NaN on draws with `p_draw=0`) | All draw tests pass `p_draw = 0.25`; the guard is a `debug_assert!` never exercised in release | | #9 (`quality` panics at 3+ groups) | `test_quality` uses exactly 2 groups | | #11 (empty history panics) | No test converges an empty history | | #19 (`online` mode inert) | No test sets `online(true)` | | #14 (quadratic incremental add) | Benchmarks only measure batched construction | | #17 (memory scales with index magnitude) | No memory assertions anywhere | ## What to add **Degenerate and boundary inputs** — currently untested end to end: - Empty history; single-event history; single-competitor event - Ties under every `p_draw` setting, including `0.0` (#8) - `Outcome::winner(w, n)` for `n` in 3..=5 (implicit ties among losers) - `quality`/`predict_quality` at 1, 3, and 10 groups; empty groups; unknown keys (#9) - Zero and negative weights; `weights` length mismatches - Events at identical timestamps; out-of-order timestamps; negative elapsed - Very large and very small `beta`/`sigma`/`score_sigma` **Release-mode runs.** Everything protected by `debug_assert!` is untested in the configuration users ship. `cargo test --release` should be a first-class job (#24) — several existing tests would need to keep passing there, and the validation tests from #18 only mean anything there. **A finiteness invariant.** Given #8 and the `large_history_converges_finite.rs` precedent, a shared helper asserting every posterior is finite and non-NaN — applied across the whole suite, not one regression file — would have caught #8, #12, and #19 at once. **Property-based tests.** The engine has invariants that generate well: posteriors are finite for any valid input; log-evidence is ≤ 0 and finite; convergence is order-independent for disjoint events; skill totals are conserved under symmetric outcomes; color-group partitions never place two events sharing an index in the same color. `proptest` or `quickcheck` as a dev-dependency would cover a large input space cheaply. **Performance regressions.** Benchmarks exist but nothing fails on regression — `benches/baseline.txt` and `benches/scored_baseline.txt` are committed and compared by hand via the `Justfile`. The incremental-add path has no benchmark at all (#14), which is why its quadratic went unnoticed. **Doc tests.** Zero, since there are no doc examples (#25). ## Acceptance - A degenerate-input test module covering the table above. - `cargo test --release` in CI, green. - A finiteness assertion helper used across the numerical tests. - Property tests for at least the finiteness and color-group-disjointness invariants. Individual regression tests for each bug belong in that bug's own issue; this one is about the systematic gaps that let all of them through.
logaritmisk commented 2026-08-04 20:13:26 +00:00
Author
Owner
Copy Link
Copy Source

Partly done — staying open for property tests and the remaining boundary cases.

Added three integration files, all of which fail against the pre-fix code:

  • tests/degenerate_inputs.rs — empty history, empty event stream, single-event history, ties at every p_draw, Outcome::winner(_, n) for n in 3..=5, non-positive score_sigma, 12-team finiteness, a 1200-team chain that underflowed the old linear evidence product, and a ±5000-mu near-certain outcome.
  • tests/ingestion_equivalence.rs — same events, different batching, same converged answer. This is the one that caught the real bug in #14, which the entire reference-validated golden suite had missed because every golden ingests in a single call with one timestamp per event. It asserts convergence before comparing, which matters: my first version compared at 1e-9 under the default 30-iteration cap and surfaced a 1.5e-6 "discrepancy" that was only the convergence residual.
  • tests/quality.rs — N-group quality by invariant (permutation invariance, monotonicity under imbalance, range) plus the degenerate-input panics.

Release-mode runs are now a first-class CI job (#24) — the single highest-value item on the list, since every debug_assert!-guarded defect was invisible to a debug-only suite.

Test count went from 136 to 179, plus 3 doc tests where there were none.

Still open:

  • Property-based tests. Not started. The invariants named above generate well — finiteness for any valid input, log-evidence ≤ 0 and finite, order-independence for disjoint events, color-group disjointness — and would want proptest as a dev-dependency.
  • A shared finiteness helper applied suite-wide. assert_finite exists in degenerate_inputs.rs but is local to that file.
  • Remaining boundary inputs: zero and negative weights, weights length mismatches, out-of-order and duplicate timestamps, extreme beta/sigma.
  • Benchmark regression gating. benches/ingest.rs now covers the previously unmeasured incremental path, but nothing still fails on regression — the baselines are compared by hand.
Partly done — **staying open** for property tests and the remaining boundary cases. Added three integration files, all of which fail against the pre-fix code: - **`tests/degenerate_inputs.rs`** — empty history, empty event stream, single-event history, ties at every `p_draw`, `Outcome::winner(_, n)` for n in 3..=5, non-positive `score_sigma`, 12-team finiteness, a 1200-team chain that underflowed the old linear evidence product, and a ±5000-mu near-certain outcome. - **`tests/ingestion_equivalence.rs`** — same events, different batching, same converged answer. This is the one that caught the real bug in #14, which the entire reference-validated golden suite had missed because every golden ingests in a single call with one timestamp per event. It asserts convergence *before* comparing, which matters: my first version compared at 1e-9 under the default 30-iteration cap and surfaced a 1.5e-6 "discrepancy" that was only the convergence residual. - **`tests/quality.rs`** — N-group quality by invariant (permutation invariance, monotonicity under imbalance, range) plus the degenerate-input panics. Release-mode runs are now a first-class CI job (#24) — the single highest-value item on the list, since every `debug_assert!`-guarded defect was invisible to a debug-only suite. Test count went from 136 to 179, plus 3 doc tests where there were none. **Still open:** - **Property-based tests.** Not started. The invariants named above generate well — finiteness for any valid input, log-evidence ≤ 0 and finite, order-independence for disjoint events, color-group disjointness — and would want `proptest` as a dev-dependency. - **A shared finiteness helper applied suite-wide.** `assert_finite` exists in `degenerate_inputs.rs` but is local to that file. - **Remaining boundary inputs:** zero and negative weights, weights length mismatches, out-of-order and duplicate timestamps, extreme `beta`/`sigma`. - **Benchmark regression gating.** `benches/ingest.rs` now covers the previously unmeasured incremental path, but nothing still *fails* on regression — the baselines are compared by hand.
logaritmisk commented 2026-08-27 16:06:43 +00:00
Author
Owner
Copy Link
Copy Source

Three of the four remaining items done — 2745fbb. Staying open for the fourth, which needs a decision rather than work.

Property-based tests — tests/properties.rs, proptest as a dev-dependency. Four invariants over generated 1v1 schedules:

  • converged posteriors are always finite with positive sigma
  • log-evidence, batch and filtered, is finite and never above zero
  • filtered evidence is invariant to whether converge has run
  • one-at-a-time ingestion reaches the same fixed point as batched

The invariance property is mutation-proved: making filtered_step read skill.forward instead of the carried message fails it with -1.1038430064192069 -> -1.1135747072822761. The others are structural rather than mutation-proved.

Shared finiteness helper — tests/common/mod.rs. assert_finite also rejects a non-positive sigma now, which the old local version let through: Gaussian::sigma reports a non-positive precision as improper rather than trapping, so a collapsed posterior passed a finite-only check.

Boundary inputs — zero and negative weights, out-of-order timestamps, extreme beta/sigma.

Two things worth recording from that last group:

  • Zero weight reaches a division by zero and survives it. (m - performance.exclude(player.performance() * w)) * (1.0 / w) with w == 0.0 is 1.0 / 0.0, and the commit is accepted today — yet the posterior comes out finite. The test pins that observed behaviour rather than asserting what ought to happen, and says so. If you would rather zero and negative weights were rejected at ingestion, that is a separate change and I would want your call on it.
  • The weight tests nearly shipped vacuous. As first written they returned early if commit errored, so they would have silently stopped testing anything the moment weight validation changed. I checked by turning the early return into a failure and confirming it did not fire; they now expect() the commit instead.

Weights length mismatches — also on your list — were covered separately in 1ac3b21 while working #18, and are enforced in release now rather than by debug_assert!.

Still open: benchmark regression gating. Nothing fails on a regression; baselines are still compared by hand. I did use one during #17 (built a pre17 baseline from HEAD~1, then compared — it caught a real +2.4% on Batch::iteration against ~-20% on the convergence benchmarks), so the workflow works manually.

Automating it needs a threshold chosen against how noisy the runner actually is, and criterion does not fail a build on its own — it would want critcmp or cargo-criterion plus a policy for what counts as a regression. That is a judgment about tolerance for false CI failures on a shared runner, so I have left it to you. If you want it, tell me the threshold and whether a regression should fail the build or just annotate the run.

Test binaries: 56 → 60. MSRV 1.85 verified with proptest in the dependency graph.

Three of the four remaining items done — `2745fbb`. **Staying open** for the fourth, which needs a decision rather than work. **Property-based tests** — `tests/properties.rs`, proptest as a dev-dependency. Four invariants over generated 1v1 schedules: - converged posteriors are always finite with positive sigma - log-evidence, batch and filtered, is finite and never above zero - filtered evidence is invariant to whether `converge` has run - one-at-a-time ingestion reaches the same fixed point as batched The invariance property is mutation-proved: making `filtered_step` read `skill.forward` instead of the carried message fails it with `-1.1038430064192069 -> -1.1135747072822761`. The others are structural rather than mutation-proved. **Shared finiteness helper** — `tests/common/mod.rs`. `assert_finite` also rejects a **non-positive sigma** now, which the old local version let through: `Gaussian::sigma` reports a non-positive precision as improper rather than trapping, so a collapsed posterior passed a finite-only check. **Boundary inputs** — zero and negative weights, out-of-order timestamps, extreme beta/sigma. Two things worth recording from that last group: - **Zero weight reaches a division by zero** and survives it. `(m - performance.exclude(player.performance() * w)) * (1.0 / w)` with `w == 0.0` is `1.0 / 0.0`, and the commit is accepted today — yet the posterior comes out finite. The test pins that observed behaviour rather than asserting what ought to happen, and says so. If you would rather zero and negative weights were rejected at ingestion, that is a separate change and I would want your call on it. - **The weight tests nearly shipped vacuous.** As first written they returned early if `commit` errored, so they would have silently stopped testing anything the moment weight validation changed. I checked by turning the early return into a failure and confirming it did not fire; they now `expect()` the commit instead. Weights length mismatches — also on your list — were covered separately in `1ac3b21` while working #18, and are enforced in release now rather than by `debug_assert!`. --- **Still open: benchmark regression gating.** Nothing fails on a regression; baselines are still compared by hand. I did use one during #17 (built a `pre17` baseline from `HEAD~1`, then compared — it caught a real +2.4% on `Batch::iteration` against ~-20% on the convergence benchmarks), so the workflow works manually. Automating it needs a threshold chosen against how noisy the runner actually is, and criterion does not fail a build on its own — it would want `critcmp` or `cargo-criterion` plus a policy for what counts as a regression. That is a judgment about tolerance for false CI failures on a shared runner, so I have left it to you. If you want it, tell me the threshold and whether a regression should fail the build or just annotate the run. Test binaries: 56 → 60. MSRV 1.85 verified with proptest in the dependency graph.
logaritmisk added the testing label 2026-09-07 13:52:05 +00:00
logaritmisk commented 2026-09-08 13:54:33 +00:00
Author
Owner
Copy Link
Copy Source

All four acceptance criteria are met, and the per-bug table is fully covered.

  • Degenerate-input module: tests/degenerate_inputs.rs (25 tests), plus tests/validation.rs and tests/quality.rs.
  • Release mode in CI: .gitea/workflows/ci.yml has a release matrix leg running cargo test and cargo test --doc; just test runs it too.
  • Shared finiteness helper: tests/common/mod.rs::assert_finite, which also rejects a non-positive sigma.
  • Property tests: tests/properties.rs has four proptest invariants.

Doc tests went from 0 to 13.

The one that was not just a coverage gap

Your "what to add" list included a single-competitor event. Read as a single-team event, it was untested — and it did not merely lack a test, it panicked, in release, from History::add_events. Details in #18; fixed in 7c6965c, covered by tests/ingestion_shape.rs. The test you asked for would have gone red the day it was written, which is the best argument for the list you wrote.

What remains

Three named gaps, all smaller and more specific than what this issue asked for:

  1. No test asserts converge() returns Err(NonFiniteResult). The name appears in tests/ only inside a doc comment. This is the sub-claim in the issue title and it is the thinnest-covered part. The NaN-score path is now rejected at ingestion, so a NonFiniteResult test needs a different route to a non-finite state.
  2. No color-group disjointness property test. Your fourth criterion named it explicitly. There are five hand-written unit tests in src/color_group.rs and a contiguity debug_assert!, but nothing generated. Needs an in-crate proptest, since ColorGroups is not public.
  3. common::assert_finite is used by 2 of 24 suites. You asked for it "applied across the whole suite".

Also still open from your last comment: benchmark regression gating. benches/ has two committed baselines but just still compares them by hand — no critcmp, no failing threshold. That is gated on your false-positive tolerance rather than on work.

Those four are a different issue from the one you filed. Suggest closing this as delivered and refiling them as a narrow "test gaps" issue, unless you would rather keep the thread. Your call.

All four acceptance criteria are met, and the per-bug table is fully covered. - Degenerate-input module: `tests/degenerate_inputs.rs` (25 tests), plus `tests/validation.rs` and `tests/quality.rs`. - Release mode in CI: `.gitea/workflows/ci.yml` has a `release` matrix leg running `cargo test` and `cargo test --doc`; `just test` runs it too. - Shared finiteness helper: `tests/common/mod.rs::assert_finite`, which also rejects a non-positive sigma. - Property tests: `tests/properties.rs` has four proptest invariants. Doc tests went from 0 to 13. ## The one that was not just a coverage gap Your "what to add" list included a **single-competitor event**. Read as a single-*team* event, it was untested — and it did not merely lack a test, it **panicked**, in release, from `History::add_events`. Details in #18; fixed in `7c6965c`, covered by `tests/ingestion_shape.rs`. The test you asked for would have gone red the day it was written, which is the best argument for the list you wrote. ## What remains Three named gaps, all smaller and more specific than what this issue asked for: 1. **No test asserts `converge()` returns `Err(NonFiniteResult)`.** The name appears in `tests/` only inside a doc comment. This is the sub-claim in the issue *title* and it is the thinnest-covered part. The NaN-score path is now rejected at ingestion, so a `NonFiniteResult` test needs a different route to a non-finite state. 2. **No color-group disjointness property test.** Your fourth criterion named it explicitly. There are five hand-written unit tests in `src/color_group.rs` and a contiguity `debug_assert!`, but nothing generated. Needs an in-crate proptest, since `ColorGroups` is not public. 3. **`common::assert_finite` is used by 2 of 24 suites.** You asked for it "applied across the whole suite". Also still open from your last comment: **benchmark regression gating**. `benches/` has two committed baselines but `just` still compares them by hand — no `critcmp`, no failing threshold. That is gated on your false-positive tolerance rather than on work. Those four are a different issue from the one you filed. Suggest closing this as delivered and refiling them as a narrow "test gaps" issue, unless you would rather keep the thread. Your call.
logaritmisk commented 2026-09-08 19:15:19 +00:00
Author
Owner
Copy Link
Copy Source

Closing as delivered. The two remaining test gaps are filled in 4e9aa6b; the benchmark item is #54.

NonFiniteResult

It had no test at all — the name appeared in tests/ only inside a doc comment, and it was the sub-claim in this issue's own title.

Investigating it turned out to be the interesting part. The obvious routes to a non-finite state are now closed at the boundary, so my first guess was that the variant had become unreachable. It has not, and the reachable cases are better than the ones I would have written: finite, individually legal inputs that overflow inside inference.

sigma 1e300        -> NonFiniteResult
beta 1e300         -> NonFiniteResult
sigma 1e-300       -> NonFiniteResult
score_sigma 1e-300 -> NonFiniteResult
scores +/-1e308    -> NonFiniteResult

That is the honest boundary: input validation rejects things that are not numbers, and NonFiniteResult catches numbers that stop being finite partway through. tests/non_finite_results.rs pins all five, plus that converge_partial does not launder a breakdown into an Ok — it is permissive about stopping short, not about NaN — and a control asserting merely extreme parameters (1e6, 1e-6, 1e150) still converge, so the suite cannot pass by always failing.

Color-group disjointness

Your fourth criterion, and the one with only five hand-written cases. Now a proptest over three shapes: a dense pool where collisions force colors to multiply, a sparse one where most events are independent, and repeated members within one event.

Two of my first assertions were wrong about the code rather than the reverse, and both are worth recording:

  • A competitor named twice within a single event is not a collision. color_greedy collects each event's members into a set for exactly that reason, and my check was accumulating without deduplicating per event.
  • Contiguity is not a property of color_greedy. It holds only after recompute_color_groups reorders events so each color occupies one range. I asserted it directly on color_greedy's output and it failed on almost every input.

The test now asserts what is actually promised: that the reorder is always possible — relabelling events in group order yields contiguous groups. That is what the parallel sweep needs, since it slices &mut sub-ranges from them and overlapping ranges would be unsound.

The one criterion I am closing without doing

common::assert_finite is used by 2 of 24 suites, and you asked for it "applied across the whole suite". I have not done that, deliberately.

Most of those suites assert exact values or tolerances — assert_ulps_eq!(x, 3.14) already implies finiteness and is strictly stronger. Adding assert_finite beside it asserts nothing new and makes the test longer. The helper is used where finiteness is the property under test, which is properties.rs and degenerate_inputs.rs, and the generative coverage in properties.rs is doing the work blanket adoption was meant to.

If you disagree, it is a mechanical hour and I will do it — but I would rather say I skipped it than quietly leave a criterion unaddressed.

Doc tests went 0 → 15 over the course of this.

Closing as delivered. The two remaining test gaps are filled in `4e9aa6b`; the benchmark item is #54. ## `NonFiniteResult` It had no test at all — the name appeared in `tests/` only inside a doc comment, and it was the sub-claim in this issue's own title. Investigating it turned out to be the interesting part. The obvious routes to a non-finite state are now closed at the boundary, so my first guess was that the variant had become unreachable. It has not, and the reachable cases are better than the ones I would have written: **finite, individually legal inputs that overflow inside inference.** ``` sigma 1e300 -> NonFiniteResult beta 1e300 -> NonFiniteResult sigma 1e-300 -> NonFiniteResult score_sigma 1e-300 -> NonFiniteResult scores +/-1e308 -> NonFiniteResult ``` That is the honest boundary: input validation rejects things that are not numbers, and `NonFiniteResult` catches numbers that stop being finite partway through. `tests/non_finite_results.rs` pins all five, plus that `converge_partial` does not launder a breakdown into an `Ok` — it is permissive about stopping short, not about NaN — and a control asserting merely extreme parameters (1e6, 1e-6, 1e150) still converge, so the suite cannot pass by always failing. ## Color-group disjointness Your fourth criterion, and the one with only five hand-written cases. Now a proptest over three shapes: a dense pool where collisions force colors to multiply, a sparse one where most events are independent, and repeated members within one event. Two of my first assertions were wrong about the code rather than the reverse, and both are worth recording: - A competitor named twice *within a single event* is not a collision. `color_greedy` collects each event's members into a set for exactly that reason, and my check was accumulating without deduplicating per event. - **Contiguity is not a property of `color_greedy`.** It holds only after `recompute_color_groups` reorders events so each color occupies one range. I asserted it directly on `color_greedy`'s output and it failed on almost every input. The test now asserts what is actually promised: that the reorder is always *possible* — relabelling events in group order yields contiguous groups. That is what the parallel sweep needs, since it slices `&mut` sub-ranges from them and overlapping ranges would be unsound. ## The one criterion I am closing without doing `common::assert_finite` is used by 2 of 24 suites, and you asked for it "applied across the whole suite". I have not done that, deliberately. Most of those suites assert exact values or tolerances — `assert_ulps_eq!(x, 3.14)` already implies finiteness and is strictly stronger. Adding `assert_finite` beside it asserts nothing new and makes the test longer. The helper is used where finiteness *is* the property under test, which is `properties.rs` and `degenerate_inputs.rs`, and the generative coverage in `properties.rs` is doing the work blanket adoption was meant to. If you disagree, it is a mechanical hour and I will do it — but I would rather say I skipped it than quietly leave a criterion unaddressed. Doc tests went 0 → 15 over the course of this.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
api
Shape or ergonomics of the public surface
 blocked
Waiting on another issue or an external decision
 breaking
Resolving it changes the public API; wants a major bump
 bug
Behaviour is wrong, including silently wrong
 decision
Needs a call to be made, not code to be written
 docs
Documentation, including doc comments and the README
 enhancement
New capability the crate does not have
 infra
CI, release process, packaging
 numerics
Floating-point accuracy, cancellation, convergence maths
 performance
Speed or memory
 refactor
Internal cleanup with no intended behaviour change
 testing
Test coverage and test infrastructure
No labels testing
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logaritmisk
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#26