Toggling between spec and implementation is broken in some cases

[dgtized]

I started trying to write a test case for this and then ran into some difficulty modeling the file system.

Basically if you have a spec file spec/lib/foo_spec.rb and the implementation lib/foo.rb, running C-c , t to flip from the spec to the implementation works, but switching from the implementation to the test fails (it tries to generate spec/foo_spec.rb even though the spec/lib/foo_spec.rb exists).

I traced it down to rspec-target-file-for considers multiple candidates, but rspec-spec-file-for doesn't look at candidates.

I know there is some dispute on if spec/lib should even exist, but it seems like we should do candidate search in both directions if possible?

[dgutov]

I'm not sure I understand what exact scenario it breaks in. It seems to work well for me, both in projects with spec/lib and without.

The directory names which "should not exist" under spec are configurable via rspec-primary-source-dirs.

[dgtized]

(defvar base (rspec-project-root (expand-file-name "spec/lib/gender_spec.rb")))

;; Both spec/lib/gender_spec.rb & lib/gender.rb exist
(file-relative-name (rspec-target-file-for (expand-file-name "spec/lib/gender_spec.rb")) base)
"lib/gender.rb"

(file-relative-name (rspec-spec-file-for (expand-file-name "lib/gender.rb")) base)
"spec/gender_spec.rb"

;; When running C-c , t in the implementation of lib/gender.rb it can't find
;; spec/gender_spec.rb because it's in spec/lib, whereas in the spec file it can
;; always fine the implementation.

;; Note that `rspec-target-file-for' loops over candidates:
(defun rspec-target-file-for (a-spec-file-name)
  "Find the target for A-SPEC-FILE-NAME."
  (cl-loop for extension in (list "rb" "rake")
           for candidate = (rspec-targetize-file-name a-spec-file-name
                                                       extension)
           for filename = (cl-loop for dir in (cons "."
                                                    rspec-primary-source-dirs)
                                   for target = (replace-regexp-in-string
                                                 "/spec/"
                                                 (concat "/" dir "/")
                                                 candidate)
                                   if (file-exists-p target)
                                   return target)
           if filename
           return filename))


;; And rspec-spec-file-for, does not:

(defun rspec-spec-file-for (a-file-name)
  "Find spec for the specified file."
  (if (rspec-spec-file-p a-file-name)
      a-file-name
    (let ((replace-regex (if (rspec-target-in-holder-dir-p a-file-name)
                             "^\\.\\./[^/]+/"
                           "^\\.\\./"))
          (relative-file-name (file-relative-name a-file-name (rspec-spec-directory a-file-name))))
      (rspec-specize-file-name (expand-file-name (replace-regexp-in-string replace-regex "" relative-file-name)
                                                 (rspec-spec-directory a-file-name))))))

[dgtized]

rspec-primary-source-dirs =>
("app" "lib")

Are you suggesting I need to adjust that variable differently?

[dgutov]

Note that `rspec-target-file-for' loops over candidates

It loops over the possible prefixes to add to the relative name, to determine the target file name. When both app/foo.rb and lib/foo.rb map to spec/foo_spec.rb, you have to loop to find whichever one that exists.

Are you suggesting I need to adjust that variable differently?

Try removing lib from its value. Or set it to nil altogether, if you have same situation with app.

[dgutov]

And rspec-spec-file-for, does not

It kind of does, inside rspec-target-in-holder-dir-p.

[dgtized]

Awesome! I used:

