jorgenschaefer / emacs-buttercup

Behavior-Driven Emacs Lisp Testing
GNU General Public License v3.0
360 stars 44 forks source link

Use default-directory instead of dot #217

Closed jcs090218 closed 2 years ago

jcs090218 commented 2 years ago

Propose fix to #62.

jcs090218 commented 2 years ago

@snogge WDYT?

Example log, https://github.com/emacs-eask/eask/runs/6279208042?check_suite_focus=true.

Loading package information... done ✓
  - Installing buttercup ([20](https://github.com/emacs-eask/eask/runs/6279208042?check_suite_focus=true#step:7:20)220410.1557)... done ✓
Running 1 specs.

File failed to load completely: 
  ./test/buttercup-test.el
  ./test/buttercup-test.el  Cannot open load file: No such file or directory, ./test/buttercup-test.el (0.04ms)

========================================
File failed to load completely:  ./test/buttercup-test.el
snogge commented 2 years ago

@jcs090218, sorry about not replying sooner.

This looks OK but I'd really like to understand the problem better to see whether we need new tests.

I tried looking in eask to understand the directory setup you have problems with but I must admit I'm not motivated enough to read all of the code to figure it out. Could you explain the directory structure and the current directory?

And is this only on Windows? Because I think there are tests that should already cover this, but maybe they are not correct.

jcs090218 commented 2 years ago

No problem at all! ❤️

By further investigate this issue. I realized this can reproduced my calling buttercup-run-discover function through code. The way I called it in my config:

(make sure you swap your workspace to correct relative path)

(defun my-buttercup-run ()
  (interactive)
  (buttercup-run-discover))

In Eask,

(eask-start
  (eask-with-archives "melpa"
    (eask-package-install 'buttercup))
  (require 'buttercup)
  (buttercup-run-discover))

❌ Then you get the error:

Running 1 specs.

File failed to load completely: 
  ./test/buttercup-test.el
  ./test/buttercup-test.el  Cannot open load file: No such file or directory, ./test/buttercup-test.el (0.07ms)

========================================
File failed to load completely:  ./test/buttercup-test.el

Traceback (most recent call last):
  load("./test/buttercup-test.el" nil t nil nil)
FAILED: Cannot open load file: No such file or directory, ./test/buttercup-test.el

Ran 1 specs, 1 failed, in 0.29ms.

✔ After applying this patch:

Running 1 specs.

A suite
  contains a spec with an expectation
  contains a spec with an expectation (0.17ms)

Ran 1 specs, 0 failed, in 0.32ms.

So my question is, does buttercup-run-discover support by calling from the code?

jcs090218 commented 2 years ago

I am not 100% percent sure about the reason. My best guess is "." points to somewhere but not in the workspace, maybe the start up path by read this stackoverflow here (hence it will work from CLI but not from code). But default-directory holds the full path and always points to the workspace (unless you changed it). I think default-directory is a better option and simply more explicit in this case, so that you could do the following:

;; Run test for first project
(let ((default-directory "path/to/project-1/"))
  (buttercup-run-discover))
;; Run test for second project
(let ((default-directory "path/to/project-2/"))
  (buttercup-run-discover))

WDYT? 😖

jcs090218 commented 2 years ago

@snogge Let me know if you have any question to this PR. This is a friendly ping! :)

akirak commented 2 years ago

I made the same fix in #204 (which I closed later) for a non-eask case. I am not sure about the cause, but the change seems to solve the issue.

jcs090218 commented 2 years ago

Maybe something between "." relative path vs default-directory absolute path? 😕

akirak commented 2 years ago

Perhaps load searches in load-path unless the file name is absolute. load-file is defined as follows:

(defun load-file (file)
  "Load the Lisp file named FILE."
  ;; This is a case where .elc and .so/.dll make a lot of sense.
  (interactive (list (let ((completion-ignored-extensions
                            (remove module-file-suffix
                                    (remove ".elc"
                                            completion-ignored-extensions))))
               (read-file-name "Load file: " nil nil 'lambda))))
  (load (expand-file-name file) nil nil t))
jcs090218 commented 2 years ago

Okay, I think I found the culprit.


 (defun buttercup--load-test-file (file failure-suite)
   "Load FILE keeping track of failures.
 If an error is raised by `load', store the error information in a
 spec and add it to FAILURE-SUITE."
   (cl-destructuring-bind (status description stack)
+      (progn
+        (buttercup--funcall #'load-file file nil t)         ; freezes Emacs for some reason...
+        (buttercup--funcall #'load-file file)               ; this work, best option?
+        (list 'passed (apply #'load-file (list file)) nil)  ; this work
+        (list 'passed (apply #'load (list file)) nil)       ; error, cannot find file
+        )
     (when (eq status 'failed)
       ;; Skip all of stack until load is called
       (while (not (eq (nth 1 (car stack)) 'load))
         (pop stack))
       (buttercup-suite-add-child
        failure-suite
        (make-buttercup-spec
         :description file
         :status 'failed
         :failure-description (error-message-string (cadr description))
         :failure-stack stack
         :function (lambda () (ignore)))))))

Propose fix is to use load-file and not load. But either this patch or that would work.

snogge commented 2 years ago

Hi all,

Just a notice that I'm not ignoring this issue. If I'm not able to get to it before I hope to have more time to devote to buttercup.el from next week.

To answer a few points raised in this thread;

Switching "." to default-directory will probably cause all found files to be reported with absolute paths as all files returned by directory-files-recursively are prefixed with the value of the DIR argument. I have not looked into how load uses relative paths against load-path yet, but this could be relevant.

All the docs say that you should add . to the load path with -L .. What happens if you try

(let ((load-path (cons "." load-path)))
  (buttercup-run-discover))
jcs090218 commented 2 years ago

That make sense! :) Thanks for the reply!

Edit: Tested and it works! :D

(let ((load-path (cons "." load-path)))
  (buttercup-run-discover))