Consider deprecating expect { }.not_to raise_error(SpecificErrorClass)

[myronmarston]

An expression like expect { }.not_to raise_error is fine, as it expresses intent (particularly when paired with a corresponding example that demonstrates when a particular error is raised), but expect { }.not_to raise_error(SpecificErrorClass) is dangerous. It's prone to producing false positives. Consider:

expect {
  expect(some_helper(foo)).to eq(:bar)
}.not_to raise_error(SomeError)

With this in place, it's impossible for the inner expectation to ever cause the example to fail. When the inner expectation fails, it raises a RSpec::Expectations::ExpectationNotMetError, but this is not a SomeError, so the outer expectation causes it to pass.

Thus, I think we should deprecate a specific error class with the negative raise_error matching. In 3.0 it'd be nice to disallow it entirely.

[JonRowe]

Alternatively we could make it fail on a RSpec::Expectations::ExceptationNotMetError, or on any Error.

[fables-tales]

I'm in favour of removal of this: people either expect some specific error or no error whatsoever. It seems backwards to me to have a spec that passes when the vast majority of errors are thrown, apart from the one we're expecting not to happen.

[myronmarston]

Alternatively we could make it fail on a RSpec::Expectations::ExceptationNotMetError

This would improve things, but it doesn't solve the problem. Consider if the spec is like this:

expect {
  some_object.do_something
}.not_to raise_error(SomeError)

Later, we rename the do_something method to do_something_else, and run our specs to find all the places we need to update the caller. Problem is, the inner expression now raises a NoMethodError, and the expectation allows that to pass because NoMehtodError is not a SomeError...so this spec is now testing absolutely nothing.

or on any Error.

Sure, we could do that, but that's not what the expectation expresses, is it? I think it's confusing to do something entirely different from what the expectation expresses. I don't think people actually ever really want this -- they're just thinking "this used to raise exception X", but I'm fixing it so it doesn't do that anymore -- and they use this expectation without thinking of the consequences. (See rspec/rspec-core#846 for an example -- it's what prompted me to open this issue).

[cupakromer]

After considering this and looking at the code bases I'm working on. The places were not_to raise_error are used, aren't necessary and should be re-written to be more explicit.

👍

[JonRowe]

I'd be happy for the behaviour to be deprecated and removed to be honest, I was just providing some alternative thought. If it's kept I think we should change the behaviour to catch only the specific named error, or all errors if none are named. That way it would be more explicit in it's intent and should create less false positives.

[hron84]

As previous I still keep my opinion: if code raises error other than we expected it is a problem of the example or problem of RSpec?

We really want to fix developer errors too?

with @myronmarston example:

expect {
  some_object.do_something
}.not_to raise_error(SomeError)

Developer wants to express the wrapped code should not raise an error what specified. The naive developer assumes if error is raised what is not a SomeError, e.g. NameError the expectation not activated, but the error itself is raised ahead. It means the example is fails with E(xception) and not F(ailed) (and of course, definitelly not pass). Another situation - what would be covered with another exampel, or code needs fix - but it's a developer's job, not of the test framework.

If we fail if any error happened it looks like a good thing - but if developer wants to fail on any error, then he not specify an error class in the method - the current codebase provides a way for this. If developer specify an error class then he wants to example pass when - and only when - the block is not raises that specific error. For example. if I want to check a templating system to finds templates correctly, I can expect to not raises a TemplateNotFoundError. But it should raise another error, because - for example - the codebase is not completed - but template loaded correctly.

[myronmarston]

if code raises error other than we expected it is a problem of the example or problem of RSpec?

It could be either: RSpec, like all software, has bugs, and it could be a bug in RSpec causing an error to be raised. Or it could be a problem with the example itself (or the code the example tests). Regardless, I don't think to silence it.

The naive developer assumes if error is raised what is not a SomeError, e.g. NameError the expectation not activated, but the error itself is raised ahead.

I'm not 100% sure what you mean by this, but it sounds like you're saying a naive developer assumes this expectation concerns itself only with the named exception, and would allow other types of exceptions to propagate?

I don't really know what a naive developer expects. Regardless, I don't think the behavior of this expectation is ever something you'd want. (I think folks who do want it don't quite understand what it is doing yet).

Another situation - what would be covered with another exampel, or code needs fix - but it's a developer's job, not of the test framework.