(setq rspec-primary-source-dirs '("app"))

And it fixed it. Happy to close this given it now works, but wonder if should include this example in the documentation somewhere? Or am I just blind and missed it?

[dgutov]

I don't know whether the lack of an example was really the problem. Have you even noticed that variable before?

[dgtized]

Yep, I noticed it while trying to figure it out, but it wasn't clear what it did. For one the variable sounds like it includes app and lib as viable targets for sources, and I must confess it wasn't until looking at it again after modifying that I realized it excluded lib.

I think I'm still a little unclear on how it excludes, like if I remove "app" from the list, what cases would break?

Just did some more experimentation, and it appears if "lib" is missing from that list then searching for matching specs for things in app/model now break instead. So neither appears to cover both cases. I will try and exhaustively write out the tests that work for each condition later.

[dgutov]

sounds like it includes app and lib as viable targets for sources

Maybe the name is not ideal. But you can search for the discussion where it was introduced, and I didn't see any better suggestions.

The list of viable targets for sources is not needed. If you have spec/foo/bar_spec.rb, the toggle function can just look for foo/bar.rb, there's no performance penalty associated with that. It does, however, need some knowledge to look for app/foo/bar.rb and lib/foo/bar.rb as well.

if I remove "app" from the list

That will mean that app/foo/bar.rb maps to spec/app/foo/bar_spec.rb.

if "lib" is missing from that list then searching for matching specs for things in app/model now break

I don't see why that would be the case. Any specific examples?

[dgutov]

If the docstring is not good enough, please suggest some improvements.

[dgtized]

(defvar base (rspec-project-root (expand-file-name "spec/lib/gender_spec.rb")))
(let ((rspec-primary-source-dirs '("app")))
  (list
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/lib/gender_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "lib/gender.rb")) base)
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/models/user_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "app/models/user.rb")) base)))
("lib/gender.rb" "spec/lib/gender_spec.rb" "app/models/user.rb" "spec/models/user_spec.rb")

(let ((rspec-primary-source-dirs '("lib")))
  (list
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/lib/gender_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "lib/gender.rb")) base)
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/models/user_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "app/models/user.rb")) base)))
;; (wrong-type-argument stringp nil)
;;   expand-file-name(nil)


(let ((rspec-primary-source-dirs '()))
  (list
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/lib/gender_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "lib/gender.rb")) base)
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/models/user_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "app/models/user.rb")) base)))
;; (wrong-type-argument stringp nil)
;;   expand-file-name(nil)

(let ((rspec-primary-source-dirs '("app" "lib")))
  (list
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/lib/gender_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "lib/gender.rb")) base)
   (file-relative-name (rspec-target-file-for (expand-file-name "spec/models/user_spec.rb")) base)
   (file-relative-name (rspec-spec-file-for (expand-file-name "app/models/user.rb")) base)))
("lib/gender.rb" "spec/gender_spec.rb" "app/models/user.rb" "spec/models/user_spec.rb")

Sorry I don't have the best suggestion immediately on naming it clearer, still having some difficulty understanding what it does exactly. How about this, I'll try again to make a PR to add the test cases so at minimum someone else who is confused can see an example of how it changes things in the specs & then can see if we can improve the name?

[dgtized]

Also, thanks for the assistance!

[dgutov]

@dgtized What is the problem with the

(let ((rspec-primary-source-dirs '("app")))

example?

[dgutov]

How about this, I'll try again to make a PR to add the test cases so at minimum someone else who is confused can see an example of how it changes things in the specs & then can see if we can improve the name?

Please go ahead, and use ERT for them.

[dgtized]

Sorry to clarify, the '("app") example is what I am now using and is the correct behavior. I just listed each of them to show the outputs for each, mostly to inform what the ERT test should be. I think there may be an error for the "lib" only case that might need better rescuing. Will try and get to tests. Thanks again for the help.

[gcentauri]

This is somewhat related, but a different use case. I'm working in a code base with an unorthodox arrangement of the spec directory. So the unit tests I'd like to switch to are in spec/unit_tests where then i have app, lib etc directories. I couldn't see an easy way to configure which directory to look in for a scenario like this. Is that something that could be added to rspec-mode or would it be better to encourage this code base to change? I can work around it using symlinks for now.

[dgutov]

@gcentauri I suppose it could be customizable. See rspec-target-file-for and rspec-spec-file-for for the logic to change.