Implicit block expectation syntax

[pirj]

The following syntax is possible

subject { -> { do_something } }
it { is_expected.to change(:something).to(...) }

which is the same as:

it { expect { do_something }.to change(:something).to(...) }

but the former comes with a few drawbacks:

• it's an exception to the case when subject is not cached
• it's near impossible to detect with static analysis tools (RuboCop, Reek, ...) that it is actually a block expectation
• it's rarely used and not known by many

Both examples below will pass:

subject { -> { fail 'Something bad happened' } }
specify do
  is_expected.to raise_error(/Something/)
  is_expected.to raise_error(/bad happened/)
end

before { @a=1 }
subject { -> {@a+=1} }
specify do
  is_expected.to change { @a }.by(1)
  is_expected.to change { @a }.by(1)
  is_expected.to change { @a }.by(1)
  is_expected.to change { @a }.by(1)
  is_expected.to change { @a }.by(1)
  is_expected.to change { @a }.by(1)
end

Keeping in mind that:

• we don't recommend using an implicit subject (should/is_expected) except in some cases,
• the benefit of using this syntax is quite low

There seem to be not much usage cases left for implicit block expectation syntax.
What do you think of recommending against this syntax?

Related blog post on blog.rubystyle.guide, reddit comments.

[bolshakov]

it's an exception to the case when subject is not cached

And this is a good thing. When testing for side effects a subject should not be cached

it's near impossible to detect with static analysis tools (RuboCop, Reek, ...) that it is actually a block expectation

The code is for humans, not for machines. Right?

it's rarely used and not known by many

I'd prefer to have evidence of that. I've seen a lot of such code. And this suggestion not aligned with current best practices http://www.betterspecs.org/#subject

[pirj]

@bolshakov

When testing for side effects a subject should not be cached

Why?
subject is defined inside MemoizedHelpers module. It's not explicitly mentioned in subject docs, but for let: return value is memoized after the first call, let!: providing a memoized reference to that state, and subject!: providing a memoized reference to that state it is. Do you have some considerations in mind that subject as opposed to its siblings should not be memoized?

Not memoizing something that is memoized according to the documentation is at least confusing.

The code is for humans, not for machines.

It is indeed. Humans will suffer from the same lookup problem as machines.
Of course, humans may do a better job with it, but how time-consuming is that?

it's rarely used and not known by many

I'd prefer to have evidence of that.

Sure, here you go, no usages found in real-world-ruby-apps:

pirj@air ~/source/real-world-ruby-apps (master *+) $ rg 'subject.+->.*\{'
apps/capistrano/spec/lib/capistrano/configuration_spec.rb
97:        subject { config.fetch(:key, -> { :lambda }) }
111:        subject { config.fetch(:key, -> { -> { "some value" } }) }
118:        subject { config.fetch(:key, proc { -> { "some value" } }) }
125:        subject { config.fetch(:key, -> { proc { "some value" } }) }
132:        subject { config.fetch(:key, ->(c) { c }).call(42) }

apps/middleman/middleman-core/spec/middleman-core/core_extensions/data_spec.rb
48:        @subject.callbacks :foo, -> { ['bar'] }
55:        @subject.callbacks :foo, -> { ['bar'] }
56:        @subject.callbacks :foo, -> { ['baz'] }
86:        @subject.callbacks :foo, -> { { 'bar' => 'baz' } }
87:        @subject.callbacks :wu, -> { %i[tang clan] }
96:        @subject.callbacks :foo, -> { { 'callback' => 'data' } }

not aligned with current best practices http://www.betterspecs.org/#subject

Can you please explain in more detail how expect { do_something } goes against "DRY them up"? Am I missing something?

[bolshakov]

Not memoizing something that is memoized according to the documentation is at least confusing.

That is not a problem. Since subject is a lambda. The lambda itself is memoized, but the result of its evaluation is not.

Sure, here you go, no usages found in real-world-ruby-apps:

Only 12 of 62 real word ruby app uses RSpec. Only 5 of 12 uses .to change syntax. I've seen real word projects (not open source) which uses that kind of syntax a lot.

Can you please explain in more detail how expect { do_something } goes against "DRY them up"? Am I missing something?

No need to repeat expect { do_sothing } again and again, you can write is_expected. instead.

I don't mind about having such a cop, but I don't like an idea to enforce personal preferences via rubocop.

[dgollahon]

Personally, when I want to test a side effect I usually do something like

def perform
# ...
end

and then do any of

expect { perform }.to raise_error(...)
expect { perform }.to change { ... }.by(n)
# or just invoke it and have an expect after
whatever_setup
perform
expect(some_state).to be(true)

or something similar.

I would find the memoized lambda expect syntax very surprising if I ran across it. I've never seen it used before on projects I've worked on, but my main opinion against it is just that I don't find should/is_expected worth it in most cases and think the explicit form is usually worth the typing. I think that's more/less in line with the style guide last I read it, so if there's an argument against the syntax that seems like the main one to me.

That said, because this seems to be rare, I'm not sure it definitely deserves specific guidance.

Re:

I don't mind about having such a cop, but I don't like an idea to enforce personal preferences via rubocop.

Probably large swaths of rubocop-* and the associated style guides could easily be classified as personal preferences. There are lots of things that would be about similarly good either way, but there's benefit for project and community homogeneity for readability. I don't know if this is one of those cases, but I don't know if we can really separate out personal preferences from guides in practice.

[pirj]

That said, because this seems to be rare, I'm not sure it definitely deserves specific guidance.

@dgollahon Agree to some extent. However, there's a guideline that recommends Flip-flops, and I bet nobody has seen it in the wild. What inclines me to propose this guideline is that this is a very surprising syntax even for seasoned developers, and the fact that it may actually be evaluated twice is far from being obvious.

@bolshakov I have a déjà vu, I think we already had this discussion.
So, pros for the syntax that I'm aware of are:

• ability to write one-liners is_expected.to change { ... }

Cons are from above.

There is no single example in RSpec repositories how to use this syntax, and documentation never mentions it.

Quote from @myronmarston:

pretty obtuse and not something I'd recommend, generally.

@JonRowe about one-liner syntax is general:

It's in the same bucket (of extras, along with e.g. expect_any_instance_of), I certainly don't recommend it.

Also:

Should we add one-liner Relish documentation about using block matchers? I'm thinking yes.

We should mention they're not supported.

subject { -> { ... } } is a hack that works by a coincidence.
is_expected is defined as expect(subject), when, as you point out, that lambda itself is being memoized.
When used with a block expectation, it works not as by design and is calling a wrong to definition that is not intended to be used with block expectation matchers and targets (this is the right one for block targets).
It turns out that the lambda is passed as an actual to matchers, and matchers call an actual.call to evaluate its value. What was expected to be a block, is a lambda.

With that in mind, subject { -> { ... } } is an abuse, undocumented and recommended against by RSpec Core team.

Do you think the pros overweight cons?

[dgollahon]

When used with a block expectation, it works not as by design and is calling a wrong to definition that is not intended to be used with block expectation matchers and targets (this is the right one for block targets ).

Woah, that's interesting. That strikes me as a relatively strong argument against.

[tomdalling]

Here's some more explanation for anyone who is confused.

There are two kinds of expectations. Let's call them "value expectations" and "block expectations". Certain matchers only work with one or the other. For example be_an only works with value expectations, and raise_error only works with block expectations.

expect(x).to be_an(Integer) # value: works
expect { x }.to be_an(Integer) # block: ~>
# You must pass an argument rather than a block to `expect` to use the
# provided matcher (be a kind of Integer), or the matcher must implement
# `supports_block_expectations?`.

expect { x }.to raise_error(Whatever) # block: works
expect(x).to raise_error(Whatever) # value ~>
# expected Exception but was not given a block

But when subject is a proc object, is_expected behaves like both types of expectation at the same time. RSpec is kind of automatically guessing which type you were intending.

subject { -> { raise "hello" } }
it { is_expected.to be_a(Proc) } # works
it { is_expected.to raise_error("hello") } # works

[rlue]

I'd be curious to hear some examples of when it's semantically appropriate for the subject of a spec to be a lambda/block. In general, I think "subject" should be an instance of the class specified in the top-level describe block, or more loosely, the object under test.

[pirj]

Not necessarily in the top-level describe according to subject in a nested group with a different class (innermost wins) . This only relates to the implicitly defined subject.

With the explicitly defined subject, I'm not aware of a commonly used practice how should the subject relate to the described class.

[rlue]

Wow, you know your stuff. I stand corrected; the official docs contain an example of a spec on Array where subject { element_list.pop }.

[pirj]

That's a different case actually, it's not a lambda, the value of the pop call is cached, it can't be used with a block matcher (at least unless the result of pop call is a lambda).

[pirj]

Yet another confirmation directly from rspec-core source:

The one-liner syntax only works with non-block expectations (e.g. expect(obj).to eq, etc) and it cannot be used with block expectations (e.g. expect { object }).

[amilligan]

I'm sorry I'm late to this conversation, again; I just came across this change with the most recent RSpec release. I appreciate this matter appears settled, but I have to disagree with the outcome. Most practically, I work on multiple projects that have literally thousands of examples written with block subjects. Even the simplest change to work around this deprecation is hours of time I just don't have.

More academically, as @tomdalling pointed out in his helpful post above, there are two different types of expectations; this is because there are two different types of methods in object oriented software: methods that returns a value, and methods that have a side effect. Yes, a method can do both (e.g. ActiveRecord::Base#save), but object design principles have recommended against this since before anyone put together the words "Single Responsibility Principle."

One of the lovely things about block subject syntax is that it succinctly makes a clear distinction between methods meant to return a value, and methods meant to have a side effect. I use RSpec for documentation as much as for executable examples, and I can see from the first two lines of any well-written describe block what sort of method I'm looking at:

  describe("#wibble") do
    subject { -> { thing.wibble } } # => side effect
...

In fact, I see the elision of the return value from the block as a good thing: you can't cheat and add a test for the return value (thus violating SRP by having a method with a side effect and a return value).

I would argue (a la David West) that methods with side-effects are preferable to methods that return values (behavior over state); they're certainly almost always more complex and interesting, and therefor more important to write clear specs for.

I appreciate the technical reasoning that has gone on in this thread regarding why block subjects are to be avoided, but I would sincerely ask you to consider replacing this functionality with something equally powerful and expressive, rather than simply deprecating it. I get that examples for side effects can be replicated thusly:

  describe("#wibble") do
    subject { -> { thing.wibble(with, bunch, o, parameters, and, such } }
    it { should do_the_dew }

# becomes:

  describe("#wibble") do
    def perform # or some other method name that doesn't create confusion with Sidekiq
      thing.wibble(with, bunch, o, parameters, and such)
    end

    it { expect { perform }.to do_the_dew }

But, the second version certainly does not read any more clearly than the first. One of RSpec's great strengths is the readability (and thus usefulness as documentation) of the examples. This is more typing for a less clear result. And raw methods hardly fit the aesthetic profile of an RSpec spec. In addition, adding raw methods in RSpec describe/context blocks can be a quick trip to Crazy Town, not to mention Test Pollution Town, for anyone not familiar with how the internals of RSpec work.

If you must remove this capability, would you consider replacing it with a distinct method? E.g.:

  describe("#blorf") do
    subject { thing.blorf }
...

  describe("#wibble") do
    subject_action { -> { thing.wibble } }

A Rubocop cop could autocorrect that reasonably easily, which would provide a lifeline for folks who do use this.

Regarding the assertion that real-world projects don't use this syntax, I've been using RSpec extensively for fifteen years now, including on dozens of Rails projects at Pivotal Labs. I have no idea how many examples I've personally written, or helped write, but I'm sure it's well into six figures, many of which use this mechanism. I've worked with many people who test at least as extensively, many of whom use this mechanism. I fully appreciate that some people may entirely reasonably not like it, or find it intuitive,, but it is entirely possible to use it effectively on large-scale projects with large, complex spec suites.

[pirj]

If it's your personal preference, I'd suggest you to define a helper method

def it_should
  expect { subject }
end

and use it as

it_should.do_the_dew

[amilligan]

To be honest, my personal preference is that you please not eliminate a perfectly effective and useful testing strategy.

I understand the concerns with caching the subject, and I get that it's a perfectly valid opinion to not like the mechanism. But, I feel it's also valid to prefer this mechanism. As with many rubocop style choices you could default to raising an error, but allow users to opt in to allowing it. Let's keep the tent as big as possible.

[pirj]

This style guide is not affiliated with RSpec.
RSpec team decided that the fact that this syntax worked is coincidental, and is an abuse of the internal mechanisms.
I suggest opening an issue there if you feel that it needs to be brought back.

Yet another random thought considering the example you mentioned

subject { -> { thing.wibble(with, bunch, o, parameters, and, such } }
it { should do_the_dew }

do you think the following would be acceptable in terms for readability?

subject_with_side_effects(:wibble!) { thing.wibble(with, bunch, o, parameters, and, such }
it { does the_dew }
# or
it { wibble!.to do_the_dew }

It's easy to implement subject_with_side_effects and does as wrappers on top of expect { } block syntax.