feat(test): add run-test-var and run-test for single-var runs - #671
Conversation
There was a problem hiding this comment.
Thank you for the PR! Semi-automated review below for consideration.
Reviewed 1be7b70 against merge-base b170a08e. One P3, no blockers.
[P3] run-test-var throws when :ns metadata is not a Namespace object
pkg/rt/core/test.lg:139. (ns-name (or (:ns (meta v)) *ns*)) assumes :ns holds a *vm.Namespace, and ns-name errors on anything else (pkg/rt/lang.go:3426, "ns-name expected Namespace"). Var metadata elsewhere in the tree stores :ns as a string: pkg/rt/native_direct_install.go:92 assocs vm.String(m.Namespace), and pkg/rt/disasm.go:235 does the same. Since the use case is an editor resolving whatever var sits under the cursor, a natively-installed var reaches this line and throws out of run-test-var, contradicting the docstring's promise that a bad run yields :error counters rather than escaping. The throw lands before the try, so the namespace is not left switched; only the error escapes. Coercing through the-ns semantics, or falling back to *ns* when :ns is not a Namespace, closes it. Vars from plain def are unaffected — they carry no :ns at all, which the sample-failing probe confirms.
Verdict
Correct. Nothing existing breaks. The run-tests refactor is behavior-preserving except that test-var* now prefers (:test (meta tv)) over (deref tv), which is what clojure.test does and which deftest sets to the same fn anyway.
Checked and not raised as findings: the three fixture helpers going public matches clojure.test, where all three are public, and collides with nothing in this repo; clojure.test/run-test-var resolves without extra wiring because nsAliases (pkg/rt/lang.go:339) aliases at the namespace-object level rather than through a per-var export list, so the editor-integration claim in #670 holds; set! reaching the bound frame inside binding is exercised by the counter tests; generated.sums and core_compiled.lgb were both regenerated. The unpopped *testing-vars* on a throwing test and the global-per-session fixtures are pre-existing and unchanged here.
The tests cover counters, isolation from an enclosing run, fixture application, the namespace switch, and namespace restore after a throw, and the probes use plain def rather than deftest so the harness does not pick them up. One gap: every test installs exactly one fixture per phase, so join-fixtures' composition order with multiple fixtures rides on inspection. That ordering is unchanged from the previous inline version, so it is a gap rather than a regression.
Merge-order note
This and #673 will conflict. Both branch from the same base and edit pkg/rt/core/test.lg, #673 in register-test! and this one in deftest directly above it, and both regenerate core_compiled.lgb and generated.sums, so the binary and the digest conflict either way. Landing one and rebasing the other with a fresh make generate is the shortest path.
|
Ordering follow-up now that I have reviewed #673 as well: land #673 first, then rebase this one onto it. Both edit |
|
@mparrett Thanks for the review! fyi, conflicts are resolved. |
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 1be7b70, so the earlier review stands. The one P3 — run-test-var throwing when a var's :ns metadata is not a Namespace object — is not blocking, and it only reaches vars installed through the native path, so it can be a follow-up.
Approving. Ordering unchanged: #673 is the cheaper one to land first, then rebase this onto it.
|
@abogoyavlensky Sorry, after merging your other PR, this one now needs a quick conflict resolution. |
|
@mparrett No worries at all, I've updated the PR |
|
Two things on this one, both mechanical: 1. It now conflicts with 2. No CI has ever run on this head. Happy to push the rebase myself if you'd prefer — I have |
Add run-test-var and run-test to the clojure.test surface so a single var can be run on its own, binding the report counters and invoking the :test fn. Documents both in the compatibility and conformance guides. Rebased onto main by a maintainer; source unchanged, generated artifacts regenerated. Original commits by Andrey Bogoyavlenskiy. Co-authored-by: Andrey Bogoyavlenskiy <abogoyavlensky@gmail.com>
|
Pushed the rebase (
This also gets CI to run for the first time on this PR — the previous head had zero check runs, so the approval had no pipeline behind it. I'll merge once it's green. |
Resolves: #670