Of course it's the developer's job to fix problems in their code. But if the test framework is hiding errors from them (as this expectation does), they won't know they have a problem that needs fixing. This is a bad thing.

[hron84]

I'm not 100% sure what you mean by this, but it sounds like you're saying a naive developer assumes this
expectation concerns itself only with the named exception, and would allow other types of exceptions to
propagate?

Exactly, sorry for my poor English.

There are many libs what communicates the problems with the developer via exceptions/errors. Especially in Ruby, devs usually do not have a C-like approach returning with nil, false, or so if error happened, instead raises an error.

What I want to say to code not raises a specific error? I allow the code to raise any other error because I know the code is not complete yet (e.g. because I follow TDD pattern), and code may raise NoMethodError, SyntaxError, and so on, it is a natural thing. But, what I want to express: this code is not raises SomeError, what is means I didn't made a specific type of mistake. But it is not means I fixed all errors in the code, it means the code should not ran into a well-defined problem (what causes SomeError being raised).

Back to ERB-trimming topic: ERB parser is raises SyntaxError if ERB.new called with wrong arguments. Even if it is a sign of poor architecture of ERB parser (it should check the template syntax an raise a TemplateParsingError instead), as a client of it I must accept this behavior. However, this means no way to ensure the trimming option is passed to ERB templating than checking the ERB parser code is not raises SyntaxError if I pass a template with trimming 'dash' (and without a syntax error, of course). However, the template parser code should raise any other error, e.g. NameError if I follow TDD pattern and I call another function what's not defined/implemented yet. Or the tester block can raise ExpectationNotMetError, because it is definitely not a SyntaxError.

[cupakromer]

@hron84 I see where you're coming from. I had that very same thought process previously. However, the test quickly becomes brittle.

Sure, you know now that you aren't creating a SyntaxError. You also don't know what other errors you are creating. Testing for the lack of the SyntaxError doesn't actually test that you did what you want; just that you didn't cause a syntax error. You want to be testing for the expected side-effect or outcome. If you get a SyntaxError you know you did something wrong. When that error goes away, you did something correct and your test, should be for what you did (outcome / side-effect).

Regarding TDD. I don't see this as an issue. Depending on how tight and small your TDD loop is, the argument can be made that you would only get the missing method exception as you implement the next feature. By your argument, you should have a test that checks that no exception is raised after you implement that method. However, that would mean you should then start adding test to make sure each possible error (that you know of) is not raised as you are testing. That's not going to add value to the test suite in the long run.

By not testing for an exception and testing the side-effect / outcome you have a solid test that demonstrates the behavior of the code. It will at the same time implicitly test for things like SyntaxError, NameError, etc.

Consider the scenario where in the future you or a colleague replaces ERB with something else (possibly a contrived scenario, but it illustrates the point of brittleness). The exception test will pass falsely. It is no longer testing it's original intent. It does not fail because no syntax errors were made, but the code that could cause a syntax error is no longer there. By instead just testing the outcomes, when you change the behavior of the method, that will fail and you/your colleague can then delete the out of date test.

[hron84]

@cupakromer

Problem: in ERB, no side-effect / outcome, if no trimming argument specified. The SyntaxError is the only thing you can test! So template.result(binding) raises an error and does not returns with anything. And no syntax checking method in ERB templating engine, because it is not a high-level templating language. ERB is basically translates template tags to some wrapper ruby code and evaluates it with eval, that's all. It is not like a HAML or SLiM what builds a syntax tree and checks it before compile/run the template code itself. ERB has very simple and very dumb implementation to use it anywhere without spending a performance for it.

Usually, I accept to not test errors, because if they appears that means something definitely wrong happened. If an error is raised, there is a mistake in the code or in the logic. (SyntaxError is even worse, because any Ruby code can raise it if there is a - syntax error. This is why this example was problematic.)
However, there is some external library with bad behaviors what I - as an application developer - cannot and should not change. My example is tried to show there is some situation where I should need to exclude some error from checking.

To be clear: I usually do not do things like checking to not raise SyntaxError. It is a crazy and useless idea, especially because - as an experienced developer - I know humans make syntax error too easily. I taken this example because I talked about it in other RSpec issue previously and I kept it in my mind.
Furthermore, I usually do use the complete expect { }.to_not raise_error very-very rarely as usually I want to all errors be raised from tests because it helps me to fix all errors in my code.

[fables-tales]

