Address fourth review round: observability, docs accuracy, dep hygiene, tests

- Warn when no timing data was found at all (empty --times-dir previously
  produced empty CSVs without any hint) — best_cars empty + non-empty car
  list now logs a warning in parse_all_times
- Remove the inaccurate "known gap" doc from write_trash_csv: category 1
  (not best on any track) already covers non-final cars, so the
  documented gap never existed
- Gate env_logger behind the bin-logging feature (default-on) and mark
  the records binary as requiring it, so downstream lib consumers do not
  pull env_logger transitively; lib builds clean with --no-default-features
- Switch fetch_car progress output from println! to log::info! so
  RUST_LOG controls it
- atomic_write takes &Path instead of &PathBuf
- Add direct unit tests for is_final_build (sum-8 finals, maxed builds,
  sub-8 non-finals, malformed inputs) and a parsing test covering
  missing/null/non-numeric "t" fields (warn + skip, valid entries still
  parsed)
This commit is contained in:
Jan-Peter von Hunnius 2026-08-15 20:14:53 +02:00
parent 58b8d23e68
commit 1a844e7aa8
4 changed files with 107 additions and 10 deletions

View file

@ -6,7 +6,7 @@
use std::collections::{HashMap, HashSet};
use std::fs;
use std::io::Write;
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use top_drives::{
compare_cars, escape_csv_field, is_final_build, AnalysisContext, AppError, BestCarRecord, Car,
@ -55,7 +55,7 @@ pub fn compare_records(a: &BestCarRecord, b: &BestCarRecord) -> std::cmp::Orderi
/// If the write callback returns an error, the temporary file is cleaned up
/// and no partial file is left at the target path.
fn atomic_write(
path: &PathBuf,
path: &Path,
write_fn: impl FnOnce(&mut fs::File) -> Result<(), AppError>,
) -> Result<(), AppError> {
let tmp_path = PathBuf::from(format!("{}.tmp", path.display()));
@ -179,11 +179,6 @@ pub fn write_very_best_csv(
/// Non-final builds are excluded from category 2 because they
/// belong in `improve.csv` instead. Prize cars and RQ ≥ 80 are always excluded.
///
/// **Known gap:** an owned non-final car that is not best on any track in *any*
/// config appears in neither trash.csv (not final yet) nor improve.csv (no best
/// config to suggest as a target). Such cars stay under the radar until they
/// either reach a final build or are manually reviewed.
///
/// Format: RQ,Brand,Model,Year,Config
pub fn write_trash_csv(
output_dir: &str,

View file

@ -336,6 +336,15 @@ pub async fn parse_all_times(
});
}
// Missing timing files are silent (expected for most cars), so a wrong or
// empty --times-dir would otherwise produce empty CSVs without any hint.
if ctx.best_cars.is_empty() && !car_list.is_empty() {
log::warn!(
"no timing data found in {times_dir} for {} car(s) — all output CSVs will be empty",
car_list.len()
);
}
Ok(car_list)
}
@ -394,7 +403,7 @@ async fn fetch_car(rid: &str, times_dir: &str) -> Result<(), AppError> {
let file_name = rid.to_owned() + ".json";
let url = format!("{CAR_TIMING_API_BASE}{rid}");
let local_file_path = format!("{times_dir}/{file_name}");
println!("Fetching {url}.");
log::info!("Fetching {url}.");
top_drives::get_file(&url, &local_file_path).await
}
@ -544,6 +553,55 @@ mod tests {
assert!(ctx.best_cars.contains_key(&("Monaco Streets".to_string(), 85, "Wet".to_string())));
}
#[test]
fn test_parse_car_times_skips_missing_or_non_numeric_time() {
let ctx = AnalysisContext::new();
let car = Car {
rid: "TestCar_2020".to_string(),
brand: "TestBrand".to_string(),
only_name: "TestModel".to_string(),
year: 2020,
rq: 85,
prize: false,
guid: "guid_test".to_string(),
};
let dir = tempfile::tempdir().unwrap();
let times_dir = dir.path().to_str().unwrap();
let file_path = dir.path().join("TestCar_2020.json");
// "t" missing entirely, null, and a string — all must be skipped
// without panicking or inserting a record.
let json_content = r#"{
"data": {
"111": {
"times": {
"laguna_a00": { "u": "Player1" },
"mnCity_a01": { "t": null, "u": "Player2" },
"testBowl_a02": { "t": "112.45", "u": "Player3" },
"butte_a01": { "t": 50.0, "u": "Player4" }
}
}
}
}"#;
fs::write(&file_path, json_content).unwrap();
let my_cars: HashSet<String> = HashSet::new();
parse_car_times(car, &my_cars, &ctx.best_cars, times_dir).unwrap();
// Only the valid butte entry survives.
assert!(!ctx.best_cars.contains_key(&("Laguna Seca".to_string(), 85, "Dry".to_string())));
assert!(!ctx.best_cars.contains_key(&(
"Monaco Streets".to_string(),
85,
"Wet".to_string()
)));
assert!(
ctx.best_cars.contains_key(&("Butte".to_string(), 85, "Wet".to_string())),
"valid time entry should still be parsed"
);
}
#[test]
fn test_parse_car_times_filters_downvoted() {
let ctx = AnalysisContext::new();

View file

@ -164,3 +164,43 @@ pub fn is_final_build(config: &str) -> bool {
}
bytes.iter().map(|&b| b - b'0').sum::<u8>() >= 8
}
#[cfg(test)]
mod tests {
use super::is_final_build;
#[test]
fn final_builds_have_digit_sum_eight() {
assert!(is_final_build("323"));
assert!(is_final_build("332"));
assert!(is_final_build("233"));
assert!(is_final_build("521"));
assert!(is_final_build("800"));
}
#[test]
fn maxed_builds_beyond_eight_are_final() {
assert!(is_final_build("879"));
assert!(is_final_build("999"));
assert!(is_final_build("990"));
}
#[test]
fn unfinished_builds_below_eight_are_not_final() {
assert!(!is_final_build("111"));
assert!(!is_final_build("221"));
assert!(!is_final_build("610"));
assert!(!is_final_build("070"));
}
#[test]
fn malformed_configs_are_not_final() {
assert!(!is_final_build(""));
assert!(!is_final_build("12"));
assert!(!is_final_build("1234"));
assert!(!is_final_build("abc"));
assert!(!is_final_build("3a3"));
assert!(!is_final_build(" 33"));
assert!(!is_final_build("33 "));
}
}