greghendershott / racket-mode

Emacs major and minor modes for Racket: edit, REPL, check-syntax, debug, profile, packages, and more.
https://www.racket-mode.com/
GNU General Public License v3.0
682 stars 93 forks source link

ert tests fail if racket-mode is installed #246

Open bremner opened 7 years ago

bremner commented 7 years ago

If the package is not installed, the tests pass. However, if the package is already installed, two tests fail.

2 unexpected results:
   FAILED  racket-tests/indent-rkt
   FAILED  racket-tests/run

Here are the relevant backtraces

Test racket-tests/indent-rkt backtrace:
  (if (unwind-protect (setq value-52 (apply fn-50 args-51)) (setq form
  (let (form-description-54) (if (unwind-protect (setq value-52 (apply
  (let ((value-52 (quote ert-form-evaluation-aborted-53))) (let (form-
  (let ((fn-50 (function racket-tests/same-indent)) (args-51 (list "ex
  (lambda nil (let ((fn-50 (function racket-tests/same-indent)) (args-
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test racket-tests/indent-rkt "Indentatio
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test r
  ert-run-tests(t #[385 "\306\307\"\203G\211\211G\310U\203\211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-eval" "(progn (add-to-list (quote load-path) \"/ho
  command-line()
  normal-top-level()

Test racket-tests/run backtrace:
  (if (eq major-mode (quote racket-mode)) nil (error "Current buffer i
  racket--do-run(medium)
  racket-run()
  (let* ((racket--repl-command-connect-timeout (* 15 60)) (racket-comm
  (lambda nil (let* ((racket--repl-command-connect-timeout (* 15 60)) 
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test racket-tests/run nil (lambda nil (l
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test r
  ert-run-tests(t #[385 "\306\307\"\203G\211\211G\310U\203\211@\20
  ert-run-tests-batch(nil)
  ert-run-tests-batch-and-exit()
  command-line-1(("-eval" "(progn (add-to-list (quote load-path) \"/ho
  command-line()
  normal-top-level()
greghendershott commented 7 years ago
  1. How are you running the tests -- via make test?
  2. What is the Emacs version?
bremner commented 7 years ago

Greg Hendershott notifications@github.com writes:

  1. How are you running the tests -- via make test?

yes

  1. What is the Emacs version?

25.1 by default; I ran "EMACSBIN=/usr/bin/emacs24 make test" and get the same results

greghendershott commented 7 years ago

Hmm. When you say "the package is already installed", do you mean using Emacs' package-install or something else?

bremner commented 7 years ago

Greg Hendershott notifications@github.com writes:

Hmm. When you say "the package is already installed", do you mean using Emacs' package-install or something else?

Using package-install, but into a system wide directory

╭─ zancas:~ ╰─% ls /usr/share/emacs25/site-lisp/elpa/racket-mode-0.4 channel.rkt racket-common.elc racket-mode.el cmds.rkt racket-complete.el racket-mode.elc defn.rkt racket-complete.elc racket-mode-pkg.el error.rkt racket-custom.el racket-mode-pkg.elc find-module-path-completions.rkt racket-custom.elc racket-profile.el fresh-line.rkt racket-edit.el racket-profile.elc gui.rkt racket-edit.elc racket-repl.el image.rkt racket-font-lock.el racket-repl.elc Install.log.gz racket-font-lock.elc racket-tests.el instrument.rkt racket-imenu.el racket-tests.elc keywords.rkt racket-imenu.elc racket-unicode-input-method.el logger.rkt racket-indent.el racket-unicode-input-method.elc mod.rkt racket-indent.elc racket-util.el racket-bug-report.el racket-keywords-and-builtins.el racket-util.elc racket-bug-report.elc racket-keywords-and-builtins.elc run.rkt racket-collection.el racket-make-doc.el scribble.rkt racket-collection.elc racket-make-doc.elc try-catch.rkt racket-common.el racket-mode-autoloads.el util.rkt

bremner commented 7 years ago

David Bremner david@tethera.net writes:

Greg Hendershott notifications@github.com writes:

Hmm. When you say "the package is already installed", do you mean using Emacs' package-install or something else?

Using package-install, but into a system wide directory

The problems seem to be caused by interactions with geiser. On the machine where the tests are failing I have (had) also installed geiser via package.el.

At least for one of the tests, the problem seems to be related to geiser helpfully starting scheme-mode (now geiser) for rkt files.

      (add-to-list 'auto-mode-alist '("\\.rkt\\'" . scheme-mode))

At a guess, the fact that this only manifests as a problem when racket-mode is also installed probably has something to do with load order.

If I think of a way to make the racket-mode tests more robust, I'll let you know.

greghendershott commented 7 years ago

Ah, interesting. I wonder what's a realistic scenario to test. I think most human users would install only one of these two packages -- or maybe, install both, but in their Emacs init file disambiguate mode they want as the default mode for .rkt files?

But to back up a step. It occurs to me now, need/ought you run packages' tests, at all?

I don't think that happens when people do the "normal" thing of using package-install?

(For example. I think some packages exclude test-related .el files from what MELPA distributes. Arguably it's wrong that the MELPA recipe for racket-mode is including them.)

bremner commented 7 years ago

Greg Hendershott notifications@github.com writes:

Ah, interesting. I wonder what's a realistic scenario to test. I think most human users would install only one of these two packages -- or maybe, install both, but in their Emacs init file disambiguate mode they want as the default mode for .rkt files?

Both modes attempt to take ownership of .rkt files, but geiser is also useful for several other scheme variants.

But to back up a step. It occurs to me now, need/ought you run packages' tests, at all?

Yes, I'm working on packaging racket-mode for Debian and this kind of integration testing is part of the point. The underlying conflict in auto-mode-alist is something we'll have to resolve somehow.

I don't think that happens when people do the "normal" thing of using package-install?

That's true. I'm not really doing the "normal thing" in that sense.

(For example. I think some packages exclude test-related .el files from what MELPA distributes. Arguably it's wrong that the MELPA recipe for racket-mode is including them.)

I'm not downloading the files from melpa; I don't really know what the general melpa policy is about test suites.

d

greghendershott commented 7 years ago

Thanks for explaining.

  1. I'll think about this more.

  2. In the meantime, my least-worst idea answer is that racket-mode expects that, if it is running, it's because the user wants to use it to handle .rkt files; otherwise it's not much use. :) The tests assume that. If the user's init file disambiguates which major mode is supposed to be set for .rkt files, and the answer is not racket-mode, then probably the correct thing to do is... not run the tests for the package that will not be used. Maybe the tests should check the .rkt mode and simply not run at all. idk

greghendershott commented 7 years ago

Or, maybe the tests that visit a .rkt file should explicitly call (racket-mode 1) -- similar to a user visiting the file, then manually doing a M-x racket-mode to set the mode.

The tests intentionally don't do that as an indirect way of making sure I set the auto-mode-alist and that it was effective. But, maybe that's "too strong" a test.

bremner commented 7 years ago

Greg Hendershott notifications@github.com writes:

Or, maybe the tests that visit a .rkt file should explicitly call (racket-mode 1) -- similar to a user visiting the file, then manually doing a M-x racket-mode to set the mode.

The tests intentionally don't do that as an indirect way of making sure I set the auto-mode-alist and that it was effective. But, maybe that's "too strong" a test.

I think the underlying issue is that calling "(package-initialize)" gives up control over the test environment. In a sense this is good, since we discovered a real world problem, but it makes the tests inherently unreprodicible. What do you think about setting 'package-load-list' to only load the packages needed for the tests?

d