Add support for `expect(value)` syntax. #119

myronmarston · 2012-03-04T07:32:55Z

Note: I'm opening this pull request just to start a discussion about this. It's not ready to be merged yet.

Feature Summary

This adds an alternate syntax to the existing should syntax for setting expectations. It's based on the already existing use of expect for block expectations but makes it work for normal ones, too:

expect(something).to be_awesome
# rather than:
something.should be_awesome

I've already had a few conversions with @dchelimsky and @justinko about this. There are some details we need to work out and I also want to get feedback from a wider base of RSpec users.

Here's a summary of why I think this new syntax has value:

Currently, there are two syntaxes for setting expectations: should for normal expectations, and expect for block expectations (or you can fall back to using a lambda/proc). This unifies them: you can use expect for both.
This also aligns the syntax with Jasmine, which can be nice for people working on projects that use both.
I believe that all matchers will work just fine with this new syntax.
This opens up the possibility of not monkey patching every object in the system with should and should_not.
should and should_not are prone to problems related to the fact that any object can undefine or redefine them on itself and it suddenly can break rspec-expectations. We've actually seen a few cases of this (where it was completely unintentional, in fact!). Consider the case of a proxy object that uses BasicObject, defines a method (proxy_method) and proxies the rest to a target object using method_missing. An expectation like proxy.should respond_to(:proxy_method) can wrongly fail, because should will be proxied through to the target object, so this winds up being target.should respond_to(:proxy_method). In contrast, expect(proxy).to respond_to(:proxy_method) would work just fine. For some more cases of problems like these, see Weird expectation failures for delegated object #114, be_* predicate gives wrong results when requiring delegate before rspec rspec-core#471 and #should method on CollectionProxy rspec-rails#445.

The last one is the biggie for me. I've been bitten by weird, hard-to-track-down bugs with should on delegate objects.

Open Questions

So, open questions for discussion:

Is anyone against adding this as an alternate syntax to rspec-expecations?
Does it makes sense to provide a way for someone to use rspec-expectations w/o should and should_not being monkey-patched onto Kernel? The value I see here is that if people decide that expect is the preferred syntax for a given project, it would be nice to be able to help enforce uniformity by preventing should and should_not from being used (which, in turn, would ensure the project never has any of the weird proxy-object should issues we have seen). Note that I wouldn't consider every removing should and should_not from rspec-expectations entirely. There's too much code out in the wild that uses and it generally works fine.
If we do want to provide a way to disable should/should_not, what should that mechanism be?
If we decide to provide the means, and decide that expect is the preferred syntax, would it make sense to disable should and should_not by default in some future major release (i.e. make it opt-in for that syntax, rather than opt-out).

My Two Cents

I like this syntax a lot (obviously; that's why I opened this PR!), and I'd like a way to be able to disable should and should_not on future projects. I'm not yet sure what that way should be, but one possibility is the existing expect_with option in rspec-core. Maybe it could support expect_with :rspec for all of rspec-expectations, and expect_with :rspec_only_expect (or something better named) for rspec-expectations w/o should. As for #4: if rspec users like the expect syntax, and we can ensure a smooth transition, and provide could deprecation messages, I'd probably be in favor of disabling should by default in 3.0 or 4.0, simply because it avoids a whole class of issues.

If you're an RSpec user and you have any opinion on this whatsoever, please leave a comment!

justinko · 2012-03-04T08:12:16Z

I'd love to see this in 2.9, and make should opt-in in 3.0.

Regarding excluding should, I would rather see it more explicit than another expect_with option. Something like RSpec::Expectations.include_should = false.

Good stuff!

dchelimsky · 2012-03-04T08:14:56Z

Not yet :)
Yes
config.enable_should_and_should_not = false
Yes

dchelimsky · 2012-03-04T08:19:03Z

