test: simulate_workload_varies

[Luc-Mcgrady]

closes #5149

simulate_workload spawns ~30 threads so might be a little expensive for a test in that regard. It seems to work ok though.

[fernandolins]

Hi Luc! Besides the note on your test above, I thought it'd be worth adding a second test too. Here's the final diff I came up with for both — it already includes the change I suggested on your test.

Two things it does:

1. simulate_workload_varies — reworked the assertion so it checks that cost actually grows with desired retention (low vs high buckets), instead of just checking the values differ. This one fails when the fix is reverted, so it truly guards the regression.
2. request_config_uses_requested_retention — a new test for the other half of #5101 ("for normal simulator too"). It pins simulate_request_to_config directly: a card with a stored per-card retention of 0.70 must be converted using the requested 0.95. Since that helper feeds the normal simulator, the workload simulator and optimal-retention computation, this covers all three callers without simulation noise.

I also extracted a small base_request() helper to avoid duplicating the request between the two tests.

diff --git a/rslib/src/scheduler/fsrs/simulator.rs b/rslib/src/scheduler/fsrs/simulator.rs
index 12cfeba74..ba2dad99c 100644
--- a/rslib/src/scheduler/fsrs/simulator.rs
+++ b/rslib/src/scheduler/fsrs/simulator.rs
@@ -366,17 +366,12 @@ impl Card {
 
 #[cfg(test)]
 mod test {
-    use std::collections::HashSet;
-
     use anki_proto::scheduler::SimulateFsrsReviewRequest;
 
     use super::*;
 
-    #[test]
-    fn simulate_workload_varies() {
-        let mut col = Collection::new();
-
-        let req = SimulateFsrsReviewRequest {
+    fn base_request() -> SimulateFsrsReviewRequest {
+        SimulateFsrsReviewRequest {
             params: vec![], // leave empty so fsrs fills defaults in tests
             desired_retention: 0.85f32,
             deck_size: 50u32,
@@ -392,17 +387,84 @@ mod test {
             historical_retention: 0.9f32,
             learning_step_count: 3u32,
             relearning_step_count: 2u32,
-        };
+        }
+    }
 
-        let resp = col.simulate_workload(req).unwrap();
+    #[test]
+    fn simulate_workload_varies() {
+        let mut col = Collection::new();
 
-        // costs are f32; compare bit representations to allow hashing
-        let cost_variants: HashSet<u32> = resp.cost.values().map(|c| c.to_bits()).collect();
+        let resp = col.simulate_workload(base_request()).unwrap();
+
+        // Regression guard for #5101: the workload simulation must apply the per-card
+        // desired retention, so cost has to grow with the target retention. A plain
+        // "the values differ" check is not enough here: even with the bug present the
+        // costs vary due to simulation noise, so we assert the actual trend instead by
+        // comparing the low- and high-retention buckets.
+        let cost = |dr: u32| resp.cost[&dr];
+        let low_avg = (70..=79).map(cost).sum::<f32>() / 10.0;
+        let high_avg = (90..=99).map(cost).sum::<f32>() / 10.0;
 
         assert!(
-            cost_variants.len() > 1,
-            "expected varying workload costs, instead got {:?}",
+            high_avg > low_avg * 1.2,
+            "expected workload cost to grow with desired retention, instead got \
+             low_avg={low_avg}, high_avg={high_avg}, cost={:?}",
             resp.cost
         );
     }
+
+    #[test]
+    fn request_config_uses_requested_retention() -> Result<()> {
+        // Regression guard for the other half of #5101 ("for normal simulator too"):
+        // simulate_request_to_config must apply the *requested* desired retention to
+        // every card, ignoring each card's individually stored value. This helper feeds
+        // the normal simulator, the workload simulator and optimal-retention
+        // computation, so pinning it here covers all three callers without simulation
+        // noise.
+        let mut col = Collection::new();
+        let nt = col.get_notetype_by_name("Basic")?.unwrap();
+        let mut note = nt.new_note();
+        col.add_note(&mut note, DeckId(1))?;
+        let mut card = col
+            .storage
+            .all_cards_of_note(note.id)?
+            .into_iter()
+            .next()
+            .unwrap();
+        card.ctype = CardType::Review;
+        card.queue = CardQueue::Review;
+        card.interval = 100;
+        card.memory_state = Some(FsrsMemoryState {
+            stability: 100.0,
+            difficulty: 5.0,
+        });
+        // Deliberately differs from the requested retention below.
+        card.desired_retention = Some(0.70);
+        card.decay = Some(0.2);
+        col.storage.update_card(&card)?;
+
+        let req = SimulateFsrsReviewRequest {
+            desired_retention: 0.95f32,
+            // Isolate the existing review card: no synthetic new cards.
+            deck_size: 0u32,
+            new_limit: 0u32,
+            ..base_request()
+        };
+
+        let (_config, cards) = col.simulate_request_to_config(&req)?;
+
+        assert!(
+            !cards.is_empty(),
+            "expected the review card to be converted"
+        );
+        assert!(
+            cards
+                .iter()
+                .all(|c| (c.desired_retention - 0.95).abs() < 1e-6),
+            "converted cards must use the requested retention (0.95), instead got {:?}",
+            cards.iter().map(|c| c.desired_retention).collect_vec()
+        );
+
+        Ok(())
+    }
 }

[fernandolins]

Hey! Thanks for adding the test. One thing though: the assertion only checks that the costs differ from each other, and that's already true even without the fix from #5101. I reverted the fix locally and the test still passed, so it doesn't actually guard against the regression.

Looking at the code, here's a suggestion:

let resp = col.simulate_workload(req).unwrap();
// Cost has to grow with the target retention. A plain "the values differ"
// check isn't enough: the costs vary even with the bug present, so we assert
// the actual trend by comparing the low- and high-retention buckets.
let cost = |dr: u32| resp.cost[&dr];
let low_avg = (70..=79).map(cost).sum::<f32>() / 10.0;
let high_avg = (90..=99).map(cost).sum::<f32>() / 10.0;
assert!(
    high_avg > low_avg * 1.2,
    "expected workload cost to grow with desired retention, instead got \
     low_avg={low_avg}, high_avg={high_avg}, cost={:?}",
    resp.cost
);

This one fails when the fix is reverted, so it actually pins the behavior.

[Luc-Mcgrady]

🤔 You're right that the current test doesn't work. The simulator is supposed to be deterministic though. The real problem is that the original bug occurs only for existing cards. e.g if a card has a dr of 0.85 then it was not overwritten by the simulations setting of 0.9. I've added another test to check for this.

[fernandolins]

What do you mean by this comment?

[Luc-Mcgrady]

anki/ts/routes/deck-options/SimulatorModal.svelte

Lines 355 to 359 in 922953a

	<SpinBoxRow bind:value={deckSize} defaultValue={0} min={0} max={100000}>
	<SettingTitle on:click={() => openHelpModal("simulateFsrsReview")}>
	{tr.deckConfigAdditionalNewCardsToSimulate()}
	</SettingTitle>
	</SpinBoxRow>

Nvm the deck_size is named in a counter intuitive way. Since there is 1 card in the deck, I thought the deck_size should be 1.

I confused it with this variable which does also include the existing cards:

anki/rslib/src/scheduler/fsrs/simulator.rs

Line 202 in 09e118b

let deck_size = converted_cards.len();

Maybe this variable should be renamed to "additional_new_cards"?

[Luc-Mcgrady]

Suggested change

	req.deck_size = 0; // ??????? TODO: FIXME should be 1
	req.deck_size = 0;