Address review findings: const guards, qualifying-count regression test, owned keys

- Add compile-time const assertions (const _: () block, stable on 1.95)
  for MAX_RQ, MIN_VALID_TIME, TIME_TOLERANCE, TEST_BOWL_FACTOR, and
  CONCURRENCY_LIMIT so bad constant edits fail the build; remove the
  stale comment that incorrectly claimed compile-time validation existed
- Add regression test for the qualifying-count fix: car with 3 configs
  where only 2 meet min_count and 2 copies suffice to cover the
  qualifying ones (previously phantom-wishlisted via overcounting)
- Use owned String keys in qualifying_per_guid to decouple the map from
  guid_to_rid's allocations
- Clarify write_improve_csv doc (exactly one row per car), restore the
  remove.csv test section header, add trailing newline to .gitignore
This commit is contained in:
Jan-Peter von Hunnius 2026-08-15 19:07:03 +02:00
parent 2ab66a249b
commit 39d1e1d71b
3 changed files with 84 additions and 16 deletions

2
.gitignore vendored
View file

@ -6,4 +6,4 @@ tracks_repo.json
times/ times/
*.csv *.csv
AGENTS.md AGENTS.md

View file

@ -304,7 +304,8 @@ pub fn compute_ideal_wishlist(
// Number of qualifying configs (track count >= min_count) per guid, // Number of qualifying configs (track count >= min_count) per guid,
// pre-computed once so the loop below does not revisit guid_to_rid per record. // pre-computed once so the loop below does not revisit guid_to_rid per record.
let qualifying_per_guid: HashMap<&str, usize> = counts // Owned keys decouple the map from guid_to_rid's allocations.
let qualifying_per_guid: HashMap<String, usize> = counts
.guid_to_rid .guid_to_rid
.iter() .iter()
.map(|(guid, keys)| { .map(|(guid, keys)| {
@ -312,7 +313,7 @@ pub fn compute_ideal_wishlist(
.iter() .iter()
.filter(|key| counts.by_rid.get(*key).is_some_and(|(_, count)| *count >= min_count)) .filter(|key| counts.by_rid.get(*key).is_some_and(|(_, count)| *count >= min_count))
.count(); .count();
(guid.as_str(), qualifying) (guid.clone(), qualifying)
}) })
.collect(); .collect();
@ -323,8 +324,7 @@ pub fn compute_ideal_wishlist(
if owned.contains(&record.guid) { if owned.contains(&record.guid) {
let copies = counts.copy_count.get(&record.guid).copied().unwrap_or(1); let copies = counts.copy_count.get(&record.guid).copied().unwrap_or(1);
let qualifying_configs = let qualifying_configs = qualifying_per_guid.get(&record.guid).copied().unwrap_or(0);
qualifying_per_guid.get(record.guid.as_str()).copied().unwrap_or(0);
if copies >= qualifying_configs && qualifying_configs > 0 { if copies >= qualifying_configs && qualifying_configs > 0 {
// Player has enough copies to cover all qualifying configs → skip // Player has enough copies to cover all qualifying configs → skip
@ -498,11 +498,12 @@ pub fn write_wishlist_files(
/// to determine whether each copy is finished. Cars whose copies are all final builds are /// to determine whether each copy is finished. Cars whose copies are all final builds are
/// skipped entirely. /// skipped entirely.
/// ///
/// For each car with an unfinished copy, one row is emitted showing the best remaining /// For each car with an unfinished copy, exactly **one row** is emitted: the best
/// target config (highest track count), excluding configs the player already owns as final /// remaining target config (highest track count), excluding configs the player
/// builds on another copy of the same car. The `Target` column shows the ideal final build /// already owns as final builds on another copy of the same car. The `Target`
/// from the timing data (not derived from the player's current config). The `Count` column /// column shows the ideal final build from the timing data (not derived from the
/// shows how many tracks this car is best on under that specific normalized config. /// player's current config). The `Count` column shows how many tracks this car is
/// best on under that specific normalized config.
/// ///
/// Format: RQ,Brand,Model,Year,Target,Count /// Format: RQ,Brand,Model,Year,Target,Count
pub fn write_improve_csv(output_dir: &str, counts: &CarCounts) -> Result<usize, AppError> { pub fn write_improve_csv(output_dir: &str, counts: &CarCounts) -> Result<usize, AppError> {
@ -1289,6 +1290,7 @@ mod tests {
} }
// ======================================================================= // =======================================================================
// remove.csv tests
// remove.csv tests for cars not in best cars data // remove.csv tests for cars not in best cars data
// ======================================================================= // =======================================================================
@ -1476,4 +1478,66 @@ mod tests {
assert_eq!(wishlist.len(), 0, "3 copies / 3 qualifying → should NOT be wishlisted"); assert_eq!(wishlist.len(), 0, "3 copies / 3 qualifying → should NOT be wishlisted");
} }
#[test]
fn test_wishlist_qualifying_count_only_counts_min_count_configs() {
// Regression test: qualifying_configs must count only configs whose track
// count meets min_count, not ALL entries in guid_to_rid.
//
// Setup: car has 3 configs in the best-cars data —
// 332: 15 tracks (qualifies, min_count = 10)
// 323: 3 tracks (does NOT qualify)
// 233: 12 tracks (qualifies)
// Player owns 2 copies, both tuned 323 (the non-qualifying config).
//
// Old buggy behavior: qualifying_configs = 3 (all guid_to_rid entries) →
// 2 < 3 → car stays eligible → phantom wishlist entry.
// Correct behavior: qualifying_configs = 2 → 2 >= 2 → skip: the player
// has enough copies to cover every qualifying config.
let ctx = AnalysisContext::new();
let mut rec_332 = make_record("Car_A", 85, 90.0, "guid_a");
rec_332.config = "332".to_string();
rec_332.normalized_config = "332".to_string();
let mut rec_323 = make_record("Car_A", 85, 95.0, "guid_a");
rec_323.config = "323".to_string();
rec_323.normalized_config = "323".to_string();
let mut rec_233 = make_record("Car_A", 85, 100.0, "guid_a");
rec_233.config = "233".to_string();
rec_233.normalized_config = "233".to_string();
for i in 0..15 {
ctx.best_cars
.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec_332.clone()]);
}
for i in 15..18 {
ctx.best_cars
.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec_323.clone()]);
}
for i in 18..30 {
ctx.best_cars
.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec_233.clone()]);
}
let car_list = vec![make_car("Car_A", 85, "guid_a")];
let mut owned_configs = HashMap::new();
owned_configs.insert("guid_a".to_string(), vec!["323".to_string(), "323".to_string()]);
let mut copy_count = HashMap::new();
copy_count.insert("guid_a".to_string(), 2); // 2 copies, 2 qualifying configs
let counts = compute_car_counts(&ctx, car_list, owned_configs, copy_count);
let owned: HashSet<String> = vec!["guid_a".to_string()].into_iter().collect();
let wishlist = compute_ideal_wishlist(&counts, &owned, 10, 300);
assert_eq!(
wishlist.len(),
0,
"2 copies / 2 qualifying (3 total) → should NOT be wishlisted"
);
}
} }