Closed by #244

[mckinnsb]

@hron84 @myronmarston I found this thread a bit late but I've more or less come here to chime in on @hron84 's position. It seems like this decision is "made", but it's really kind of a shame. While @cupakromer 's arguments are valid re: intent of behavioral testing, there are actually specific cases where it does make sense.

In my case, for instance, I have an application that raises a CanCan::AccessDenied error. When I'm writing controller specs to see if particular roles can access particular resources at the controller level ( not driving through views here, trying to speed up the spec ), I want to detect if CanCan does not raise a particular type of error, because all I am testing in those specs is "affirming" what each role can do, and that the controllers have authorization enabled. The absence of that specific error confirms this. I'm not really concerned with errors of any other nature. If there's a view error, I have integration tests that will catch that.

Add to the fact that my controller catches this error and responds to it differently based on the role ( admins for instance ), and you could see how looking for "not_to raise_error CanCan::AccessDenied" is simpler than checking for the "aftereffects" of the error handlers.

[myronmarston]

@mckinnsb -- I'm still convinced that your use is a bad idea. Consider that a rename-method refactoring somewhere in the call stack can create a NoMethodError that turns the spec into a false positive that is actively silencing problems. It's unclear to me why expect { }.not_to raise_error won't work for your use case. If CanCan::AccessDenied is raised, the spec will fail as you expect, and it's not prone to false positives.

Still, if you want to use an unsafe expectation, that's your prerogative. You can write a custom matcher for this:

RSpec::Matchers.define :deny_access do
  match do |block|
    begin
      block.call
    rescue CanCan::AccessDenied
      true
    rescue
      false
    end
  end
end

# use it like so:
expect {
  do_the_thing
}.to deny_access

[mckinnsb]

@myronmarston , thanks for the suggestion, but I'm not sure you understand that in this case, I am testing very specifically for the presence or absence of this error in order to assert that a library is functioning properly at a certain level - a use case which @hron84 alluded to earlier. There's nothing "unsafe" about that. This controller test is testing exactly what it should be. It's making sure that messages are passed through the code base - or are not passed through the code base - given a set of conditions.

So, I don't see that as a false positive if the test passes in the rename method scenario you outlined. There would certainly be errors somewhere else in the tests.

As to why I do not check for all errors, it's actually because of the situation you have just outlined. I'd prefer in general if the number of failures that I see is more or less proportional to the actual # of discrete "problems" in the codebase. The way I have things now, if someone were to rename a method in a model or controller, it would very quickly break those model/controller tests. Not this "cancan" controller test, which is something very specific, but still very useful.

If I were to do what you suggested, an error in a model/controller could break a hundred tests or more. While there are obviously ways to recognize patterns in your error reports or use tools that condense them, minimizing noise is a priority to me when you are looking at a suite nearing 1000+ tests.

[myronmarston]

There's nothing "unsafe" about that.

This expression is fundamentally unsafe:

expect {
  some_block_of_code
}.not_to raise_error(something_specific)

Because, to test the block of code in expect properly, it requires that you never, ever make a change that would cause some other unexpected error to be raised...if another unexpected error is raised (such as a NoMethodError), the example will pass, even though it may not even be executing the code you are intending it to test!

Consider if your code begins life like this:

def do_something(arg1)
end

With a spec like so:

expect {
  do_something(:foo)
}.not_to raise_error(SomeSpecificError)

Later on, you need to change do_something to accept a second argument:

def do_something(arg1, arg2)
end

You change the couple of call sites you think of (but forget to change it in the spec above).
You run your specs, and the above spec passes, because ruby is raising an ArgumentError, which is not SomeSpecificError -- so the block passed to expect is not raising SomeSpecificError...but your test is no longer even executing do_something, which is the entire point of the test. This test is no a false positive that is giving you a false sense of confidence.

It's a subtle point, but it's not something I want to encourage RSpec users to use. If you think you're good enough never to make a simple mistake that causes a new unexpected error to be raised, then feel free to use a custom matcher as I outlined above.

[mckinnsb]

Because, to test the block of code in expect properly, it requires that you never, ever make a change that would cause some other unexpected error to be raised.

And, as I said, this test checks:

for the presence or absence of this error in order to assert that a library is functioning properly at a certain level - a use case which @hron84 alluded to earlier. There's nothing "unsafe" about that

