actual.should \n matcher always passes

[dchelimsky]

5.should
  eq(5)

5.should
  eq(3)

Both of these pass because should allows 0 args in order to support operator matchers (actual.should == expected).

To ensure this doesn't happen, we probably need to disallow the forms listed above by failing examples in which should receives no args but also never receives an operator matcher message. Unfortunately, the only way I can think of to do this is for operator matcher handlers to register end-of-example callbacks that verify they've received one of the operator matchers.

This also suggests we should not support operator matchers with the expect(actual).to matcher syntax proposed in #119.

[patmaddox]

An end-of-example callback means that the example will continue to run, and potentially fail elsewhere. I expect that we'd see the original error message ("expected 5, got 3") which means we'd have to trap any errors that might come after. I see this as potentially confusing for someone who goes through the logs and sees logged actions occurring after the example supposedly failed.

If the caller takes no arguments maybe we should look at the line and see if there's an operator match coming after. I wonder how much that would slow things down.

Regardless of how this is implemented, I think it should raise an SyntaxError. You don't solve the problem of someone looking at logged events that never should have happened in the first place, but hopefully you tip them off because a SyntaxError means that none of the code is valid. Basically seeing a syntax error should indicate that the code's logic is irrelevant because the structure is illegal and needs to be fixed.

[dchelimsky]

@patmaddox the trick is we don't know there's even a syntax error until the end of the example and, as you point out, we'd now have to capture and ignore any other failures or exceptions. I think I'm about ready to deprecate operator matchers.

[patmaddox]

We can look at the calling line at runtime and parse that line. It might be unacceptably slow, not sure, might be worth experimenting with though.

(or maybe there's a pre-processor enabled/disabled with a flag?)

On another note, might we consider addressing cases like this with "don't do that"? I'm torn because I think the test runner should be air-tight, but at the same time I've always found the operator matchers to be immensely expressive. What are we going to do with 3.should > 2 and [1,2,3].should =~ [3,1,2] ? Replace them with be_greater_than and be_remember_how_much_trouble_we_had_naming_this_thing :) ?

[myronmarston]

I'm on the fence about this, too. When you put a line break between a paren-less method call and the argument, ruby doesn't pass that on as an arg to the method. That's just how ruby works. There are other cases where ruby interprets the syntax one way even though the developer intended it to be interpreted a different way. I don't think we can (or should) put special handling in RSpec to handle all these cases. Down that way lies madness. Sure, if there are common mistakes that people make, and there's a relatively pain-free way to add an error or warning to RSpec to nudge people in the right direction, great...I'm all for that. But is this a common mistake people make? The suggestion of an end-of-example callback isn't too painful but it's not super simple, either. It would be yet another point of coupling between rspec-core and rspec-expectations.

[justinko]

I think I'm about ready to deprecate operator matchers.

+1

Replace them with be_greater_than and be_remember_how_much_trouble_we_had_naming_this_thing

Just like eq, we can have gt, gte, lt, lte, etc.

[dchelimsky]

Those don't read very well with actual.should gte(min) or expect(actual).to gte(min). The would read well with expect actual, eq(min), but that's not where we're headed for rspec-expectations :)

The obvious next thing to try is expect(actual).to be_gt(min), but I don't really like that. WDYT?

[justinko]

Now that the new expect syntax is merged, it still doesn't solve this problem.

I think we should deprecate operator matchers in 3.0 in favor of be.

[dchelimsky]

We have to actually remove them to solve this problem, and I think doing that would be a deal breaker for a lot of rspec users. How about we make it configurable with the default on in rspec-2 and then off in rspec-3?

[myronmarston]

Now that the new expect syntax is merged, it still doesn't solve this problem.

It solves the problem if you use the expect syntax. It does not support operator matchers and will raise an error if you do something like:

expect(5)
  to eq(5)

It doesn't solve the problem for the old should syntax, though.

We have to actually remove them to solve this problem, and I think doing that would be a deal breaker for a lot of rspec users. How about we make it configurable with the default on in rspec-2 and then off in rspec-3?

I'd favor that we consider the expect syntax to be the solution to this issue. It has multiple benefits, one of which is that this isn't a problem with it. Users can configure syntax = :expect to enforce the usage of that syntax on the project and avoid this problem entirely.

[dchelimsky]

Agree with @myronmarston. Closing.

[justinko]

What I meant was that the new syntax has nothing to do with this should problem. Anyway, I don't really care about this problem - just trying to close the issue.

If a user is putting an argument on a new line with no parentheses, God help them.

[gabetax]

After encountering this with expect.to, I had to poke around in pry a bit to wrap my head around this problem. Can anyone verify my writeup at https://gist.github.com/gabetax/6160319 for accuracy?

[JonRowe]

Yep, that writeup looks accurate to me.

[myronmarston]

@gabetax: yep, @JonRowe said that's all accurate. There are a few additional reasons, though:

• Ruby issues a warning: possibly useless use of == in void context for an expression like x.should == y when it's not the last expression in an example. (== is generally intended to be a query method with no side effects, so ruby is noticing that you're not using the return value of == and warns).
• There was a gotcha in that x.should != y did not work. It's essentially impossible to make it work on 1.8, because 1.8 treats that as sugar for !(x.should == y) -- so an expression like 5.should != 5 would pass.
• As you noticed (but didn't state outright), the operator matchers required special handling whereby the object returned by should had special definitions of the various operators, rather than them actually being a normal matcher passed to should. It's nice to move in the direction of simplicity and consistency and make all matchers work identically.