@justinko 2.9 is imminent and this is going to take some time, so probably 2.10 or 3.0 (if there isn't a 2.10). If it's 3.0, I think we're safer leaving it an opt-in and make it opt-out in 4.0).

dchelimsky · 2012-03-04T08:24:50Z

@myronmarston what about the it { should matcher } syntax?

dchelimsky · 2012-03-04T08:28:38Z

spec/rspec/expectations/expectation_target_spec.rb

+    describe ExpecationTarget do
+      context 'when constructed via #expect' do
+        it 'constructs a new instance targetting the given argument' do
+          expect(7).target.should eq(7)


This is funny - we're using the old syntax to specify the new. Reminds me of rspec's early days when rspec was tested w/ test/unit.

Do you think expect(expect(7).target).to eq(7) would be too much of a leap at this point?

For all these examples, I started with just the new syntax and empty to and not_to methods, but everything (wrongly) passed since to and not_to were no-ops. Using should here got me correctly passing and failing examples, and gave me confidence I was actually specifying something :).

We can certainly refactor in the future.

justinko · 2012-03-04T08:33:28Z

so probably 2.10 or 3.0

@dchelimsky for some reason my brain couldn't process the fact that we can increment from 2.9 to 2.10 rather than 2.9 to 3.0 😪

config.enable_should_and_should_not = false

IMO, It feels strange to me to have a config option for a library that is opt-in. Oh well.

justinko · 2012-03-04T08:39:17Z

IMO, It feels strange to me to have a config option for a library that is opt-in. Oh well.

@dchelimsky maybe config.expectation_framework.enable_should_and_should_not = false

That way we don't bind RSpec Core to opt-in libraries.

UPDATE: Or rather I should say, let opt-in libs leak into RSpec Core. Right now it's all contained in config.expect_with.

dchelimsky · 2012-03-04T08:49:01Z

@justinko it would be defined in rspec-expectations via config.add_setting, so no binding.

What you propose, however, might be a cleaner way to handle config settings for extensions. It would certainly clarify where the options live, and it might allow us to clean up Configuration a bit.

justinko · 2012-03-04T08:54:01Z

it would be defined in rspec-expectations via config.add_setting, so no binding.

@dchelimsky RSpec Expectations can be run stand-alone (e.g. w/ T/U), so that would be binding it to RSpec Core 😄

justinko · 2012-03-04T08:56:15Z

@dchelimsky we do a if RSpec.respond_to?(:configure) in expectations, so nevermind.

tpope · 2012-03-04T08:58:41Z

Ever since we started turning our nose up at the noise word "should" in example descriptions, it's kind of irked me we still use it in examples themselves. So, from that perspective alone, I like this.

dchelimsky · 2012-03-04T09:03:09Z

RSpec.configuration.add_setting(:enable_should_and_should_not) if defined?(RSpec.configuration)

dchelimsky · 2012-03-04T09:05:11Z

@justinko That said, I really like the idea of a name for the extension.

justinko · 2012-03-04T09:27:34Z

@dchelimsky and in the case of multiple expectation/mock frameworks:

config.expectation_framework(:rspec).enable_should_and_should_not = true
config.expectation_framework(:test_unit)
config.mock_framework(:rr)

Just setup a mapping from symbol to module/constant. Obviously, not passing a symbol would return the first framework.

Sorry to take this off track folks!

justinko · 2012-03-04T09:33:15Z

@tpope EXACTLY.

Which brings up an interesting point: maybe should_receive could be receives...

dchelimsky · 2012-03-04T14:17:44Z

@justinko expects (like mocha).

myronmarston · 2012-03-04T16:39:54Z

@myronmarston what about the it { should matcher } syntax?

Great question. I didn't even consider that. I don't use it very often. You could still use specify { expect(whatever).to matcher } for one liners (although, it's not the same, with the implicit subject and what not). If anyone has a better idea we can consider it, but I'm ok with the disabling of should and should_not being a trade off that means you no longer have that syntax available.

This actually brings up a good point in favor of the config option being exposed by rspec-core (but added by expectations, as you've suggested): it means that rspec-core will have access to it and can thereby choose not add these methods.

One reason I opened this PR now was to hopefully get this new syntax in a 2.x release (potentially with should/should_not being opt-out) so that we have the possibility of making it opt-in for 3.0. That said, if 3.0's just around the corner that might be moving too quickly. If we do want to do that, or something like it, I had an idea for a way to ease the transition. Given that people will update to 3.0 w/o reading the fine print to understand what they have to configure to keep should and should_not available, it could be good to do something like:

class Object
  def method_missing(name, *args, &block)
    if %w[ should should_not ].include?(name.to_s)
      warn "should and should_not are not available by default in RSpec 3.0. To enable them, do XYZ."
    end
    super
  end
end

Redefining Object#method_missing like this kinda scares me but I can't think of a better way to give people a friendly heads-up about the change.

dchelimsky · 2012-03-04T16:57:14Z

Whether we release the configuration option as 2.x or 3.0, I want the default to start off the way it is now (with should and should_not available). If that means leaving it as the default until 4.0, so be it. That message will frustrate people, so I'd rather wait until expect has lived for a while as part of the lib.

The should used for one-liners is defined on ExampleGroup (not globally on object). I think it would be OK to leave that definition there, but have it delegate to expect instead of the global should.

myronmarston · 2012-03-04T17:38:56Z

Actually, here's a pretty decent syntax for one-liners with expect:

describe MyModel do
  expect_it { to validate_presence_of(:name) }
end

I'm generally reluctant to add new methods to the example group DSL, but here it's just an alias for it that reads nicer. to, not_to and to_not can be defined on Example to delegate to expect(subject).to/to_not.

@dchelimsky -- you're right that it does not harm to leave the should and should_not one-liner syntax since it's not the global should on kernel, but I think it would be jarring to disable should and should_not for a project and then still see it used liike that.

myronmarston · 2012-03-04T17:40:37Z

Random thought that occurs to me about my suggested expect_it syntax above: to me, expect_it suggests what's really happening (i.e. expect(subject)) in a way that the old syntax didn't. That's a plus!

dchelimsky · 2012-03-04T18:25:15Z

I'd rather figure out a way to configure things such that should remains should for one liners. Some possibilities:

config.enable_should_for_one_liners = true
config.enable_should_on_every_object = false

# or

config.enable_should :scope => :global
config.enable_should :scope => :one_liners

#etc

zdennis · 2012-03-05T02:28:40Z

I vote for having separate methods for this configuration opposed to the latter. The latter option looks like the second call to #enable_should is overriding the first.

myronmarston · 2012-03-05T03:54:02Z

I'd rather figure out a way to configure things such that should remains should for one liners.

I'm not sure I agree, but I don't necessarily disagree. Why would you prefer to keep should for one liners?

dchelimsky · 2012-03-05T15:40:13Z

There's a difference between reducing risks associated with a method added to all objects and should. I have no issue w/ should (re: @tpope's comment above, the move away from "should" in messages is orthogonal to any move away from "should" for expectations), and whatever we do here I'd like to minimize the friction imposed by such a change.

For me, expect_it { to matcher } has no real advantage to it { should matcher } and I think it has a couple of disadvantages: it's new syntax, and it's subtly different syntax from the in-line syntax. We'd end up fielding countless "expect { to matcher } gives a NoMethodError" bug reports.

alexrothenberg · 2012-03-05T17:17:22Z

If its just a question of syntax for the one liners what about expects_to like this?

describe MyModel do
  it { expects_to validate_presence_of(:name) }
end

Oh, and I do like the whole idea of this change. +1

samwgoldman · 2012-03-05T18:11:36Z

What about 3rd party libraries that extend RSpec? I maintain such a library, which uses "should," "should_not," and "should_receive" internally. I think that if these methods are removed in a future version, I would like to still let my library work with pre-removal and post-removal versions, instead of having to maintain two branches of my code. Can this change be implemented such that libraries continue to work?

dchelimsky · 2012-03-05T18:14:09Z

@samwgoldman we're talking about opt-in/out, not removal, so there shouldn't be a conflict.

samwgoldman · 2012-03-05T18:17:13Z

@dchelimsky Okay, but just to be absolutely certain I understand: if the application owner opts out, would an extension like this still work? https://gist.github.com/1777846

myronmarston · 2012-03-05T18:22:18Z

@samwgoldman -- two suggestions:

Once we add the expect syntax, I think it'll always be available (especially since it's already available for block expectations). So, you could refactor that to use the expect syntax and it would regardless of whether or not the user has opted in/out of should/`should_not. Note that if you did that your extension would not work on old rspec versions before this change.
Your extension could opt-in to the should/should_not syntax if the config option is available. That would allow it to work on old and new versions of RSpec. It would force the user to opt-in to the syntax, though.

Note: there's more to do here (documentation, etc); this is just a starting point for discussion and comments.

Also, fix the spelling of the class while I'm at it.

:should, :expect or both can be chosen.

myronmarston · 2012-05-09T06:31:04Z

I rebased (to get things current again) and implemented @dchelimsky's suggested config API. It was actually fairly easy to make it revertible as well.

I'll tackle disabling operator matchers next.

travisbot · 2012-05-09T06:35:11Z

This pull request fails (merged d9ab1a3 into 2e348ff).

myronmarston · 2012-05-09T06:38:19Z

Woah, that travis notification is pretty sweet. I'll tackle fixing it tomorrow.

travisbot · 2012-05-09T06:42:29Z

This pull request fails (merged ad2a757 into 2e348ff).

* Fix build on JRuby. Our sandboxing via forking didn't work on JRuby since fork isn't available. On JRuby we just re-enable all syntaxes at the end of each sandboxed example. * Testing this revealed that the way I was restoring a disabled syntax didn't always work. Based on the random order, sometimes spec/rspec/matchers/be_spec.rb:427 would fail with "TypeError: bind argument must be an instance of Kernel". * Refactored main logic into new syntax module, that can add the syntaxes to any class or module. Kernel/RSpec::Matcher defaults are provided for convenience. This also fixes the bind failure, by redefining the methods anew rather than re-binding the old ones.

travisbot · 2012-05-10T05:44:13Z

This pull request passes (merged 309c4f0 into 2e348ff).

travisbot · 2012-05-10T05:55:41Z

This pull request passes (merged 6de81c7 into 2e348ff).

@justinko

- Remove bang from method...as @justinko rightly pointed out, there's no corresponding bangless method so it didn't really follow convention here. - Use an early guarded return.

travisbot · 2012-05-11T17:51:04Z

This pull request passes (merged 8567ac6 into 2e348ff).

myronmarston · 2012-05-11T17:58:03Z

@justinko -- I just removed the bang. Thinking about it some more, I like the convention that blog post mentions. I've never thought very consciously about my bang usage.

BTW, as the code stands now, when you do this:

RSpec.configure do |rspec|
  rspec.expect_with :rspec do |c|
    c.syntax = :should
  end
end

...then this will be disallowed:

it 'tries to use the expect block syntax' do
  expect { something }.to raise_error
end

Prior to this pull request, expect could be used with a block as a more-readable alternative than lambda { }.should. Disabling the expect syntax causes all uses of expect to be disabled, not simply for the new functionality. I wasn't sure about doing it this way originally but it was far simpler than syntax = :should preventing expect(something) but allowing expect { something }, and it's more consistent...if the user tells rspec "I don't want to use the expect syntax" then I figure it makes sense to disable it entirely.

Another thing...RSpec::Expectations::Syntax.enable_should(DelegateClass) can now be used to deal with issues like #114, but I'm not sure if we should make that an official public API. I'm leaning towards marking it as a private API, and encouraging users to use the new expect syntax as the supported solution to those kinds of issues.

Finally, @justinko suggested match_array above as the matcher replacement for =~ and the more I think about it, the more I like it. Any opposition to using that as the name?

@dchelimsky, @justinko, or anyone else: thoughts on any of these?

justinko · 2012-05-11T19:51:49Z

Agreed on all points from me.

[ci skip]

This is needed because we've decided not to support operator matchers off of `expect(value).to`, and `match_array` is the best name we've come up with for it.

travisbot · 2012-05-12T05:22:01Z

This pull request passes (merged f00de57 into 3cf9110).

myronmarston · 2012-05-22T03:13:31Z

@dchelimsky -- anything else you want changed before I merge this in?

dchelimsky · 2012-05-22T03:39:47Z

@myronmarston nope - have at it.

Add support for `expect(value)` syntax.

coveralls · 2013-08-05T18:07:12Z

Changes Unknown when pulling f00de57 on expect_syntax into * on master*.

JonRowe · 2013-08-05T23:11:10Z

Lol coveralls bot. Although it has drawn my attention to the fact the branch still exists, can it be safely deleted now @myronmarston?

dchelimsky reviewed Mar 4, 2012
View reviewed changes

myronmarston added 3 commits May 8, 2012 21:04

Add support for expect(value) syntax.

d279c3b

Note: there's more to do here (documentation, etc); this is just a starting point for discussion and comments.

Don't expose #target as a public API.

0d7ce9c

Also, fix the spelling of the class while I'm at it.

Add configuration API for choosing an expectation syntax.

d9ab1a3

:should, :expect or both can be chosen.

Don't redefine should or expect when re-configured.

ad2a757

Prevent operator matchers from new expect syntax.

6de81c7

Cleanup code a bit.

8567ac6

- Remove bang from method...as @justinko rightly pointed out, there's no corresponding bangless method so it didn't really follow convention here. - Use an early guarded return.

myronmarston added 2 commits May 11, 2012 22:04

Add yard docs for new modules.

f1b8aa2

[ci skip]

Add match_array matcher method for the old =~ array matcher.

f00de57

This is needed because we've decided not to support operator matchers off of `expect(value).to`, and `match_array` is the best name we've come up with for it.

myronmarston added a commit that referenced this pull request May 22, 2012

Merge pull request #119 from rspec/expect_syntax

71a2c8d

Add support for `expect(value)` syntax.

myronmarston merged commit 71a2c8d into master May 22, 2012

myronmarston mentioned this pull request Jun 4, 2012

equal matcher broken for Delegator since 309c4f08b59651737ddc8c3c6e0167220b604b12 #148

Closed

myronmarston mentioned this pull request Jun 25, 2013

Enhance expect syntax expressiveness rspec/rspec-core#953

Closed

myronmarston mentioned this pull request Dec 23, 2013

Consider renaming (or aliasing) match_array to contain_exactly #398

Closed

myronmarston mentioned this pull request Jan 6, 2014

Don't use should betterspecs/betterspecs#15

Open

robwise mentioned this pull request Sep 11, 2014

Use is_expected.to instead of should thoughtbot/guides#215

Closed

Add support for expect(value) syntax. #119

Add support for expect(value) syntax. #119

Conversation

myronmarston commented Mar 4, 2012

Feature Summary

Open Questions

My Two Cents

justinko commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

dchelimsky Mar 4, 2012

Choose a reason for hiding this comment

myronmarston Mar 4, 2012

Choose a reason for hiding this comment

justinko commented Mar 4, 2012

justinko commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

justinko commented Mar 4, 2012

justinko commented Mar 4, 2012

tpope commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

justinko commented Mar 4, 2012

justinko commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

myronmarston commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

myronmarston commented Mar 4, 2012

myronmarston commented Mar 4, 2012

dchelimsky commented Mar 4, 2012

zdennis commented Mar 5, 2012

myronmarston commented Mar 5, 2012

dchelimsky commented Mar 5, 2012

alexrothenberg commented Mar 5, 2012

samwgoldman commented Mar 5, 2012

dchelimsky commented Mar 5, 2012

samwgoldman commented Mar 5, 2012

myronmarston commented Mar 5, 2012

myronmarston commented May 9, 2012

travisbot commented May 9, 2012

myronmarston commented May 9, 2012

travisbot commented May 9, 2012

travisbot commented May 10, 2012

travisbot commented May 10, 2012

travisbot commented May 11, 2012

myronmarston commented May 11, 2012

justinko commented May 11, 2012

travisbot commented May 12, 2012

myronmarston commented May 22, 2012

dchelimsky commented May 22, 2012

coveralls commented Aug 5, 2013

JonRowe commented Aug 5, 2013

Add support for `expect(value)` syntax. #119

Add support for `expect(value)` syntax. #119