Emphasis on library. I understand that in nearly every other case this block of code is fundamentally unsafe. To me, the point is not subtle. I get it, and your trying to encourage people to write good tests.

But - is it unreasonable to write tests that make sure that a library/gem is correctly integrated? Is there another way to encourage people without removing this functionality? I mean, I'm happy to use a custom matcher, but it's not because I think I'm a human being completely incapable of making an error.

[myronmarston]

Emphasis on library. I understand that in nearly every other case this block of code is fundamentally unsafe. To me, the point is not subtle. I get it, and your trying to encourage people to write good tests.

Maybe I'm totally misunderstanding something, but I have no idea what testing against a library vs another piece of code has to do with this issue. The issue remains that not_to raise_error(SomethingSpecific) is prone to silencing legitimate errors and giving you false confidence, and a given block of code will either raise no errors (in which case I recommend using expect { }.not_to raise_error) or will raise some other error (in which case I recommend using expect { }.to raise_error(SomeOtherError).

But - is it unreasonable to write tests that make sure that a library/gem is correctly integrated?

That's not unreasonable, but again, I have absolutely no idea what that has to do with this issue.

[mckinnsb]

Maybe I'm totally misunderstanding something, but I have no idea what testing against a library vs another piece of code has to do with this issue.

We are assuming, in this case, that the library code is not changing. We are testing the integration with the library, not our application directly.

[myronmarston]

We are assuming, in this case, that the library code is not changing. We are testing the integration with the library, not our application directly.

So you never plan to upgrade the library?

[mckinnsb]

As long as it suits my needs, no I don't.

The library is also very well covered by its own tests ( or else I would not be using it ). Please keep in mind that these tests would mostly cover the unsafe cases "hidden" by the block.

Again, this is just testing integration between the two codebases, not the behavior of either.

[JonRowe]

Please bare in mind that even if you don't upgrade the library, it might have a dependency which isn't locked, could be upgraded and break the underlying integration without you noticing, after all YourSpecifcError is never raised.

As @myronmarston says if you really want to go against our advice it's fairly easy to implement this yourself.

[xaviershay]

just going to bikeshed a bit here on a tangential issue: in my experience testing access control like this is basically like testing configuration: lots of duplication and doesn't catch errors.

Prevent access to everything by default, then any normal tests will fail unless you've explicitly enabled them.

if you really want to go against our advice it's fairly easy to implement this yourself.

this

[jk779]

Sorry to pop this old thing up, but I'm currently in the situation to fix a bug, hence I'm writing a test to provoke it. I think thats a common thing.

But the information that my fix is working is not the only reason I'm writing the test. I want to document with it, what happend before, and because of that I feel the need to specify very exactly what has been raised.

Here is what I'm doing:

expect { handler.each { true } }.not_to raise_error(ArgumentError, /user malicious_filename.jpg/)
expect { handler.each { true } }.not_to raise_error

The actual Exception is: #<ArgumentError: user malicious_filename.jpg20140226-24903-1c07oqn doesn't exist>

The second expect would cover a regression (in this case, when the Ruby Dir-Class gets changed).
The first one got marked as deprecated by you.

Am I missing something?

[ideasasylum]

(sorry for the side-topic)

Github says that I "referenced this issue from a commit 8 days ago" but that commit has been removed. I did not.

Has anyone seen this before? A glitch in the matrix or has my github account been hacked?

[myronmarston]

@ideasasylum -- no idea. You can ask github support about that, maybe.

@jk779 -- if you used (and we allowed) this:

expect { handler.each { true } }.not_to raise_error(ArgumentError, /user malicious_filename.jpg/)

...then there are easy ways the bug could regress w/o causing this to fail. For example, imagine that someone decided to change user to User in the failure message. Your regex does not have i on the end, so it is case case sensitive...so if an ArgumentError with the message "User malicious_filename.jpg20140226-24903-1c07oqn doesn't exist" was raised, your expectation would pass, even though this is the exact failure you are trying to prevent.

If you want to document what the error was, I recommend using the docstring of your example for that.

[jk779]

@myronmarston Thanks for your reply. I agree that this line can turn very easy false positive and I guess I'll take your advice and instead use the description of the example for my "documentation purposes". Thank you! 👍

[rwilcox]

Hate to drag this up again, but I'm hoping to help people who landed on this page from a search engine. Hello people of the future!

