`should_not raise_error` annoyingly swallows original exception backtrace #59

Closed
myronmarston opened this Issue Feb 7, 2011 · 11 comments

Comments

Projects
None yet
6 participants
Owner

myronmarston commented Feb 7, 2011

Given a spec like this:

describe "something" do
  it "does not raise an error" do
    expect { something }.to_not raise_error
  end
end

When an error is raised, the spec fails (as it should) but the failure backtrace isn't very useful. The backtrace I care about is for the original error, not the error raised by the raise_error matcher. Really, it would be better to use it("does not raise an error") { something } but the matcher may still be useful in some situations (i.e. to ensure every spec has at least one expectation) and I've got some error reports that have unhelpful backtraces because of this (see this VCR issue, for example).

It would be nice to have the raise_error matcher preserve the original exception backtrace. A few ideas:

  1. Maybe raise_error in the should_not case should be pure syntactic sugar that doesn't actually do anything but execute the block? That way the original error (with associated backtrace) will bubble up and get printed. Users can still use the matcher to make it clear what expectation they are setting for the example.
  2. Alternately, the matcher could print out the original exception's backtrace as part of the failure message. Presumably, we'd want it to respect the full_backtrace configuration option, although we'd need to find a way to do that in such a way that it doesn't couple rspec-expectations to rspec-core.

Myron

Owner

dchelimsky commented Feb 18, 2011

I'd prefer option 2.

Contributor

alindeman commented Apr 16, 2011

What are the pros of 2? To me, 1 sounds better.

Owner

dchelimsky commented Apr 16, 2011

It's a failed expectation. I think it's more consistent with other failure messages if this gets wrapped up in a failure message rather than bubbling up directly. Why do you like 1 better?

chendo commented Dec 20, 2011

I haven't got the time to write proper tests etc for it at the moment, but this is a fairly simple version of the RaiseException matcher, but puts the backtrace the message. Probably could do with cleaning out the backtrace etc, but it works (at least with 1.3.2)

https://gist.github.com/1500276

myronmarston added a commit that referenced this issue Jul 13, 2012

Loosen error message constraints for raise_error matcher specs.
This is in prep for #59--once I had some backtrace info, all of these would fail since they are doing exact string matches.
Owner

myronmarston commented Sep 11, 2012

I took a first pass at implementing number 2 from above, but it's tricky to figure out how to format this correctly. Here's what I've got so far:

  1) should_not raise_error includes the backtrace of the error that was raised in the error message
     Failure/Error: expect { raise "boom" }.not_to raise_error
       expected no Exception, got #<RuntimeError: boom> with backtrace:
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/spec/rspec/matchers/raise_error_spec.rb:56:in `block (3 levels) in <top (required)>'
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/raise_error.rb:24:in `call'
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/raise_error.rb:24:in `matches?'
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/raise_error.rb:43:in `does_not_match?'
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/expectations/handler.rb:31:in `handle_matcher'
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/lib/rspec/expectations/expectation_target.rb:48:in `to_not'
         # /Users/myron/code/rspec-dev/repos/rspec-expectations/spec/rspec/matchers/raise_error_spec.rb:56:in `block (2 levels) in <top (required)>'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:113:in `instance_eval'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:113:in `block in run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:253:in `with_around_each_hooks'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:110:in `run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:380:in `block in run_examples'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:376:in `map'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:376:in `run_examples'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:361:in `run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `map'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `block in run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:34:in `report'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:25:in `run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:69:in `run'
         # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:8:in `block in autorun'
     # ./lib/rspec/expectations/fail_with.rb:33:in `fail_with'
     # ./lib/rspec/expectations/handler.rb:42:in `handle_matcher'
     # ./lib/rspec/expectations/expectation_target.rb:48:in `to_not'
     # ./spec/rspec/matchers/raise_error_spec.rb:56:in `block (2 levels) in <top (required)>'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:113:in `instance_eval'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:113:in `block in run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:253:in `with_around_each_hooks'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example.rb:110:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:380:in `block in run_examples'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:376:in `map'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:376:in `run_examples'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/example_group.rb:361:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `map'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:28:in `block in run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/reporter.rb:34:in `report'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/command_line.rb:25:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:69:in `run'
     # /Users/myron/code/rspec-dev/repos/rspec-core/lib/rspec/core/runner.rb:8:in `block in autorun'

I don't think this is a very promising route to take. Thoughts?

Contributor

patmaddox commented Sep 11, 2012

You want to go with #2 so that it is consistent with other expectations, formats nicely in the various reporters, etc

I think the formatting you've got there is fine. Perhaps some sort of dividing text between the error message and the failure message? I'm thinking of the spacing getting mangled on people's terminals and it being difficult to decipher exception message vs expectation failure.

Owner

myronmarston commented Sep 11, 2012

The formatting consistency is a good point.

Anyhow, one other option I just thought of: looking at the above, it's clear that the two backtraces are nearly identical except for a few frames. We could set the backtrace on the expectation failure error to the backtrace of the rescued error, so that the user gets the full backtrace as the "main" backtrace while keeping a consistently formatted failure message.

Edit: another benefit of this approach is that I don't have to go through gymnastics to filter the backtrace the same as core does.

Contributor

patmaddox commented Sep 12, 2012

So just have it be one long backtrace, originating from the rescued error?

Owner

myronmarston commented Sep 12, 2012

Yep. The message would be in the existing format, though.

Sent from my iPhone

On Sep 11, 2012, at 7:04 PM, Pat Maddox notifications@github.com wrote:

So just have it be one long backtrace, originating from the rescued error?


Reply to this email directly or view it on GitHub.

nyarly commented Oct 2, 2012

I've wanted this for so long, it's painful. I think the output in the example is fine.

One important variation, though is that the positive matcher should work the same way: expect{ raise "boom"}.to raise_error(ArgumentError) should print the backtrace of the RuntimeError that actually gets raised.

@myronmarston - are you onto a PR already?

Owner

myronmarston commented Oct 3, 2012

@myronmarston - are you onto a PR already?

Not yet. I've got the code lying around on my machine. There's some refactoring that needs to happen in rspec-core first to make it possible for this to use the same backtrace filtering logic core uses. Thanks for reminding me about this!

myronmarston added a commit to rspec/rspec-core that referenced this issue Oct 5, 2012

myronmarston added a commit to rspec/rspec-core that referenced this issue Oct 9, 2012

myronmarston added a commit that referenced this issue Oct 9, 2012

Include backtrace in errors reported by `raise_error` matcher.
It's hard to troubleshoot unexpected errors when the backtrace is silenced,
as it was previously.

Closes #59.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment