fix(test): register each test var once per namespace - #673
Conversation
register-test! appended unconditionally, so evaluating a deftest twice registered the same var twice and run-tests then ran and counted that test twice. That is the ordinary editor flow: edit a test, send it to the REPL again, run the suite. Vars compare by identity, so skipping the conj when the var is already present keeps registration idempotent while leaving distinct vars and their registration order untouched.
There was a problem hiding this comment.
Thank you for the PR! Semi-automated PR review below, for consideration.
Reviewed 1ad26c1 against merge-base b170a08e. One P3, no blockers.
The fix hinges on = being identity for vars, and it is. *Var has no Equals method, so = falls through identityEqShortCircuit, which decides same-pointer early, into valueEqualsCtx, which has no Var case and terminates at return a == b (pkg/rt/lang.go:1263), a Go interface comparison and so pointer identity. Two distinct vars aliased to the same fn object therefore still register separately, because the comparison never derefs. distinct-vars-still-register-separately would not catch that case on its own, since its two probes hold different fns, but the semantics make it moot.
[P3] Resoves: #672 will not close the issue
The PR body reads Resoves: #672. GitHub's closing keywords are resolve/resolves/resolved, so the misspelling means #672 will not auto-close or cross-link on merge.
Verdict
Correct. Minimal and targeted, and the semantics it relies on hold. The tests cover idempotent re-registration, distinct vars registering separately, and order preservation, and the harness note explains why the file binds *registered-tests* rather than calling clear-registered-tests!, which would wipe the registry the enclosing runner is iterating over. generated.sums and core_compiled.lgb were both regenerated.
Checked and not raised as findings: the linear some scan makes registration O(n²) in tests per namespace, which is negligible at realistic namespace sizes since the comparisons are pointer-equality; and the guard does not help the rename case (rename a deftest, re-evaluate, and the stale var stays registered), which is pre-existing and not fixable without keying the registry by name rather than by var.
Merge-order note
Same conflict I flagged on #671. Both PRs edit pkg/rt/core/test.lg from the same base, #673 in register-test! and #671 in deftest directly above it, and both regenerate core_compiled.lgb and generated.sums, so they conflict either way. Landing this one first is the cheaper order: 6 source lines against #671's ~130, so #671 is the one to rebase.
# Conflicts: # pkg/rt/core_compiled.lgb # pkg/rt/generated.sums
|
@mparrett fyi, conflicts are resolved. |
mparrett
left a comment
mparrett
left a comment
There was a problem hiding this comment.
Rebase verified: the net diff against main is identical to what I reviewed at 1ad26c1, so the earlier review stands. The one P3 (the Resoves: #672 typo, which will stop GitHub closing the issue on merge) is not blocking — worth a one-character fix to the description before this lands, but it does not gate the merge.
Approving. Note this still conflicts with #671: both edit pkg/rt/core/test.lg and regenerate the bundle, so landing this one first and rebasing #671 onto it is the cheaper order.
# Conflicts: # pkg/rt/generated.sums
2 participants
Resoves: #672