Skip to content

Repair the unit-test suite and wire it into CI#15

Open
nh13 wants to merge 2 commits into
learnedsystems:masterfrom
nh13:fix-unit-tests-ci
Open

Repair the unit-test suite and wire it into CI#15
nh13 wants to merge 2 commits into
learnedsystems:masterfrom
nh13:fix-unit-tests-ci

Conversation

@nh13

@nh13 nh13 commented Jun 13, 2026

Copy link
Copy Markdown

The crate's #[cfg(test)] modules had stopped compiling — they still used the old ModelData API and by-value ModelInput conversion, so cargo test failed with ~50 errors and none of the unit tests ran. Since CI only runs the tests/ integration Makefile (cargo build, not cargo test), this went unnoticed. This migrates every test to RMITrainingData / &ModelInput, rewrites the two mod.rs tests that used removed helpers against set_scale/iter, and adds a cargo test -p rmi_lib step to .drone.yml so they keep running. (Bare cargo test at the repo root only exercises the rmi binary, since rmi_lib is a path dependency.)

Reviving the tests surfaced one real bug: FixDupsIter (behind RMITrainingData::iter()) re-emitted the final training row, so every model saw one phantom duplicate. The CDF trainers (ncdf/lncdf/loglinear_slr) compute statistics with n = len() while iterating len()+1 times, which skewed their parameters; stopping the iterator instead of replaying corrects them. Heads-up: this slightly changes generated-RMI training output, so it's worth a look under the integration suite (I couldn't run the 200M-key SOSD benchmarks locally) — test_iter now pins the iterator's behavior.

One expectation was stale rather than buggy: the equidepth-histogram test expected 333 for a key past all training data, a leftover from the old PLR-based linear_norm_histogram. The current equidepth_histogram has num_bins = 333, so the max bin index is 332; I corrected the expected value.

Made in collaboration with Claude.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant