Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cider-test-jump doesn't work properly from test report #3533

Closed
rrudakov opened this issue Oct 17, 2023 · 52 comments · Fixed by #3535
Closed

cider-test-jump doesn't work properly from test report #3533

rrudakov opened this issue Oct 17, 2023 · 52 comments · Fixed by #3535

Comments

@rrudakov
Copy link
Contributor

Expected behavior

When I hit RET on a failed test in *cider-test-report* buffer I jump to corresponding failed test source.

Actual behavior

There are 2 possible problems. Assuming the following test report:

Results

com.example.app-test
1 non-passing tests:

Fail in foo-test

expected: (= {:one 1, :two 2, :three 3, :five 5, :six 6} {:four 4})
  actual: (not (= {:one 1, :two 2, :three 3, :five 5, :six 6} {:four 4}))          
  • If the point is on expected word, I'm getting Symbol ‘com.example.app-test/foo-test’ not resolved error in the minibuffer.
  • If the point is inside of the expected value I'm getting a prompt in the minibuffer No comment syntax is defined. Use: . After setting ;, I'm getting Symbol not resolved error again.
image

Steps to reproduce the problem

Project files:
deps.edn:

{:paths   ["src"]
 :deps    {}
 :aliases {:test {:extra-paths ["test"]}}}

src/com/example/app.clj:

(ns com.example.app)

(defn foo
  [bar]
  (println bar))

test/com/example/app_test.clj:

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

(deftest foo-test
  (is (= {:one   1
          :two   2
          :three 3
          :five  5
          :six   6}
         {:four 4})))
  1. Jack-in to the project (enable :test alias additionally)
  2. Run faulty test
  3. Try to navigate from test report buffer to the failed test source.

Environment & Version information

CIDER version information

;; CIDER 1.8.2 (Geneva), nREPL 1.0.0
;; Clojure 1.11.1, Java 17.0.8.1

Lein / Clojure CLI version

Clojure CLI 1.11.1

Emacs version

GNU Emacs 29.1.50 (build 1, aarch64-apple-darwin23.0.0, NS appkit-2487.00 Version 14.0 (Build 23A344)) of 2023-10-06

Operating system

MacOS 14 Sonoma

JDK distribution

openjdk version "17.0.8.1" 2023-08-24
OpenJDK Runtime Environment Homebrew (build 17.0.8.1+0)
OpenJDK 64-Bit Server VM Homebrew (build 17.0.8.1+0, mixed mode, sharing)
@vemv
Copy link
Member

vemv commented Oct 17, 2023

I tried it just now and it works for me.

Try placing the cursor over foo-test (from Fail in foo-test) and M-x describe-char.

You should see ns and var info, denoting an existing and loaded var:

image

Is it not the case for you?

@vemv
Copy link
Member

vemv commented Oct 17, 2023

The error message comes from either of these:

(defun cider--find-var-other-window (var &optional line)
  "Find the definition of VAR, optionally at a specific LINE.

Display the results in a different window."
  (if-let* ((info (cider-var-info var)))
      (progn
        (if line (setq info (nrepl-dict-put info "line" line)))
        (cider--jump-to-loc-from-info info t))
    (user-error "Symbol `%s' not resolved" var)))

(defun cider--find-var (var &optional line)
  "Find the definition of VAR, optionally at a specific LINE."
  (if-let* ((info (cider-var-info var)))
      (progn
        (if line (setq info (nrepl-dict-put info "line" line)))
        (cider--jump-to-loc-from-info info))
    (user-error "Symbol `%s' not resolved" var)))

Therefore, please enable nrepl message logging and attach the nrepl logs here.

Thanks - V

@rrudakov
Copy link
Contributor Author

Thanks for the quick response @vemv. Here's the requested data:

Describe char

There are text properties here:
  actual               "(not (= {:one 1, :two 2, :three 3, :five 5, :six 6} {:four 4}))\n"
  context              nil
  elapsed-time         (dict "humanized" "Completed in 7 ms" "ms" 7)
  expected             "(= {:one 1, :two 2, :three 3, :five 5, :six 6} {:four 4})\n"
  file                 "app_test.clj"
  font-lock-face       font-lock-function-name-face
  index                0
  line                 6
  message              ""
  ns                   "com.example.app-test"
  type                 "fail"
  var                  "foo-test"

Nrepl log messages

