Skip to content

fix(test): restore namespace after explicit run-tests - #798

Open
jasalt wants to merge 1 commit into
nooga:mainfrom
jasalt:fix/test-run-tests-namespace-restore
Open

fix(test): restore namespace after explicit run-tests#798
jasalt wants to merge 1 commit into
nooga:mainfrom
jasalt:fix/test-run-tests-namespace-restore

Conversation

@jasalt

@jasalt jasalt commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Fix test/run-tests failing after otherwise successful tests when called with explicit namespaces.

The explicit-namespace branch saves *ns* as a namespace object, but attempted to restore it with:

(in-ns (symbol (name old-ns)))

name only accepts symbols, keywords, or strings in let-go—not namespace objects—so restoration throws:

error: name expected Symbol, Keyword or String

The no-argument branch already handles namespace objects correctly with ns-name. This change makes the explicit branch use the same conversion:

(in-ns (ns-name old-ns))

Reproduction

(ns example-test
  (:require [test :refer [deftest is]]))

(deftest passes
  (is true))

(test/run-tests 'example-test)

Before this fix (lg 1.12.2), the assertion passes, but run-tests throws while restoring the caller namespace, causing the command to fail:

example-test=> (test/run-tests 'example-test)
Running tests...
PASS true
PASS true
error: name expected Named
  --> <embedded:test>:80:29

stack trace:
  at name (<embedded:test>:80:29)
  at anonymous fn (<embedded:test>:59:33)
  at anonymous fn (<embedded:test>:72:6)
  at run-tests (REPL:1:1)

After this fix, the test completes successfully and the caller’s namespace is restored:

...
example-test=> (test/run-tests 'example-test)
Running tests...
PASS true
Finished running tests. Tests: 1 Pass: 1 Fail: 0 Error: 0
#'test/*test-result*

Clojure reference

Babashka v1.13.219:

(ns example-test
  (:require [clojure.test :refer [deftest is]]))

(deftest passes
  (is true))

(clojure.test/run-tests 'example-test)

;; Testing example-test
;; Ran 1 tests containing 1 assertions.
;; 0 failures, 0 errors.
;; => {:test 1, :pass 1, :fail 0, :error 0, :type :summary}

Changes

  • Restore the saved namespace with ns-name when explicit namespaces are passed.
  • Add a regression test verifying:
    • tests execute in the explicitly requested namespace;
    • run-tests returns without a restoration error;
    • the caller namespace is restored afterward.
  • Regenerate the embedded runtime bundle and generated manifests.

Testing

  • make check-generated
  • make test

LLM assisted PR (gpt-5.6-sol)

@nooga

nooga commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Review summary

Verified the fix directly: reverted just the one-line change ((in-ns (symbol (name old-ns)))(in-ns (ns-name old-ns))), regenerated core_compiled.lgb, and reran test/run_tests_test.lg — it reproduces the exact reported error (name expected Symbol, Keyword or String). Restoring the fix makes it pass again, so the new regression test genuinely exercises the bug rather than passing vacuously.

Also checked:

  • go generate ./pkg/rt/ reproduces core_compiled.lgb/generated.manifest/generated.sums byte-for-byte — consistent with what's checked in.
  • go build ./..., go vet ./... clean.
  • go test ./... — green apart from environment-only failures in this bare worktree (missing test/clojure-test-suite git submodule, no .lgbgen-tree.sum for the lowered-tree parity tests) and one pre-existing flaky os_paths_test.lg case that also fails standalone on main and passes in isolation — none attributable to this change.

Small, correct, well-tested fix. LGTM — approving.

@nooga nooga left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the fix reproduces and resolves the reported error; regression test is genuine (confirmed it fails without the fix). See summary comment.

@nnunley

nnunley commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

This remains approved and its head checks are green at 7b3d2c1fd9f1f2605cdba345e5e9d35fee89d895, but GitHub now reports conflicts with main (3aa9f4ea86d41dd55ced66fb3ca800c4bc66c355).

@jasalt, please update/rebase the contributor-owned branch and resolve the generated-artifact overlap, then let CI rerun. I will not push to it. Once it is current-base, conflict-free, and fully green, it is eligible to land.

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.

3 participants