Include exception that raised failure in backtrace #2074
Conversation
|
|
||
| unless potential_lines.empty? | ||
| lines += potential_lines.last | ||
| end |
There was a problem hiding this comment.
If you're going to only take the last cause, why bother generating the string for all of them? It would seem better to loop over the causes until you've found the last, then do the string generation and add the last line directly to lines.
There was a problem hiding this comment.
Good point, I'll update the pr to do the string generation for the last cause only 👍
|
Great start, I have some feedback from a mostly style point of view, but the rubocop build is also failing so thats needs addressing, let me know if you need any help. |
|
@JonRowe Thanks for the feedback, I'll update this pr tonight with your suggestsions and fix the failing build |
|
This all looks good, I have nothing to add on top of what Jon said. Let us know if you need any further help :) |
|
@JonRowe I've addressed a few of the issues you mentioned but my solution still needs some refactoring and I need to get the build passing. I'll review this tomorrow and see if I can get this finished off |
|
@samphippen The build is broken and I don't fully understand why, can you please point me in the right direction so I can figure out what the problem is? @JonRowe suggested I pin the https://github.com/rspec/rspec-core/blob/master/script/run_build#L28 These are the builds that fail and below them I've printed out the part that shows which 1.8.7 https://travis-ci.org/rspec/rspec-core/jobs/83143375 *
Below is the
|
|
@mrageh -- the problem you are running into is described in rspec/rspec-dev#137. We're going to have to fix our merge the rspec-support PR or change our build scripts to fix it. For now don't worry about it. |
|
@mrageh -- I just merged rspec/rspec-support#240. You should be able to remove your temporary pin to the rspec-support branch and the build should work now. (Please force push so the temporary pin isn't in our history). |
|
thank you @myronmarston |
|
@myronmarston I've forced pushed and the commit containing temporary pin is no longer in my branch. However the build is still failing on Ruby 2.2 and 2.1 and on Ruby 2.1 the failure makes little sense to me |
| def formatted_cause(exception) | ||
| exceptions = [] | ||
|
|
||
| while exception.respond_to?(:cause) && exception.cause |
There was a problem hiding this comment.
Is the respond_to? check here necessary? Under what conditions would Exception.method_defined?(:cause) return true (that's the logic for the supports_exception_cause? check) and exception.respond_to?(:cause) return false?
There was a problem hiding this comment.
A few of the tests within describe '#fully_formatted' in https://github.com/rspec/rspec-core/blob/master/spec/rspec/core/formatters/exception_presenter_spec.rb fail because instance_double received an unexpected message :cause. So in order to side step this issue and keep the diff small I decided to use exception.respond_to?(:cause). That way I would not need to add a before block stubbing out the method or stub it out in each particular instance.
However now that I've finished the initial implementation and this pr has been reviewed I think it's good that I stub this method out in each instance_double, what would you do in this case?
There was a problem hiding this comment.
Actually, I think I'd favor not using test doubles for this case. Exceptions aren't hard or expensive to create, and we're not designing the exception API, so I don't see much benefit to using test doubles for the exceptions here.
There was a problem hiding this comment.
Should I change all the existing exception test doubles to actual Exceptions?
There was a problem hiding this comment.
It's not necessary but if you want to - go for it! I'd just ask that you do that in a separate commit.
There was a problem hiding this comment.
There are seven failing tests and that's cause they are test doubles of the Exception class, so I need to either stub the method or I could just use actual instances of the Exception class. Since we both prefer not using doubles in this case, I think it's best I just change the failing tests to use real objects.
I'll do it in a separate commit thanks for the swift response
There was a problem hiding this comment.
I spent sometime trying to change the three exception test doubles to actual instances of exception. but that would mean I still have to stub the cause, message and backtrace for a lot of the tests in a before block. @myronmarston do you think I should still use real instances of exception?
There was a problem hiding this comment.
If you use a real exception, please do not stub anything on it. Instead, you can set the exception by passing it as an argument to Exception.new. You can set the backtrace by calling exception.set_backtrace. You can set the cause by simulating the conditions where ruby sets the cause:
irb(main):023:0> ex = begin
irb(main):024:1* begin
irb(main):025:2* raise "root cause"
irb(main):026:2> rescue
irb(main):027:2> raise "from rescue"
irb(main):028:2> end
irb(main):029:1> rescue => e
irb(main):030:1> e
irb(main):031:1> end
=> #<RuntimeError: from rescue>
irb(main):032:0> ex.cause
=> #<RuntimeError: root cause>
There was a problem hiding this comment.
Thank you I think I prefer using test_doubles of the Exception class in this case as I think that looks nicer, that's only if you have no objections to me doing that 👍
|
There are two things at work causing the build failures:
Thanks for following through on this. Our build can be a bit fidly but helps ensure consistency and better code quality. |
|
@myronmarston thank you :) |
|
@myronmarston I've kind of finished the PR, you can see the changes I introduced to fix the failing build in the last commit.... |
|
What are the next steps for this pull request? |
|
this LGTM, @JonRowe @myronmarston. This good to merge? |
|
Needs to be squashed first please |
| lines = backtrace_formatter.format_backtrace((exception.backtrace || []), example.metadata) | ||
| potential_lines = formatted_cause(exception) | ||
|
|
||
| lines += potential_lines unless potential_lines.empty? |
There was a problem hiding this comment.
[a+b+c] + [] is [a+b+c] so it really doesn't matter if its empty
|
@JonRowe thanks for the review. I've changed my solution a bit and I like this approach better, instead of using a |
| def formatted_backtrace(exception=@exception) | ||
| lines = backtrace_formatter.format_backtrace((exception.backtrace || []), example.metadata) | ||
|
|
||
| lines + formatted_cause(exception) |
There was a problem hiding this comment.
I think it makes sense to collapse these lines and just break it up over two lines. e.g.
backtrace_formatter.format_backtrace((exception.backtrace || []), example.metadata) +
formatted_cause(exception)
|
@JonRowe After applying the changes you suggested the build has started failing and the failure messages do not contain descriptive error messages. |
|
The build is currently failing due to some recent changes @JonRowe made to our license file in master. Reasonably, he pushed to master expecting it to not have an affect on the build but it actually does (due to YARD/gemspec references to the license file and whitespace issues). He's got a PR out to fix the issues and I expect it to be fixed later today. You'll have to rebase after those get merged and then your PR should (hopefully) go green. Sorry about the failures noise! |
|
The build should be fixed now, so if you rebase (and squash please!) you should find that this passes |
|
No problem @myronmarston |
|
@myronmarston and @JonRowe you've both been really helpful and I enjoyed working on this issue. This is my first time contributing to rspec and I've got some questions/feedback that may make it easier for new comers to contribute to this project. If you're interested in hearing my feedback please let me know |
Problem
Sometimes rspec-core does not display the underlying cause of an
exception that was raised.
Previously if an exception was raised there would sometimes be an
underlying cause of the exception. For example an exception that's
raised and rescued within an application causes the exception that
is displayed in the stacktrace.
```
class BrokenCode
def self.shallow_method
begin
deep_method
rescue
raise "Something happened and I'm hiding that from the message
'because usability'."
end
end
def self.deep_method
raise 'The real cause for the failure is here!'
end
end
RSpec.describe 'some broken code' do
it 'works' do
BrokenCode.shallow_method
end
end
```
```
1) some broken code works
Failure/Error: raise "Something happened and I'm hiding that from
the message 'because usability'."
RuntimeError:
Something happened and I'm hiding that from the message 'because
usability'.
# ./my_spec.rb:32:in `rescue in shallow_method'
# ./my_spec.rb:29:in `shallow_method'
# ./my_spec.rb:43:in `block (2 levels) in <top (required)>'
```
The above example demonstrates the problem, the stacktrace above is not
very clear as it does not show the initial exception that caused the
problem. This makes it difficult to fix their failing test.
Solution
Ruby 2.1 introduced `Exception#cause` which shows the underlying cause
of an exception if there is one. This commit uses that new Ruby method
to get the cause of an exception and display it in the stacktrace.
The stacktrace for the above code example would change to the below
example:
```
1) some broken code works
Failure/Error: raise "Something happened and I'm hiding that from
the message 'because usability'."
RuntimeError:
Something happened and I'm hiding that from the message 'because
usability'.
# ./my_spec.rb:32:in `rescue in shallow_method'
# ./my_spec.rb:29:in `shallow_method'
# ./my_spec.rb:43:in `block (2 levels) in <top (required)>'
------------------
--- Caused by: ---
RuntimeError:
The real cause for the failure is here!
# ./my_spec.rb:37:in `deep_method'
# ./my_spec.rb:30:in `shallow_method'
# ./my_spec.rb:43:in `block (2 levels) in <top (required)>'
```
This makes it a lot easier to fix the main cause of the test failure.
Edge case
There may be a situation where lots of exceptions are captured and
bubble up the stacktrace. In that case I don't think we want to print out
the cause of every single exception. Instead what I propose we do is to
print out the first exception that caused the other exceptions to bubble
up the stack trace.
|
Yes, we're always interested in feedback in how to make the contributing process smoother. You can either write it here or email it to us directly if you prefer. You can get our email addresses from our git commits: |
|
This LGTM to me now. Thanks for the hard work, @mrageh! |
…-stacktrace-in-exception-backtrace Include exception that raised failure in backtrace
`Exception.new.backtrace` is `nil` so we can't count on there always being a backtrace. Before now we relied on the `format_backtrace` caller handling this (passing `ex.backtrace || []`), but with #2074 now also including the `exception.cause` in failure output, the code there did not guard the `format_backtrace` call with `|| []` and is causing users to get a `NoMethodError`. It's easiest if `format_backtrace just handles the `nil` case so each caller does not have to remember to handle it.
`Exception.new.backtrace` is `nil` so we can't count on there always being a backtrace. Before now we relied on the `format_backtrace` caller handling this (passing `ex.backtrace || []`), but with #2074 now also including the `exception.cause` in failure output, the code there did not guard the `format_backtrace` call with `|| []` and is causing users to get a `NoMethodError`. It's easiest if `format_backtrace just handles the `nil` case so each caller does not have to remember to handle it. Fixes #2117.
`Exception.new.backtrace` is `nil` so we can't count on there always being a backtrace. Before now we relied on the `format_backtrace` caller handling this (passing `ex.backtrace || []`), but with #2074 now also including the `exception.cause` in failure output, the code there did not guard the `format_backtrace` call with `|| []` and is causing users to get a `NoMethodError`. It's easiest if `format_backtrace just handles the `nil` case so each caller does not have to remember to handle it. Fixes #2117.
`Exception.new.backtrace` is `nil` so we can't count on there always being a backtrace. Before now we relied on the `format_backtrace` caller handling this (passing `ex.backtrace || []`), but with rspec#2074 now also including the `exception.cause` in failure output, the code there did not guard the `format_backtrace` call with `|| []` and is causing users to get a `NoMethodError`. It's easiest if `format_backtrace just handles the `nil` case so each caller does not have to remember to handle it. Fixes rspec#2117.
…aused-stacktrace-in-exception-backtrace Include exception that raised failure in backtrace
`Exception.new.backtrace` is `nil` so we can't count on there always being a backtrace. Before now we relied on the `format_backtrace` caller handling this (passing `ex.backtrace || []`), but with rspec#2074 now also including the `exception.cause` in failure output, the code there did not guard the `format_backtrace` call with `|| []` and is causing users to get a `NoMethodError`. It's easiest if `format_backtrace just handles the `nil` case so each caller does not have to remember to handle it. Fixes rspec#2117.
`Exception.new.backtrace` is `nil` so we can't count on there always being a backtrace. Before now we relied on the `format_backtrace` caller handling this (passing `ex.backtrace || []`), but with rspec/rspec-core#2074 now also including the `exception.cause` in failure output, the code there did not guard the `format_backtrace` call with `|| []` and is causing users to get a `NoMethodError`. It's easiest if `format_backtrace just handles the `nil` case so each caller does not have to remember to handle it. Fixes rspec/rspec-core#2117. --- This commit was imported from rspec/rspec-core@92c95d5.
Problem
Sometimes rspec-core does not display the underlying cause of an
exception that was raised.
Previously if an exception was raised there would sometimes be an
underlying cause of the exception. For example an exception that's
raised and rescued within an application causes the exception that
is displayed in the stacktrace.
The above example demonstrates the problem, the stacktrace above is not
very clear as it does not show the initial exception that caused the
problem. This makes it difficult to fix their failing test.
Solution
Ruby 2.1 introduced
Exception#causewhich shows the underlying causeof an exception if there is one. This commit uses that new Ruby method
to get the cause of an exception and display it in the stacktrace.
The stacktrace for the above code example would change to the below
example:
This makes it a lot easier to fix the main cause of the test failure.
Edge case
There may be a situation where lots of exceptions are captured and
bubble up the stacktrace. In that case I don't think we want to print out
the cause of every single exception. Instead what I propose we do is to
print out the first exception that caused the other exceptions to bubble
up the stack trace.
This may be a potential fix for #2044