search

jacktasia / dumb-jump

an Emacs "jump to definition" package for 50+ languages
GNU General Public License v3.0
1.58k stars 151 forks source link

dumb-jump-goto-file-line-test failure against GNU Emacs 29.1 #442

Open dogsleg opened 1 year ago

dogsleg commented 1 year ago

Hi,

Thanks for your work on dumb-jump.

I'm getting the following error when run against GNU Emacs 29.1 from the Debian unstable archive:

passed   88/161  dumb-jump-go-var-let-test (0.041804 sec)
Test dumb-jump-goto-file-line-test backtrace:
  signal(mock-error (not-called ring-insert))
  mock-verify()
  #f(compiled-function () #<bytecode -0x108c38f63c4a627d>)()
  mock-protect((closure ((js-file . "/home/dogsleg/freedom/packaging/e
  (let ((js-file (f-join test-data-dir-proj1 "src" "js" "fake.js"))) (
  (closure (t) nil (let ((js-file (f-join test-data-dir-proj1 "src" "j
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name dumb-jump-goto-file-line-test :docume
  ert-run-or-rerun-test(#s(ert--stats :selector t :tests ... :test-map
  ert-run-tests(t #f(compiled-function (event-type &rest event-args) #
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-l" "package" "--eval" "(add-to-list 'package-direc
  command-line()
  normal-top-level()
Test dumb-jump-goto-file-line-test condition:
    (mock-error not-called ring-insert)
   FAILED   89/161  dumb-jump-goto-file-line-test (0.003110 sec) at test/dumb-jump-test.el:348

All other tests are fine.

jobor commented 8 months ago

The failing test does

(mock (ring-insert * *))

and in Emacs 29, ring-insert is not called anymore.

dumb-jump-goto-file-line has the following bit at the start:

  (if (fboundp 'xref-push-marker-stack)
      (xref-push-marker-stack)
    (ring-insert find-tag-marker-ring (point-marker)))

In recent Emacs versions, xref-push-marker-stack is called. Thus, the test essentially checks the implementation of that function.

xref-push-marker-stack looks like this in Emacs 28:

(defun xref-push-marker-stack (&optional m)
  "Add point M (defaults to `point-marker') to the marker stack."
  (ring-insert xref--marker-ring (or m (point-marker))))

In Emacs 29, it's

(defun xref-push-marker-stack (&optional m)
  "Add point M (defaults to `point-marker') to the marker stack.
Erase the stack slots following this one."
  (xref--push-backward (or m (point-marker)))
  (let ((history (xref--get-history)))
    (dolist (mk (cdr history))
      (set-marker mk nil nil))
    (setcdr history nil)))

and

(defun xref--push-backward (m)
  "Push marker M onto the backward history stack."
  (let ((history (xref--get-history)))
    (unless (equal m (caar history))
      (push m (car history)))))

Long story short: ring-insert is not called anymore in Emacs 29. The test is faulty.

We could fix the issue like this:

(ert-deftest dumb-jump-goto-file-line-test ()
  (let ((js-file (f-join test-data-dir-proj1 "src" "js" "fake.js")))
    (with-mock
     (when (version< emacs-version "29")
       (mock (ring-insert * *)))
     (dumb-jump-goto-file-line js-file 3 0)
     (should (string= (buffer-file-name) js-file))
     (should (= (line-number-at-pos) 3)))))

or turn the mock into a stub or remove that line alltogether.

jobor commented 8 months ago

cc @jacktasia who added this quite some time ago. I'm sure you remember as if you added this yesterday... ;) but is the mock call supposed to prevent side-effects or can it be removed? Or should it be replaced with something else for Emacs 29?

jacktasia commented 8 months ago

@jobor Honestly I can't really remember, but I think the last example where you sniff the emacs-version is necessary since we definitely support older versions than that