-
Notifications
You must be signed in to change notification settings - Fork 248
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
fix crash by returning after notify_failure #314
Conversation
1 similar comment
Thanks. This should have a test at least - but I don't have enough info to reproduce the problem. Could you add https://github.com/bronson/guard-rspec/blob/patch-1/lib/guard/rspec/runner.rb#L77 I'm guessing the file doesn't exist or something. The code is bad there, because it should at least show an error in that case, and not hide the exception. I don't mind pulling this change in - I'd just prefer to find the actual cause and prevent it from ever causing trouble again. |
Always happy to understand a bug rather than just shooting in the dark. Your guess is close. The entire tmp directory doesn't exist (since this isn't a Rails project). Should that be /tmp?
And |
Hm, creating a |
So the file can't be created because, get this, somebody didn't require 'fileutils'. The missing require was hidden because all exceptions are getting slurped up. I think the comment at rspec_formatter.rb:74 is not actually true. If it had printed something, this would have been a 2 minute bughunt. |
The missing require was hidden because of the rescue all at line 74.
Adding the require seems to fix everything. But, quietly creating a tmp dir in a project that doesn't need one seems like an antifeature to me. |
fix crash by returning after notify_failure
Thanks for investigating!
That's why I love getting structured exceptions and stack traces - instead of the "user friendly" idea of "not crashing". Especially since most RSpec users are developers anyway. The whole gem probably needs some reviewing - though I guess most people would be too afraid to touch anything (since it's a popular gem - and quite complex too).
That's because the formatter has to somehow communicate (the test results) with the main process. An environment variable may be a better idea - then the temp file could be in /tmp or whatever. I might look into it, but I can't make promises. |
Thanks! I'd be annoyed except I'm not volunteering for the cleanup either... I'll take what I can get :) I suppose the main process forks the formatter? Could it read the formatter's output over a pipe? I wrote that for Shrimple, could look into doing it here too. |
That's been considered (for other purposes). The issue is: when using a pipe, you'll get colored ANSI output - which means there would be an extra step of filtering, etc. That's why a custom formatter is used - because that's how you can get RSpec to create "normal" output. Also, the file with output is useful to keep anyway - beyond just guard-rspec (for other tools like edit integration, etc.). So we quickly get into some kind of persistent storage, e.g. files, redis, or at least a socket. Everything else is just a poor-man's workaround. Overall guard (and plugins) would benefit from persistent storage anyway - but that's a project in of itself. |
Guard crashes on my machine unless I insert this return statement. Dunno if it's the right fix.
What's funny is that the bug seems to be introduced as a result of RuboCop? 846ca48
Here's the crash: