Warn users about stupid behavior like returning from example.

[arturaz]

Currently if you return from example, RSpec happily carries away not
executing whole bunch of your examples without any warning. This is VERY
dangerous, because it gives false sense that user code is being tested,
while it is really not.

These warnings should at least give user notification that something is
wrong.

[alindeman]

My gut is that this feels a bit unnecessary. If you were writing tests first and watching them fail for the right reasons (or even if you write tests afterward, forcing them to fail), is this needed?

It's definitely a mistake, but I wonder the value of maintaining code in RSpec to warn about it.

[arturaz]

I wrote that test 6 months ago. It worked perfectly well, no errors. Running rspec "spec_file.rb" gave no errors because lots of examples were not executed (try tracking if there's 150 or 170 dots). How should I have known that rspec behaves like that?

And because of this numerous releases with broken code have been deployed into production, just because rspec spec/ reported no errors. How should I catch that?

Last of all - I spend half of a day tracking this down.

Do you still feel like this is irrelevant?

[arturaz]

Sorry for the tone, but maintaining 5 lines of code (not counting in warning text) isn't too hard and could save hundreds of hours of developer pain.

[dchelimsky]

@arturaz are you saying that you wrote a return statement in an example, or that rspec put one somewhere that messed you up?

[alindeman]

And because of this numerous releases with broken code have been deployed into production, just because rspec spec/ reported no errors. How should I catch that?

That totally sucks, I agree.

But I'm thinking that it's because the tests are not written very well. I personally would fully expect that return would stop a test in its tracks like it does any other method: I'm not sure why we need a warning to tell people about a basic programming construct.

I don't like the idea of adding a warning simply because someone used a construct (return) that did exactly what it was supposed to, even if that turned out to be a bad idea.

If the tests were written in such a way that the original developer watched the test fail first, this wouldn't have been an issue, I don't think.

Last of all - I spend half of a day tracking this down.

Again, I hear you that it sucks :(

Do you still feel like this is irrelevant?

I think so, but I'll wait to hear from other folks or team members.

[fj]

There are all kinds of silly errors one can make, e.g. writing foo == :bar instead of foo.should == :bar. Almost all of these are caught by making the test fail first and pass afterwards.

[arturaz]

@dchelimsky I did put return into it block.

@alindeman I feel that code should try to handle as much human stupidity as possible.

Lets talk about dreaded NullPointerException in Java or NoMethodError on nil in Ruby. Does it mean that developer was stupid and messed up? Indeed it does. Does it mean measures shouldn't be taken against it? Of course not. Scala Option monad, Java @NotNull annotation, Kotlin null checking. All of those are there to help people, who make mistakes no matter how skilled they are.

Does writing return in it block is a developer error? Sure. Is it basic programming construct. Sure.

But if RSpec could possibly prevent somebody wiping their database clean, because some test that handled that delicate situation with broken code didn't run and rspec did not report it - I'd say that's a good thing.

One way or another - this could be argued as a bug. RSpec has to run 200 examples, but it only runs 100 without any interaction. Seems like a bug to me. Even if it is user induced.

[arturaz]

@fj Please explain exactly how you would catch this kind of error with "fail first, pass afterwards".

About stupid errors:

IDEs issue warning in Java if you try to compare two strings with == (obj equality) instead of .equals (content equality)

This isn't a huge change. It is not a bunch of smart code. Saving people from frustration by merging 5 lines = great success.

And if it was possible to catch and warn about other stupid mistakes - my take would be to do that too.

[fj]

Please explain exactly how you would catch this kind of error with "fail first, pass afterwards".

@arturaz Not sure how much more I can distill it than that. Just write the test first before you write any code and then run it, to be sure that it failed. If you accidentally type "return" at some point, your test won't pass; it won't even run to begin with.

I think there could be some merit to something like a style-checker for RSpec (warn if your test contains no assertions, warn if you do foo == :bar instead of foo.should == :bar, that sort of thing). I don't think that should be part of RSpec core though.

[dchelimsky]

@arturaz there are hundreds of mistakes you might make like this. RSpec does try from time to time to help when you make a mistake using RSpec's APIs, but it can't be in the business of protecting you from basic Ruby mistakes like this.

[arturaz]

@dchelimsky Seriously? RSpec ignores a bunch of the examples and that's perfectly fine? :) Not even a warning? Not even "100 examples not run" at the end of the specs? Wow.

[alindeman]

RSpec isn't ignoring examples. The test explicitly tells Ruby to bail out of the example via return.

[myronmarston]

FWIW, I tried to repro the error, and from what I can see, you get a LocalJumpError when you use return in an example, as I expected. No examples are skipped.

https://gist.github.com/2875353

@arturaz I'm 100% on board with @dchelimsky on this.

[alindeman]

@myronmarston, I think it's a 1.8 <=> 1.9 difference.

1.8.7 :001 > proc { return }.call
 => nil 

1.9.3p194 :003 > proc { return }.call
LocalJumpError: unexpected return

[arturaz]

Ok, it seems that I need to take this to JRuby guys then. Sorry for the fuss.

[travisbot]

This pull request passes (merged 4c52ff1 into 4feeec6).