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

When Using Spring, Specs Won't Run Unless rspec Has Been Installed as a Spring Command #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sshaw
Copy link

@sshaw sshaw commented Jul 31, 2015

This adds a check for the SPRING_DISABLE environment variable.

Also note that even though Spring was not running it still tried to execute spring rspec .... This resulted in Spring's usage.

@@ -583,6 +583,7 @@ file if it exists, or sensible defaults otherwise."

(defun rspec-spring-p ()
(and rspec-use-spring-when-possible
(not (equal (getenv "SPRING_DISABLE") "1"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't it be set to a different non-empty value?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be set to anything. How about I do this:

(member (getenv "SPRING_DISABLE") '(nil "" "0"))

Or this:

(or
  (null (getenv "SPRING_DISABLE"))
  (equal "" (getenv "SPRING_DISABLE"))
  (equal "0" (getenv "SPRING_DISABLE")))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But where do we stop, should we also check for "false", "off", etc..?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does 0 mean "not disable"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose so, then. Check all three of these possible values, using member.

It's doesn't seem like false, etc, should be treated similarly: https://github.com/rails/spring/blob/7efd492018049df43310feb89acbaf226446a123/lib/spring/binstub.rb

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please fix the name of the variable (words in wrong order).

@dgutov
Copy link
Collaborator

dgutov commented Jul 31, 2015

Also note that even though Spring was not running it still tried to execute spring rspec ....

Maybe you had a .pid file left over? See rspec-spring-p.

@sshaw
Copy link
Author

sshaw commented Jul 31, 2015

Also note that even though Spring was not running it still tried to execute spring rspec ....

Maybe you had a .pid file left over? See rspec-spring-p.

Sure, though have you considered using (process-live-p PID) or spring status? Of course these may be overkill...

@dgutov
Copy link
Collaborator

dgutov commented Jul 31, 2015

process-live-p doesn't take PID as argument.

@sshaw
Copy link
Author

sshaw commented Jul 31, 2015

process-live-p doesn't take PID as argument.

Ah of course, a process object... 💨

@sshaw
Copy link
Author

sshaw commented Jul 31, 2015

process-live-p doesn't take PID as argument.

There's always (memq PID (list-system-processes)). But I'm getting sidetracked...

@dgutov
Copy link
Collaborator

dgutov commented Jul 31, 2015

Hmm, actually, since setting this var makes Spring not do anything, why bother with it on our end?

@sshaw
Copy link
Author

sshaw commented Jul 31, 2015

Hmm, actually, since setting this var makes Spring not do anything, why bother with it on our end?

If Spring's pid file is there but Spring is not running, then (rspec-verify) will run spring and the result will be its usage. So yes, the current approach is not correct, checking the pid would more suitable.

I can modify.

@sshaw sshaw changed the title Check SPRING_DISABLE environment variable Specs Don't Run When Spring's PID File Exists but Spring Is Not Running Jul 31, 2015
@dgutov
Copy link
Collaborator

dgutov commented Jul 31, 2015

If Spring's pid file is there but Spring is not running, then (rspec-verify) will run spring and the result will be its usage.

Please clarify: this is unrelated to DISABLE_SPRING, right?

What exactly happens? "Spring gets used" or "specs don't run"?

@sshaw
Copy link
Author

sshaw commented Jul 31, 2015

If Spring's pid file is there but Spring is not running, then...
Please clarify: this is unrelated to DISABLE_SPRING, right?

Yes

What exactly happens? "Spring gets used" or "specs don't run"?

I don't normally use Spring so it was not obvious at first, but after doing some research it turns out that it fails when spring is running because rspec is not available as a spring command:

spring rspec --format documentation /Users/sshaw/code/ruby/txs/spec/models/source_spec.rb
Version: 1.3.6

Usage: spring COMMAND [ARGS]

Commands for spring itself:
...

You can only call spring rspec ... if rspec has been installed as a spring command.

@sshaw sshaw changed the title Specs Don't Run When Spring's PID File Exists but Spring Is Not Running When Using Spring, Specs Won't Run Unless rspec Has Been Installed as a Spring Command Jul 31, 2015
@dgutov
Copy link
Collaborator

dgutov commented Jul 31, 2015

You can only call spring rspec ... if rspec has been installed as a spring command.

Ok. Do you think rspec-mode should do anything about that?

@sshaw
Copy link
Author

sshaw commented Aug 1, 2015

Ok. Do you think rspec-mode should do anything about that?

The mode performs several checks in an effort to find the right option. It finds bundle exec -which would work fine, but it chooses to use spring -which doesn't. So yes, I think it should.

Maybe something like this:

(defun rspec-spring-p ()
  (and rspec-use-spring-when-possible
    (stuff-that-looks-up-spring-pid-file)
    (not (cl-notany (lambda (s) (string-match "^\s+rspec\s" s)) (process-lines "spring" "help")))))

@dgutov
Copy link
Collaborator

dgutov commented Aug 1, 2015

spring help takes half a second on my machine. Are you sure the certainty is worth the delay?

@sshaw
Copy link
Author

sshaw commented Aug 1, 2015

spring help takes half a second on my machine. Are you sure the certainty is worth the delay?

Is saving 0.5 seconds worth the bug?

@dgutov
Copy link
Collaborator

dgutov commented Aug 1, 2015

Why not?

And if Spring is installed, not running specs through it is unlikely to be what the user wants. Rather, they want to be notified that spring-commands-rspec is required for that.

That could be done with a message at the bottom of the compilation buffer. Like rspec-handle-error currently does for another problem.

@sshaw
Copy link
Author

sshaw commented Aug 1, 2015

Is saving 0.5 seconds worth the bug?

Why not?

(sigh) okay, let's agree that it is worth the bug.

And if Spring is installed, not running specs through it is unlikely to be what the user wants.

Maybe, maybe not. Is this what your experience has shown? Over the past 24 hours here are the problems I've had with spring + rspec:

  1. specs ran with rspec pass and fail with spring rspec
  2. spec fails, change a line, fails again, output shows old line
  3. rspec-mode sometimes it runs bundle exec other times spring rspec

That could be done with a message at the bottom of the compilation buffer.

Certainly not a bad approach but, as you've said, it assumes that those using spring + rspec
always want to run spring rspec.

@dgutov
Copy link
Collaborator

dgutov commented Aug 2, 2015

Is this what your experience has shown?

Not really. I haven't used Rails for quite a while, so I haven't used Spring either. But you're the first to complain about it here.

it assumes that those using spring + rspec always want to run spring rspec

Why else would they be running Spring?

But anyway, they can avoid it by setting rspec-use-spring-when-possible to nil, for example. No need to spend those 0.5 seconds each time.

rspec-mode sometimes it runs bundle exec other times spring rspec

That's normal. IIRC, Spring performs bundle setup by itself.

@carlthuringer
Copy link

carlthuringer commented Oct 19, 2017

FWIW I also ran into this issue and after pondering and poking around at this for a little while, I realized that nobody on my team knows what spring is or what it's for. It was just cargo-culted in from rails new.

What we do know about and use is rspec, which is not included when you rails new. Actually since we're using rails and rspec, we use rspec-rails, then rails generate rspec:install. And that doesn't happen to add spring-commands-rspec to the bundle.

So I propose that this is a problem with rspec-rails, which should recognize that rails is distributing spring and that there's an integration necessary for full functionality: spring-commands-rspec.

Edit: the maintainer politely responded and closed as you can see:

Why doesn't rspec-rails add spring-commands-rspec as a development dependency and generate the stub after rails generate rspec:install?

Because spring is not a requirement of using rspec-rails or rails, in fact many of us advocate not using spring at all as it causes more problems than it fixes.

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 this pull request may close these issues.

3 participants