I wrote a custom macher to replace expect {}.not_to raise_error(SpecificException). I term this matcher expect {}.to never_raise(SpecificException). I believe this implementation solves problems exposed by some of the above commenters (mainly making sure to not swallow expectation failure exceptions nor swallow other Ruby exceptions). But if SpecificException is raised by the block we have a pretty message.

# This matcher will return:
#    1. TRUE if the code was run without exceptions
#    2. FALSE if the code was run but raised (only) the specified exception
#
# It *will* raise an exception if the block of code raises an exception other than
# (the exception specified)
#
# To use it
#
# expect {
#  code
# }.to never_raise(MySpecificException)
#
RSpec::Matchers.define :never_raise do |exception_class|
  global_result = nil

  match do |block|
    begin
      block.call
    rescue exception_class
      global_result = "expected #{block.source_location[0]}:#{block.source_location[1]} to never raise #{exception_class.name}, but did"
      false  # we did NOT never raise this exception

    rescue RSpec::Expectations::ExpectationNotMetError => exception
      global_result = "expectation failed inside block at #{block.source_location[0]}:#{block.source_location[1]}: #{exception}"
      # give us a pretty error message in addition to the error message from the exception
      raise exception

    rescue
      # handle other exceptions by reraising them. They are exceptional!!!
      # (also, no pretty error messages here)
      raise

    else
      true   # everything ran, nothing raised at all, thus code did in fact not raise anything
    end
  end

  failure_message_for_should do |player|
    global_result
  end
end

[thethanghn]

Hello, please allow me to dug it up once again. But this deprecation is really annoying.

It is true that we should not catch a general error, we should always go specific. But in case of writing tests, it may not be true.

I am writing test spec for access rights into my controller, I used action filters for this purpose:

• If the user is allowed to access, go into the controller's action code
• If the user is not allowed to access, he will get a RoutingError instead

So in case of expecting it '}.to raise_error(RoutingError)' it is clear, I get what I am expecting to. But in the other case, I wrote it as '}.not_to raise_error(RoutingError)' I got a lot of deprecation message. Then if I leave only not_to raise_error without specific error, it will attempt to go through the controller's action to detect for any other error, whether params not found, or query failure, etc.

It is realy annoying because for testing the correctness of controller's action, I have other tests for them already. So why I can't get the opposite of not_to be something here? Why it must be any error?

It is similar to the situation when you test the output equals to a number, or not that number, or any other number? If I want to test it 'not that number' I must be able to do it, you can't say that you prefer it is to be 'any number' for a broader use cases or coding practice. This is such a very ambiguous practice to be applied to everyone.

Please remove that annoying deprecation message.

[myronmarston]

For anyone who still wants a replacement, there's a pretty simple way in RSpec 3.1+ (3.1 is the first release to have define_negated_matcher) to get it back, with an expectation that is much more explicit about what it's doing (and thus, IMO, better than what we had before):

RSpec::Matchers.define_negated_matcher :an_error_other_than, :an_instance_of

RSpec.describe "Using `an_error_other_than` as an argument to `raise_error`" do
  example "passing" do
    expect {
      raise ArgumentError
    }.to raise_error(an_error_other_than(SyntaxError))
  end

  example "failing" do
    expect {
      raise SyntaxError
    }.to raise_error(an_error_other_than(SyntaxError))
  end
end

Running this produces:

Using `an_error_other_than` as an argument to `raise_error`
  failing (FAILED - 1)
  passing

Failures:

  1) Using `an_error_other_than` as an argument to `raise_error` failing
     Failure/Error: expect {
       expected an error other than SyntaxError, got #<SyntaxError: SyntaxError> with backtrace:
         # ./spec/raise_error_spec.rb:12:in `block (3 levels) in <top (required)>'
         # ./spec/raise_error_spec.rb:11:in `block (2 levels) in <top (required)>'
     # ./spec/raise_error_spec.rb:11:in `block (2 levels) in <top (required)>'

Finished in 0.00248 seconds (files took 0.11564 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/raise_error_spec.rb:10 # Using `an_error_other_than` as an argument to `raise_error` failing

Note that this isn't exactly the same: expect { }.not_to raise_error(SyntaxError) would pass if no error was raised or if any other type of error was raised. However, these two cases are entirely different, and IMO, if you want the former case, then expect { }.not_to raise_error is much better, and for the latter case, this is much more explicit about what it is actually doing.