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

is_expected with block expectations #805

Closed
xaviershay opened this Issue Jun 15, 2015 · 18 comments

Comments

Projects
None yet
9 participants
@xaviershay
Member

xaviershay commented Jun 15, 2015

The following situation just confused someone here at work:

describe 'is_expected with block expectation' do
  subject { raise }

  it { is_expected.to raise_error }
end

That reads like it should work, but doesn't because is_expected is defined as expect(subject).

I don't typically use is_expected, so not sure if there is a canonical way to do this already. It isn't listed at https://www.relishapp.com/rspec/rspec-core/docs/subject/one-liner-syntax

  1. Is there a "correct way" to do this?
  2. Is one-liner syntax something that we (as a core team) use much, or is it in the expect_any_instance_of bucket of extras?
  3. Should we add one-liner Relish documentation about using block matchers? I'm thinking yes.
  4. Is there a way to fix this transparently, and if yes, should we? My gut says no because it would be switching is_expected implementation based on matcher used and that seems gnarly.

Tangentially related discussion thread on separating block vs value expectations, in which we didn't discuss this case: #526

cc @rspec/rspec

@JonRowe

This comment has been minimized.

Member

JonRowe commented Jun 15, 2015

  1. Is there a "correct way" to do this?

Not with the one liner syntax that I'm aware of.

  1. Is one-liner syntax something that we (as a core team) use much, or is it in the expect_any_instance_of bucket of extras?

It's in the same bucket, I certainly don't recommend it.

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

We should mention they're not supported.

  1. Is there a way to fix this transparently, and if yes, should we? My gut says no because it would be switching is_expected implementation based on matcher used and that seems gnarly.

this has come up before and we shied away from it because the only way to really do it is to pass a block out of the subject block and then conditionally detect what to do with it, the last conversation we had around this resulted in the original issue raiser producing https://github.com/puyo/rspec-subject_call

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 15, 2015

Not with the one liner syntax that I'm aware of.

There is, if you're willing to make your subject a lambda:

describe 'is_expected with block expectation' do
  subject { -> { raise } }

  it { is_expected.to raise_error }
end

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

Is one-liner syntax something that we (as a core team) use much, or is it in the expect_any_instance_of bucket of extras?

I consider it in between. It's not something I use frequently, and it's an often abused feature of RSpec...but it's not generally problematic the way expect_any_instance_of is, and there are situations where it works really, really well. I think mustermann has a great example of its usage (although it's using the older should one-liner syntax, but I consider them interchangable). I think for a case like mustermann's, you want the details of the exact string you are matching against to be included in the doc output (as coming up with prose descriptions for all those would be tricky and more ambiguous). Letting the matcher generate the description for the example keeps them in sync, ensuring you don't wind up with doc strings that "lie" like comments can, which can lead to other problems.

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

We should mention they're not supported.

Agreed.

Is there a way to fix this transparently, and if yes, should we? My gut says no because it would be switching is_expected implementation based on matcher used and that seems gnarly.

I think the complexity to doing such a thing would be greater than the benefit. I also think it's good for people to understand that the expectation is being set on a lambda/proc that wraps an expression, and not on the return value of the expression itself. Any attempt to make this work nicely would hide that distinction which would only lead to more confusion.

Also, you'll generally get warnings from RSpec 3 when you attempt to use a block matcher against a non-block subject. That falls over for raise_error, unfortunately, because in an expression like expect(raise).to raise_error ruby evaluates the raise first and it aborts before RSpec even has a chance to do anything with it.

@xaviershay

This comment has been minimized.

Member

xaviershay commented Jun 16, 2015

For the mustermann-style case, I tend to use a custom method: https://github.com/xaviershay/pacebot/blob/master/spec/parse_spec.rb#L23 ... but then I'm the kind of heathen that tends to like assert methods :P

I'll put together a doc PR.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 16, 2015

but then I'm the kind of heathen that tends to like assert methods :P

😱

@xaviershay

This comment has been minimized.

Member

xaviershay commented Jul 18, 2015

I obviously haven't done this. I may still, but am relaxing the commitment expressed in my prior comment.

@xaviershay xaviershay added the small label Jul 18, 2015

@codesnik

This comment has been minimized.

codesnik commented Nov 24, 2015