When I hit RET in the test report buffer I don't see any new messages in the log BTW.

nrepl-log-messages
(-->
  id         "11"
  op         "load-file"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:26:57.098964000"
  file       "(ns com.example.app-test
  (:require
   [clojure.test :refer..."
  file-name  "app_test.clj"
  file-path  "/Users/rrudakov/Personal/Projects/clj-bug/test/com/example/a..."
)
(<--
  id         "11"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:26:57.324723000"
  value      "#'com.example.app-test/foo-test"
)
(<--
  id         "11"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:26:57.327089000"
  status     ("done")
)
(-->
  id         "12"
  op         "classpath"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:26:57.556363000"
)
(<--
  id         "12"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:26:57.564491000"
  classpath  ("/Users/rrudakov/Personal/Projects/clj-bug/test" "/Users/rrudakov/Personal/Projects/clj-bug/src" "/Users/rrudakov/.m2/repository/cider/cider-nrepl/0.40.0/cider-nrepl-0.40.0.jar" "/Users/rrudakov/.m2/repository/nrepl/nrepl/1.0.0/nrepl-1.0.0.jar" "/Users/rrudakov/.m2/repository/org/clojure/clojure/1.11.1/clojure-1.11.1.jar" "/Users/rrudakov/.m2/repository/refactor-nrepl/refactor-nrepl/3.9.0/refactor-nrepl-3.9.0.jar" "/Users/rrudakov/.m2/repository/cider/orchard/0.16.1/orchard-0.16.1.jar" "/Users/rrudakov/.m2/repository/mx/cider/logjam/0.1.1/logjam-0.1.1.jar" "/Users/rrudakov/.m2/repository/org/clojure/core.specs.alpha/0.2.62/core.specs.alpha-0.2.62.jar" "/Users/rrudakov/.m2/repository/org/clojure/spec.alpha/0.3.218/spec.alpha-0.3.218.jar")
  status     ("done")
)
(-->
  id              "13"
  op              "ns-list"
  session         "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp      "2023-10-17 13:26:57.623745000"
  exclude-regexps ("^cider.nrepl" "^refactor-nrepl" "^nrepl")
)
(<--
  id         "13"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:26:57.648918000"
  ns-list    ("cider.nrepl" ...)
  status     ("done")
)
(<--
  id                 "11"
  session            "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp         "2023-10-17 13:26:57.709883000"
  changed-namespaces (dict ...)
  repl-type          "clj"
  status             ("state")
)
(-->
  id         "14"
  op         "info"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:27:11.518092000"
  context    "nil"
  ns         #("com.example.app-test" 0 20 (face font-lock-type-face cider-block-dynamic-font-lock t cider-locals nil fontified t))
  sym        "com.example.app-test/foo-test"
)
(<--
  id         "14"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:27:11.743880000"
  column     1
  file       "file:/Users/rrudakov/Personal/Projects/clj-bug/test/com/exam..."
  line       5
  name       "foo-test"
  ns         "com.example.app-test"
  status     ("done")
)
(-->
  id         "15"
  op         "test"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:27:11.798929000"
  fail-fast  "true"
  load?      "true"
  ns         #("com.example.app-test" 0 20 (face font-lock-type-face cider-block-dynamic-font-lock t cider-locals nil fontified t))
  tests      ("foo-test")
)
(<--
  id               "15"
  session          "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp       "2023-10-17 13:27:11.846249000"
  elapsed-time     (dict ...)
  gen-input        nil
  ns-elapsed-time  (dict ...)
  results          (dict ...)
  summary          (dict ...)
  testing-ns       "com.example.app-test"
  var-elapsed-time (dict ...)
)
(-->
  id         "16"
  op         "info"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:27:11.846654000"
  context    ":same"
  ns         "user"
  sym        "com.example.app-test/foo-test"
)
(<--
  id         "15"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:27:11.848141000"
  status     ("done")
)
(<--
  id         "16"
  session    "e4726101-37e7-4ba1-bbfa-b2aa80d65f58"
  time-stamp "2023-10-17 13:27:11.848296000"
  column     1
  file       "file:/Users/rrudakov/Personal/Projects/clj-bug/test/com/exam..."
  line       5
  name       "foo-test"
  ns         "com.example.app-test"
  status     ("done")
)

Screenshot

image

@vemv
Copy link
Member

vemv commented Oct 17, 2023

What if you M-: (cider-var-info "foo-test") while POINT is anywhere in the test .clj buffer?

It should result in a successful request being logged.

@rrudakov
Copy link
Contributor Author

nrepl logs
(-->
  id         "11"
  op         "load-file"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:39:53.770214000"
  file       "(ns com.example.app-test
  (:require
   [clojure.test :refer..."
  file-name  "app_test.clj"
  file-path  "/Users/rrudakov/Personal/Projects/clj-bug/test/com/example/a..."
)
(<--
  id         "11"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:39:53.991295000"
  value      "#'com.example.app-test/foo-test"
)
(<--
  id         "11"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:39:53.993647000"
  status     ("done")
)
(-->
  id         "12"
  op         "classpath"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:39:54.232972000"
)
(<--
  id         "12"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:39:54.240529000"
  classpath  ("/Users/rrudakov/Personal/Projects/clj-bug/test" "/Users/rrudakov/Personal/Projects/clj-bug/src" "/Users/rrudakov/.m2/repository/cider/cider-nrepl/0.40.0/cider-nrepl-0.40.0.jar" "/Users/rrudakov/.m2/repository/nrepl/nrepl/1.0.0/nrepl-1.0.0.jar" "/Users/rrudakov/.m2/repository/org/clojure/clojure/1.11.1/clojure-1.11.1.jar" "/Users/rrudakov/.m2/repository/refactor-nrepl/refactor-nrepl/3.9.0/refactor-nrepl-3.9.0.jar" "/Users/rrudakov/.m2/repository/cider/orchard/0.16.1/orchard-0.16.1.jar" "/Users/rrudakov/.m2/repository/mx/cider/logjam/0.1.1/logjam-0.1.1.jar" "/Users/rrudakov/.m2/repository/org/clojure/core.specs.alpha/0.2.62/core.specs.alpha-0.2.62.jar" "/Users/rrudakov/.m2/repository/org/clojure/spec.alpha/0.3.218/spec.alpha-0.3.218.jar")
  status     ("done")
)
(-->
  id              "13"
  op              "ns-list"
  session         "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp      "2023-10-17 13:39:54.302728000"
  exclude-regexps ("^cider.nrepl" "^refactor-nrepl" "^nrepl")
)
(<--
  id         "13"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:39:54.327877000"
  ns-list    ("cider.nrepl" ...)
  status     ("done")
)
(<--
  id                 "11"
  session            "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp         "2023-10-17 13:39:54.754141000"
  changed-namespaces (dict ...)
  repl-type          "clj"
  status             ("state")
)
(-->
  id         "14"
  op         "eldoc"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:06.406749000"
  context    "(deftest __prefix__
  (is (= {:one   1
          :two   2
  ..."
  ns         #("com.example.app-test" 0 20 (face font-lock-type-face cider-block-dynamic-font-lock t cider-locals nil fontified t))
  sym        "foo-test"
)
(-->
  id         "15"
  op         "eldoc"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:06.596965000"
  context    "(__prefix__ foo-test
  (is (= {:one   1
          :two   2
 ..."
  ns         #("com.example.app-test" 0 20 (face font-lock-type-face cider-block-dynamic-font-lock t cider-locals nil fontified t))
  sym        "deftest"
)
(<--
  id         "14"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:06.691189000"
  docstring  nil
  name       "foo-test"
  ns         "com.example.app-test"
  status     ("done")
  type       "variable"
)
(<--
  id         "15"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:06.731593000"
  docstring  "Defines a test function with no arguments.  Test functions m..."
  eldoc      (("name" "&" "body"))
  name       "deftest"
  ns         "clojure.test"
  status     ("done")
  type       "macro"
)
(-->
  id         "16"
  op         "eldoc"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:07.486164000"
  context    "(deftest __prefix__
  (is (= {:one   1
          :two   2
  ..."
  ns         #("com.example.app-test" 0 20 (face font-lock-type-face cider-block-dynamic-font-lock t cider-locals nil fontified t))
  sym        "foo-test"
)
(<--
  id         "16"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:07.499497000"
  docstring  nil
  name       "foo-test"
  ns         "com.example.app-test"
  status     ("done")
  type       "variable"
)
(-->
  id         "17"
  op         "info"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:22.003575000"
  context    "nil"
  ns         #("com.example.app-test" 0 20 (face font-lock-type-face cider-block-dynamic-font-lock t cider-locals nil fontified t))
  sym        "foo-test"
)
(<--
  id         "17"
  session    "1da9b704-4723-42f1-b05e-7e30aea09155"
  time-stamp "2023-10-17 13:40:22.018045000"
  column     1
  file       "file:/Users/rrudakov/Personal/Projects/clj-bug/test/com/exam..."
  line       5
  name       "foo-test"
  ns         "com.example.app-test"
  status     ("done")
)

Result of evaluation:

(dict "status" ("done") "column" 1 "file" "file:/Users/rrudakov/Personal/Projects/clj-bug/test/com/example/app_test.clj" "id" "17" "line" 5 "name" ...)

@vemv
Copy link
Member

vemv commented Oct 17, 2023

Thanks!

I'd recommend restarting emacs, enabling nrepl logging in advance, jacking in and retrying.

We're hunting for a "info" request which wasn't successful.

There's the possibility no request was sent at all, but let's go step by step.

@vemv
Copy link
Member

vemv commented Oct 17, 2023

...Note that cider-var-info has:

(defun cider-var-info (var &optional all)
  "Return VAR's info as an alist with list cdrs.
When multiple matching vars are returned you'll be prompted to select one,
unless ALL is truthy."
+  (when (and var (not (string= var "")))

So that's a possible reason for the request not being sent.

You can debug with message, cl-assert, etc.

@rrudakov
Copy link
Contributor Author

After edebugging cider-var-info and cider-sync-request:info I figured out that wrong CIDER session was used (I had another project running).I closed all other REPLs, and now I'm getting another error: No linked CIDER session (even though REPL is running).

image

@rrudakov
Copy link
Contributor Author

I've started a fresh emacs session using emacs -q and tried to reproduce the issue. Now jumping works fine from expected word, but if I hit RET from the inside of expected map, I'm still being asked to specify comment syntax:

image

@rrudakov
Copy link
Contributor Author

Looks like there is a problem when more than 1 REPL is running and buffer *cider-test-report* is being re-used. If I run tests for the first project, dismiss *cider-test-report* buffer and then run tests from another project I'm getting either No linked CIDER session error or Symbol not found depends on whether the REPL for an old project is still running.

@vemv
Copy link
Member

vemv commented Oct 17, 2023

Thanks! So it all seems to boil down to Sesman session linking.

We've fixed a bunch of those this summer, but not this one in particular.

I'll see what I can do - should be feasible.

No comment syntax is defined is unrelated. I wish I knew how to fix it 😅 we have an open issue for it.

@vemv
Copy link
Member

vemv commented Oct 17, 2023

Note to self.

A possible algo for determining session friendliness is:

  • grab the namespaces from *cider-test-report* buffer contents (as text properties)
  • Check if any of those namespaces belongs to the cached (all-ns) list.

Edit: or we could just save the current repl as a buffer-local variable.

@rrudakov
Copy link
Contributor Author

No comment syntax is defined is unrelated. I wish I knew how to fix it 😅 we have an open issue for it.

So, I tracked down where it comes from.

cider-var-info has the following code:

(cider-sync-request:info var nil nil (cider-completion-get-context t))

and cider-completion-get-context at some point calls cider-completion-get-info-context-at-point, which tries to get a context if the following condition returns true:

(when (save-excursion
          (condition-case _
              (progn
                (up-list)
                (check-parens)
                t)
            (scan-error nil)
            (user-error nil)))

I'm not 100% sure, but it looks like it returns true if point is inside of parenthesis, and because it actually is, we're trying to get context and at some point the following coder from clojure-mode is called:

(defun clojure--looking-at-non-logical-sexp ()
  "Return non-nil if text after point is \"non-logical\" sexp.
\"Non-logical\" sexp are ^metadata and #reader.macros."
  (comment-normalize-vars)
  (comment-forward (point-max))
  (looking-at-p "\\(?:#?\\^\\)\\|#:?:?[[:alpha:]]"))

Maybe we could just check if the major mode is cider-test-report-mode and not requesting completion context in this case?

@rrudakov
Copy link
Contributor Author

I've tried to update cider-completion-get-info-context-at-point locally as following:

(when (and (not (derived-mode-p 'cider-test-report-mode))
             (save-excursion
               (condition-case _
                   (progn
                     (up-list)
                     (check-parens)
                     t)
                 (scan-error nil)
                 (user-error nil))))
...

and it fixed the problem for me, although maybe it would be better to perform this check earlier.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Thanks for looking into it!

I'm not sure how clojure--looking-at-non-logical-sexp has to do with No comment syntax is defined - did you try to repro?

You can M-: (clojure--looking-at-non-logical-sexp) from any buffer.

We have #3535 btw, if you have it easy, feel free to try it out.

@rrudakov
Copy link
Contributor Author

I'm not sure how clojure--looking-at-non-logical-sexp has to do with No comment syntax is defined - did you try to repro?

Here's the part of source of comment-normalize-vars function:

(defun comment-normalize-vars (&optional noerror)
  "Check and set up variables needed by other commenting functions.
All the `comment-*' commands call this function to set up various
variables, like `comment-start', to ensure that the commenting
functions work correctly.  Lisp callers of any other `comment-*'
function should first call this function explicitly."
  (unless (and (not comment-start) noerror)
    (unless comment-start
      (let ((cs (read-string "No comment syntax is defined.  Use: ")))
	(if (zerop (length cs))
	    (error "No comment syntax defined")
          (setq-local comment-start cs)
          (setq-local comment-start-skip cs))))
    ;; comment-use-syntax

You can M-: (clojure--looking-at-non-logical-sexp) from any buffer.

Yes, I can reproduce it.

We have #3535 btw, if you have it easy, feel free to try it out.

Thanks! Will check it out.

@rrudakov
Copy link
Contributor Author

We have #3535 btw, if you have it easy, feel free to try it out.

unfortunately this PR doesn't fix the problem with navigation to the failed test from test report if there are 2 projects. I can reproduce it now 100% when I have 2 or more REPLs running.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Weird, I could repro the problem and verify that the change fixed it.

Make sure to re-eval all relevant code / remove .elc files?

Note that the solution partly relies on state - make sure to restart Emacs, just this time.

@rrudakov
Copy link
Contributor Author

Yes, it's a fresh emacs -q session, I've cleaned all *.elc files and additionally eln-cache (I use native compilation). I double checked that the patch from the PR is applied:

image

@vemv
Copy link
Member

vemv commented Oct 18, 2023

What's the value of cider-test--current-repl?

It should point at the buffer repl representing that of your current project.

@rrudakov
Copy link
Contributor Author

What's the value of cider-test--current-repl?

It should point at the buffer repl representing that of your current project.

cider-test--current-repl is a variable defined in ‘cider-test.el’.

Its value is
#<buffer *cider-repl Projects/clj-bug:localhost:64273(clj)
*>

Contains the reference to the REPL where the tests were last invoked from.
This is needed for *cider-test-report* navigation
to work against the correct REPL session.

from the name it looks correct, but after debugging cider-sync-request:info I figured that (cider-current-repl) returns the wrong session. Will keep digging.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Thanks. (cider-current-repl) should precisely return the right repl thanks to this patch.

It would not work if you have any custom code around that performs sesman linking (e.g. sesman-link-with-buffer).

@rrudakov
Copy link
Contributor Author

I don't have any custom code.

cider-repl-type-for-buffer returns nil, so in cider-repls function it falls into:

(_ (cdr (if ensure
                             (sesman-ensure-session 'CIDER)
                           (sesman-current-session 'CIDER))))

which return the wrong session. No idea how to debug it further :)

@vemv
Copy link
Member

vemv commented Oct 18, 2023

I'm at a loss.

Say that I'm running two repls for two projects: Haystack and Logjam.

Can you verify what (sesman--friendly-sessions 'CIDER 'sort) returns when If POINT is at *cider-test-report*? It should only give an entry for the current project.

If not, you would have to determine why cider--sesman-friendly-session-p is returning a truthy value for an unrelated repl.

(If I had to guess, I'd say it's because of the ns-list logic - maybe your two projects share some namespaces in common?)

I'd totally get if I was asking for too much to debug (although it would be super useful to have a variety of contributors understand these critical bits). OTOH you can rest assured that the PR would at least fix 80% of the cases.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Are your projects/repls JVM-clojure only?

@rrudakov
Copy link
Contributor Author

I'm at a loss.

Say that I'm running two repls for two projects: Haystack and Logjam.

  • Given that I'm at the Logjam project
  • If POINT is at a .clj buffer and I run (sesman--friendly-sessions 'CIDER 'sort), I see 2 entries in the result (logjam, haystack)

That's correct, I'm getting 2 sessions if I do that as well.

  • If POINT is at *cider-test-report* and I run (sesman--friendly-sessions 'CIDER 'sort), I see 1 entry in the result (logjam)

That's also correct, I see the correct session if I call this function.

The problem is when you execute cider-test-jump, sesmam--friently-sessions is never being called. The chain of calls is the following:
cider-test-jump -> cider-find-var -> cider--find-var -> cider-var-info -> cider-sync-request:info -> cider-current-repl.

cider-current-repl is called without any arguments (so type is nil).

(If I had to guess, I'd say it's because of the ns-list logic - maybe your two projects share some namespaces in common?)

No there are no conflicting namespaces.

I'd totally get if I was asking for too much to debug (although it would be super useful to have a variety of contributors understand these critical bits). OTOH you can rest assured that the PR would at least fix 80% of the cases.

No problem, I want to help solving this issue :)

@rrudakov
Copy link
Contributor Author

Are your projects/repls JVM-clojure only?

Yes, both projects are JVM-only.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

The problem is when you execute cider-test-jump, sesmam--friently-sessions is never being called.

I would say it is, deeper in the stack.

I'll be checking something, will get back at this thread.

No problem, I want to help solving this issue :)

Thanks much! 🙌

@vemv
Copy link
Member

vemv commented Oct 18, 2023

A simpler way to look at it (which also correctly works for me):

  • Run tests in Logjam, leave *cider-test-report* open
  • When POINT is at a .clj buffer of Haystack, (cider-current-repl) points at Haystack
  • When POINT is at *clojure-test-report*, (cider-current-repl) points at Logjam

i.e., make sure that (cider-current-repl) always returns the right repl.

If it doesn't, it's because cider--sesman-friendly-session-p is returning t for a repl of the other project. You'd have to alter defun cider--sesman-friendly-session-p such that you understand what it's doing - why it ends up returning a truthy value.

Normally I add setqs or messages.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Just in case, please make sure you're actually using my PR. Sometimes CIDER is defined twice in .emacs.d, e.g. elpa/ + a git clone.

Re-eval cider--sesman-friendly-session-p at the IELM repl for good measure. Make sure that its source code begins like this:

  (when-let ((repl (cadr session)))
    (cond
     ((equal (buffer-name)
             cider-test-report-buffer)
      (or (not cider-test--current-repl)
          (not (buffer-live-p cider-test--current-repl))
          (equal repl
                 cider-test--current-repl)))

i.e. that you're evaling the intended version.

@rrudakov
Copy link
Contributor Author

I would say it is, deeper in the stack.

I doubt it, because I marked it for edebug using C-u M-C-x and it wasn't called.

A simpler way to look at it (which also correctly works for me):

  • Run tests in Logjam, leave cider-test-report open
  • When POINT is at a .clj buffer of Haystack, (cider-current-repl) points at Haystack
  • When POINT is at clojure-test-report, (cider-current-repl) points at Logjam

i.e., make sure that (cider-current-repl) always returns the right repl.

(cider-current-repl) returns the wrong repl when called from test report buffer, cider--sesman-friendly-session-p is not called (I've tried both edebug and just adding (message) call to the body).

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Is cider-merge-sessions customized for you? I have it at its default, nil

Otherwise, you'll have to debug why cider-current-repl doesn't reach cider--sesman-friendly-session-p.

Its path to it is cider-current-repl -> cider-repls -> then:

(_ (cdr (if ensure
             (sesman-ensure-session 'CIDER)
           (sesman-current-session 'CIDER))))

, then sesman-current-session is reached by either branch (since sesman-ensure-session is a wrapper for it), then sesman--friendly-sessions, finally we reach cider--sesman-friendly-session-p should be reached.

...Note that it's defined like this:

(cl-defmethod sesman-friendly-session-p ((_system (eql CIDER)) session)
  "Check if SESSION is a friendly session."
  (cider--sesman-friendly-session-p session))

i.e. the cl-defmethod bit is unusual, but it doesn't need anything special to work.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

If you still don't see it reached, note this post #3533 (comment)

@rrudakov
Copy link
Contributor Author

Is cider-merge-sessions customized for you? I have it at its default, nil

no, I can reproduce it on emacs -q with default CIDER settings.

Hmm. I evaluated cider--sesman-friendly-session-p in IELM and I added a custom message to the body (to the very top). And I can see that my message was printed after calling (cider-current-repl), but only once (after the first call). If I call (cider-current-repl) again, the message is not printed anymore. Is it possible that some caching takes place?

@rrudakov
Copy link
Contributor Author

(defun sesman-current-session (system &optional cxt-types)
  "Get the most relevant current session for the SYSTEM.
CXT-TYPES is a list of context types to consider."
  (or (car (sesman--linked-sessions system 'sort cxt-types))
      (car (sesman--friendly-sessions system 'sort))))

Here sesman--linked-sessions returns not empty list, maybe that's the reason cider--sesman-friendly-session-p is not being called?

@rrudakov
Copy link
Contributor Author

Yes, I removed (car (sesman--linked-sessions system 'sort cxt-types)) and now I'm getting the right session from test report buffer.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

That would make sense.

Does (sesman--linked-sessions 'CIDER 'sort nil)) return the wrong repl?

@rrudakov
Copy link
Contributor Author

That would make sense.

Does (sesman--linked-sessions 'CIDER 'sort nil)) return the wrong repl?

Yes, it does.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Do your two projects share a current directory, something like that? Maybe you're working on a monorepo?

@rrudakov
Copy link
Contributor Author

No, one of them is not even a git repository.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Mmm. That would be a severe issue. (sesman--linked-sessions 'CIDER 'sort nil) returns exactly one (and the correct) repl in two connected projects.

It's not an area we've been touching.

At least I can be rest assured that the PR does what it does correctly.

My PR will close this issue. You are free to create an issue that demonstrates how to reproduce wrong results for (sesman--linked-sessions 'CIDER 'sort nil), in a setup with two connected projects / repls.

Please do not mention *cider-test-repl* there - as I understand it, this affects vanilla .clj buffers.

vemv added a commit that referenced this issue Oct 18, 2023
@rrudakov
Copy link
Contributor Author

Please do not mention *cider-test-repl* there - as I understand it, this affects vanilla .clj buffers.

But (sesman--linked-sessions 'CIDER 'sort nil) returns correct session in clojure source buffers, the wrong result is returned only in *cider-test-report*.

@vemv
Copy link
Member

vemv commented Oct 18, 2023

Mmm, I see, it's because the buffer's default-directory points to one of the given projects.

By making it nil, it will work.

Thanks much for the perseverance!

@vemv
Copy link
Member

vemv commented Oct 18, 2023

@rrudakov
Copy link
Contributor Author

Could you try https://github.com/clojure-emacs/cider/pull/3535/files again?

Yes, looks like now the issue is fixed! Thank you!

vemv added a commit that referenced this issue Oct 18, 2023
@vemv
Copy link
Member

vemv commented Oct 18, 2023

Thanks to you 🍻!

@rrudakov
Copy link
Contributor Author

It was quite interesting experience :)

BTW, any chance that another stable release with this fix will be on nongnu ELPA any time soon?

@vemv
Copy link
Member

vemv commented Oct 18, 2023

MELPA releases (stable / unstable) are automatically available after any given commit (or tags, for Stable)

Does that answer the q?

@rrudakov
Copy link
Contributor Author

I usually stick with nongnu ELPA (currently I'm on version 1.8.2), so I guess next stable version would be 1.8.3.

@rrudakov
Copy link
Contributor Author

I think these changes break some other things in emacs which rely on default-directory. For example project-kill-buffers doesn't work anymore.

Here's the stacktrace:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  expand-file-name(nil)
  (setq dd (expand-file-name (buffer-local-value 'default-directory buf)))
  (let ((buf (car tail))) (setq dd (expand-file-name (buffer-local-value 'default-directory buf))) (if (and (string-prefix-p root dd) (not (cl-find-if #'(lambda (module) (string-prefix-p module dd)) modules))) (progn (setq bufs (cons buf bufs)))) (setq tail (cdr tail)))
  (while tail (let ((buf (car tail))) (setq dd (expand-file-name (buffer-local-value 'default-directory buf))) (if (and (string-prefix-p root dd) (not (cl-find-if #'(lambda ... ...) modules))) (progn (setq bufs (cons buf bufs)))) (setq tail (cdr tail))))
  (let ((tail (buffer-list))) (while tail (let ((buf (car tail))) (setq dd (expand-file-name (buffer-local-value 'default-directory buf))) (if (and (string-prefix-p root dd) (not (cl-find-if #'... modules))) (progn (setq bufs (cons buf bufs)))) (setq tail (cdr tail)))))
  (let* ((root (expand-file-name (file-name-as-directory (project-root project)))) (modules (if (or (project--vc-merge-submodules-p root) (project--submodule-p root)) nil (mapcar #'(lambda (m) (format "%s%s/" root m)) (project--git-submodules)))) dd bufs) (let ((tail (buffer-list))) (while tail (let ((buf (car tail))) (setq dd (expand-file-name (buffer-local-value 'default-directory buf))) (if (and (string-prefix-p root dd) (not (cl-find-if ... modules))) (progn (setq bufs (cons buf bufs)))) (setq tail (cdr tail))))) (nreverse bufs))
  (progn (let* ((root (expand-file-name (file-name-as-directory (project-root project)))) (modules (if (or (project--vc-merge-submodules-p root) (project--submodule-p root)) nil (mapcar #'(lambda ... ...) (project--git-submodules)))) dd bufs) (let ((tail (buffer-list))) (while tail (let ((buf (car tail))) (setq dd (expand-file-name (buffer-local-value ... buf))) (if (and (string-prefix-p root dd) (not ...)) (progn (setq bufs ...))) (setq tail (cdr tail))))) (nreverse bufs)))
  (closure (t) (project) (progn (let* ((root (expand-file-name (file-name-as-directory ...))) (modules (if (or ... ...) nil (mapcar ... ...))) dd bufs) (let ((tail (buffer-list))) (while tail (let (...) (setq dd ...) (if ... ...) (setq tail ...)))) (nreverse bufs))))((vc Git "~/Work/adgoji-campaign-server-proto/"))
  apply((closure (t) (project) (progn (let* ((root (expand-file-name (file-name-as-directory ...))) (modules (if (or ... ...) nil (mapcar ... ...))) dd bufs) (let ((tail (buffer-list))) (while tail (let (...) (setq dd ...) (if ... ...) (setq tail ...)))) (nreverse bufs)))) (vc Git "~/Work/adgoji-campaign-server-proto/") nil)
  project-buffers((vc Git "~/Work/adgoji-campaign-server-proto/"))
  (let ((tail (project-buffers pr))) (while tail (let ((buf (car tail))) (if (project--buffer-check buf project-kill-buffer-conditions) (progn (setq bufs (cons buf bufs)))) (setq tail (cdr tail)))))
  (let (bufs) (let ((tail (project-buffers pr))) (while tail (let ((buf (car tail))) (if (project--buffer-check buf project-kill-buffer-conditions) (progn (setq bufs (cons buf bufs)))) (setq tail (cdr tail))))) bufs)
  project--buffers-to-kill((vc Git "~/Work/adgoji-campaign-server-proto/"))
  (let* ((pr (project-current t)) (bufs (project--buffers-to-kill pr)) (query-user #'(lambda nil (yes-or-no-p (format "Kill %d buffers in %s? " (length bufs) (project-root pr)))))) (cond (no-confirm (mapc #'kill-buffer bufs)) ((null bufs) (message "No buffers to kill")) (project-kill-buffers-display-buffer-list (if (let* ((vbuffer-or-name (get-buffer-create "*Buffer List*")) (vaction (list ... ... ... ... ...)) (vquit-function #'...) (buffer (temp-buffer-window-setup vbuffer-or-name)) (standard-output buffer) window value) (save-current-buffer (set-buffer buffer) (setq value (progn)) (setq window (temp-buffer-window-show buffer vaction))) (if (functionp vquit-function) (funcall vquit-function window value) value)) (progn (mapc #'kill-buffer bufs)))) ((funcall query-user) (mapc #'kill-buffer bufs))))
  project-kill-buffers()
  funcall-interactively(project-kill-buffers)
  command-execute(project-kill-buffers)

@vemv
Copy link
Member

vemv commented Oct 18, 2023

I usually stick with nongnu ELPA (currently I'm on version 1.8.2), so I guess next stable version would be 1.8.3.

Got it. I guess a git tag is picked up likewise. I'll create it now.

I think these changes break some other things in emacs which rely on default-directory

Fixed on master

@rrudakov
Copy link
Contributor Author

Great! Thank you. Will test it a bit later.

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 a pull request may close this issue.

2 participants