Skip to content

New syntax

gitlab-qa-bot requested to merge new_syntax into master

Created by: myronmarston

This is my first pass stab at the new syntax discussed in #153 (closed).

It's not ready to merge yet but I wanted to start the code review conversation. Notes:

  • I decided to call the old syntax the :direct syntax because you call methods directly on the object...and the new syntax the :wrapped syntax because you wrap the object with a method like expect or allow. What do others think of these names?
  • When we introduced the new syntax to rspec-expectations, we decided to default to both syntaxes enabled. This was necessary because we had supported expect { }.to for a long time, so it would have been a backwards incompatibility to only enable should by default. This time around, I'm thinking of only enabling the existing :direct syntax by default. Generally, I think projects should pick one syntax or the other, and only use both syntaxes during a transition period. There's no need to enable both syntaxes for backwards compatibility like there was for rspec-expectations. On the other hand, it might encourage more folks to try out the new syntax if both are enabled. I'm kinda on the fence here.
  • For any_instance stubbing, we never figured out what to do for the new syntax. For now, I've gone with expect_any_instance_of(klass).to receive and allow_any_instance_of(klass).to receive but @alindeman put together a gist of some other possibilities to consider.
  • With the current version of this code, the any instance stuff doesn't actually work when you enable only the :wrapped syntax (it works fine when both syntaxes are enabled). The problem is that the way that any instance works, it records should_receive and stub and then plays it back on the instance later...but with the :direct syntax disabled, the methods aren't there to playback onto. I'm not sure of a fix yet.
  • I didn't realize this, but the old syntax allowed any object to be as_null_object. I personally think that only makes sense on a test double, not on a partial mock. Any objections to only supporting it on test doubles for the new syntax? (On a side note, I need to enable it when only the new syntax is enabled).
  • stub_chain: Do we need to bring this forward to the new syntax? (It's kind a code smell, IMO).
  • The old syntax supported obj.stub(:msg_1 => 1, :msg_2 => 2) but I can't think of a good way to support that with the new syntax. (We'll still support double(:msg_1 => 1, :msg_2 => 2), of course). Any objects to not bringing that forward to the new syntax?
  • unstub isn't (yet) supported with the new syntax, and I'm on the fence about whether to support it or not. (There's discussion in #153 (closed) about this).
  • When rspec-mocks is used but rspec-expectations is not, expect is not available. It's trivial to write one (it'll be < 10 lines of code both for the method and the class that it instantiates), but I need to figure out how to reliably define expect if and only if it is actually needed, because we don't want it overriding expect from rspec-expectations. (I've implemented this now in ee9d82ee).
  • Lots of documentation updates are needed.

Please review and give your feedback!

Merge request reports