Cover the exported API the golden-value tests never reach - #4
Merged
Merged
Conversation
…alize ONE-VS-REST and ONE-VS-ONE cache function objects in struct slots that cl-store cannot serialize, so SAVE nulls them and RESTORE re-resolves them. Nothing tested that. Each round-trip test trains the restored learner rather than only testing it, since a learner that restores but cannot train is exactly what a missed slot produces. Verified the guard bites: stubbing out RESTORE's re-resolution turns the three multiclass tests red and leaves the binary and regression ones green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DIM-OF, N-CLASS-OF and SPARSE-LEARNER? are how CLOL-PREDICT decides to read a test file; none was executed by the suite. Nor was :STREAM for classifiers -- REGRESSION-RLS covers it only for regression, while the classifier path is what CLOL-PREDICT actually emits. Verified both guards bite: hardcoding N-CLASS-OF to 2 turns two assertions red, and printing a constant instead of the rounded prediction turns three red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in the library calls to-int, to-float, class-min/max or shuffle-vector -- they exist for the CLI, which parses every option as a string. clol-train and clol-predict themselves were untested, and are the only place defmain's option parsing runs; each pair of subprocess runs costs about a second. The CLI tests skip when ros is not on PATH rather than failing for an environmental reason. Verified: forcing roswell-available-p to nil leaves the suite green with two skips, and dropping clol-train's save call turns two assertions red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review caught two vacuous assertions in the roswell script tests. UIOP:WITH-TEMPORARY-FILE creates its file before the subprocess runs, so (probe-file model) was always true. And DEFMAIN wraps every script body in a HANDLER-CASE that prints the condition and returns normally, so a failing script still exits 0 and (zerop code) was always true too. The scripts' only real failure signal is what they print, so assert stderr is empty and the model file is non-empty. Dropping clol-train's save call now turns eight assertions red, each naming its cause, where before it turned two red only via a downstream line-count mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-review noted that asserting stderr is empty couples these tests to every compile-time style-warning the subprocess emits -- a warning anywhere in reachable source would turn them red for an unrelated reason, and CI runs them under CCL as well as SBCL. Both of DEFMAIN's handlers print the usage text to standard output and a successful run never does, so that is the marker. stderr is still checked for DEFMAIN's own Error: prefix, which surfaces the swallowed condition in the failure output. Verified both directions: dropping clol-train's save call still turns eight assertions red, and injecting a deliberate style-warning into the script leaves the suite green where the old assertion went red. Also made file-size return 0 for a missing file rather than signalling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test-only. No production code changes —
git diff master..HEAD --name-onlyist/cl-online-learning.lispalone.What was uncovered
The suite is a golden-value regression test: it asserts exact learned weight vectors, biases
and accuracies. That is thorough about the learning, and blind to everything around it. An
sb-profilerun over the whole exported API during a suite run found 16 exported functionsthe suite never executed.
This covers five groups of them. 36 deftests / 104 assertions → 54 / 178; runtime stays
under a second on a warm cache.
1.
clol:save/clol:restoreSerialization had no test at all, and it is the most fragile thing here.
one-vs-restandone-vs-onecache function objects in struct slots thatcl-storecannotserialize, so
savenulls them, stores, then re-resolves, andrestorere-resolves afterloading. Add a slot holding a function and forget the matching pair of helpers, and the model
saves and loads fine — it just cannot train any more.
So every round-trip test trains the restored learner, not merely tests it. Six tests: dense
binary, sparse binary, one-vs-rest, one-vs-one, sparse multiclass, and RLS.
Verified the guard bites: stubbing out
restore's re-resolution turns the three multiclasstests red and leaves the binary and regression ones green.
2.
dim-of/n-class-of/sparse-learner?clol-predictcalls these three on a restored model to decide how to read the test file —n-class-of > 2selects multiclass label handling,sparse-learner?selects the sparsereader,
dim-ofgives the width. Get one wrong and the tool silently reads the dataset thewrong way. None was executed.
Hardcoding
n-class-ofto 2 turns two assertions red.3. The classifier
:streampathThe previous PR covered
:streamfor regression. The classifier path — whatclol-predictactually emits — was still untested, and it is the far more common use. The tests pin that a
binary model emits the rounded sign and a multiclass one emits class indices
0..K-1, notthe original LIBSVM labels (
iris.scaleis labelled 1..3 andread-datasubtracts one). Thatoff-by-one is deliberate and now guarded.
Printing a constant instead of the rounded prediction turns three assertions red.
4.
clol.utilsto-int,to-float,class-min/max,shuffle-vector— nothing in the library calls them;they exist for the CLI, which parses every option as a string.
to-float's tests assert theresult is a
single-float, since a double would break the type declarations the update bodiescompile under.
shuffle-vectoris randomized, so it is asserted on by length, multisetequality and in-place identity, never on a specific permutation.
5. The roswell scripts
clol-trainandclol-predictare the library's only user-facing programs and the only placedefmain's option parsing runs. Driven as subprocesses; a pair of runs costs about a second.They
skipwhenrosis not on PATH rather than failing for an environmental reason —verified by forcing that path, which leaves the suite green with two skips.
Getting the assertions right here took two rounds of review:
(probe-file model)and(zerop exit-code). Both are vacuous.uiop:with-temporary-filecreates its file before the subprocess runs, anddefmainwrapsevery script body in a
handler-casethat prints the condition and returns normally — so afailing script still exits 0. Dropping
clol-train'ssavecall was caught only indirectly,by a downstream line-count mismatch surfacing as an opaque error.
compile-time style-warning the subprocess emits — and CI runs them under CCL as well as SBCL.
defmain's handlers print the usage text to standard output, and asuccessful run never does, so that is the marker. stderr is still checked for
defmain's own"Error:"prefix, which surfaces the swallowed condition in the failure output. Model filesize replaces
probe-file.Dropping
clol-train'ssavecall now turns eight assertions red, each naming its cause.Injecting a deliberate style-warning into the script leaves the suite green, where the previous
version went red.
How each test was validated
Every group was checked by breaking the production code it covers and confirming the suite goes
red, then restoring. A test that cannot fail is worth nothing, and for coverage added to code
that already works, deliberately breaking it is the only way to know:
restorestops re-resolving function slotsn-class-ofhardcoded to 2:streamprints a constantclol-trainstops callingsaveStill uncovered, deliberately
clol.vector:ds-v/,ds2s-v*,s-v*n,make-sparse-vectorandsparse-vector-lengthareexported but reached from nowhere — not
src/, notroswell/, not the tests.ds-v/andds2s-v*appear exactly twice in the tree: theirdefunand the export list. They computecorrectly when called by hand, so this is a question of whether they are public API worth
testing or leftovers worth unexporting — a design call, not a coverage gap, so it is left for
you.
clol-trainalso cannot produce a regression or logistic-regression model: its-typeswitchonly offers Perceptron, AROW and SCW-I. Worth knowing given the previous PR fixed
clol-predictfor RLS models — those can currently only be produced from a REPL.🤖 Generated with Claude Code