if is_expected would be defined as

def is_expected
   expect { subject }
end

( using { } instead of ( )) then it should work. Also will help with is_expected.to change when subject is a method call with side effects, and it would make "unit specs" with a lot of describe "#method" do sections much shorter, while mantaining readability.

But it could slow down a little every is_expected call. Any other reasons, why it wasn't done in that way?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Nov 24, 2015

Any other reasons, why it wasn't done in that way?

The reason is that expect { subject } only works with block expectations. Non-block expectations (e.g. eq, include, be_within, be <, etc) can only be used off of expect(object), not expect { object }. There're a few reasons for that:

  • The generic expect {}.to matcher machinery does not evaluate the block -- instead the block is passed to the matcher so the matcher can evaluate it. For this block matchers, this is important because the matcher must "wrap" evaluation of the block with special logic (such as rescuing an error, etc).
  • That means that if you tried expect { 5 }.to eq(5), the eq matcher would receive proc { 5 } as an argument to match against 5. The eq matcher checks == of the two arguments. proc { 5 } == 5 is obviously not true so it won't match.
  • Procs are objects, too, which means that it's completely valid for eq to compare two proc objects for equality -- which in turn means that eq can't safely evaluate a proc if that's what it's given.

See #526 for more background and info on this.

@mrageh

This comment has been minimized.

Contributor

mrageh commented Jan 6, 2016

@xaviershay and @myronmarston what's the status of this issue?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jan 6, 2016

The consensus is to improve the documentation around this but no one has taken that on yet. Would you like to?

@mrageh

This comment has been minimized.

Contributor

mrageh commented Jan 10, 2016

yup @myronmarston I'll do that during this upcoming week

@mrageh

This comment has been minimized.

Contributor

mrageh commented Jan 19, 2016

I think we can close this PR as the docs around the one-liner syntax has been improved

@JonRowe JonRowe closed this Jan 19, 2016

@singpolyma

This comment has been minimized.

singpolyma commented Feb 25, 2016

Could maybe add a is_expected_block syntax for this?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Feb 25, 2016

Could maybe add a is_expected_block syntax for this?

I don't think it reads well but if you want to add it to a project (or build a gem that provides it), it's trivial:

module IsExpectedBlock
  def is_expected_block
    expect { subject }
  end
end

RSpec.configure do |c|
  c.include IsExpectedBlock
end
@cmolenda

This comment has been minimized.

cmolenda commented Jun 15, 2016

@myronmarston I basically ended up doing the same thing in a project, but I named it #will_be_expected

@guih

This comment has been minimized.

guih commented Jul 13, 2016

Since it's related to exceptions it could be something like

def is_expected_to_raise(*args)
  expect { subject }.to raise_exception(*args)
end
# and use it like
it { is_expected_to_raise SomeError, 'error message' }`

Even though I think naming the subject will produce very expressive sentence too.

describe '#some_method' do
  subject(:some_method) { described_class.new(...).some_method(invalid_args) }
  it { expect { some_method }.to raise_exception(SomeError, 'error message') }
end
@JonRowe

This comment has been minimized.

Member

JonRowe commented Jul 14, 2016

@guih that is certainly a way to do it, but I still don't feel it warrants being included in the main gems, as you've demonstrated it's easy enough to customise your own install when needed

@guih

This comment has been minimized.

guih commented Jul 14, 2016

Thanks for answering @JonRowe 😃

Sorry, I expressed myself badly in the previous comment.

I was not intended to have this included to the main gem, I was just trying to show that sometimes it's more expressive without it as an argument not to include it.

@khalilfazal

This comment has been minimized.

khalilfazal commented Dec 23, 2016

I've decided to use this in my project:

module WithinBlockIsExpected
  def within_block_is_expected
    expect { subject }
  end
end

RSpec.configure do |config|
  config.include WithinBlockIsExpected
end

and in spec:

it { within_block_is_expected.not_to raise_exception }

khalilfazal added a commit to khalilfazal/advent_of_code that referenced this issue Dec 27, 2016

added color to profiler
moved #singleton to BasicObject
changed title in README.md
added short circuit for bad year/day combinations
added tests for 404
using let instead of begin

created with_block_is_expected rspec/rspec-expectations#805 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment