Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit block expectation syntax #76

Closed
pirj opened this issue Dec 28, 2018 · 11 comments

Comments

@pirj
Copy link
Member

commented Dec 28, 2018

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

This comment has been minimized.

Copy link

commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@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

This comment has been minimized.

Copy link

commented Jun 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

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.

@pirj pirj added the question label Jul 17, 2019

@tomdalling

This comment has been minimized.

Copy link

commented Jul 18, 2019

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

This comment has been minimized.

Copy link

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

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

This comment has been minimized.

Copy link

commented Jul 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

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 added a commit that referenced this issue Jul 23, 2019

Add Implicit Block Expectation guideline
Implicit syntax is discouraged to use by RSpec Core team and the
majority of voters.

There were no good arguments for using the syntax except for brevity and
avoiding repetition, but there are better options to achieve the same
goal, e.g. by extracting the lengthy block to methods (instead of
putting it inside the lambda).

Fixes #76

https://www.reddit.com/r/ruby/comments/cejl3q/call_for_discussion_rspec_implicit_block/
https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
#76

pirj added a commit that referenced this issue Jul 23, 2019

Add Implicit Block Expectation guideline
Implicit syntax is discouraged to use by RSpec Core team and the
majority of voters.

There were no good arguments for using the syntax except for brevity and
avoiding repetition, but there are better options to achieve the same
goal, e.g. by extracting the lengthy block to methods (instead of
putting it inside the lambda).

Fixes #76

https://www.reddit.com/r/ruby/comments/cejl3q/call_for_discussion_rspec_implicit_block/
https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
#76

@pirj pirj referenced this issue Jul 23, 2019

Merged

Add ImplicitBlockExpectation cop #789

8 of 8 tasks complete

@pirj pirj closed this in #103 Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.