View file

@ -46,6 +46,16 @@ pub const TEST_BOWL_FACTOR: f64 = 2000.0;
/// since each concurrent permit holder also occupies a `spawn_blocking` thread. /// since each concurrent permit holder also occupies a `spawn_blocking` thread.
pub const CONCURRENCY_LIMIT: usize = 20; pub const CONCURRENCY_LIMIT: usize = 20;
// Compile-time guards: a bad edit to any constant above fails the build here
// instead of producing subtly wrong behavior at runtime.
const _: () = {
assert!(MAX_RQ > 0);
assert!(MIN_VALID_TIME > 0.0);
assert!(TIME_TOLERANCE > 0.0);
assert!(TEST_BOWL_FACTOR > 0.0);
assert!(CONCURRENCY_LIMIT > 0);
};
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
// Core parsing: per-car timing data // Core parsing: per-car timing data
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
@ -736,10 +746,4 @@ mod tests {
ctx.best_cars.get(&("Monaco Streets".to_string(), 90, "Wet".to_string())).unwrap(); ctx.best_cars.get(&("Monaco Streets".to_string(), 90, "Wet".to_string())).unwrap();
assert_eq!(records[0].config, "323", "bare config should pass through"); assert_eq!(records[0].config, "323", "bare config should pass through");
} }
// =======================================================================
// Constants tests
// =======================================================================
// Constants are compile-time validated - explicit tests removed as they
// check literal values that cannot fail unless the constants themselves are edited
} }