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

Single expectation test #5

Open
andreareginato opened this Issue Oct 3, 2012 · 27 comments

Comments

Projects
None yet
@andreareginato
Copy link
Collaborator

andreareginato commented Oct 3, 2012

Write your thoughts about the "single expectation test" best practice.

@sj26

This comment has been minimized.

Copy link

sj26 commented Oct 3, 2012

This can, however, be the bane of performant testing.

@josephlord

This comment has been minimized.

Copy link

josephlord commented Oct 3, 2012

I'm fairly new and inexperienced with Rspec and especially where there is some significant set up it can make it very slow. I have sometimes written as separate tests during TDD, got it working and then written a combined test to leave running in CI process/automatic testing and commented out the separate tests. If the combined test fails I can quickly activate the separate tests to find the exact issue.

@davearonson

This comment has been minimized.

Copy link

davearonson commented Oct 3, 2012

I sometimes use an assertion to verify that the conditions are right for the test to be valid. For instance, on one project, we had a very complicated set of fixtures, in several related models. There were about a dozen devs, so we were often stepping on each other's fixtures. When I tested a scope, I would usually do "it finds only those that fit" and "it omits only those that don't fit", by looping through the found or omitted records, and verifying that they fit or not. However, this tactic depends on there being ANY records that do or don't fit. So, I preceded the loop with a test that the set contained at least one record. One of our pull-request-approvers regularly made the perfect the enemy of the good, strictly enforcing "one assert per test", and rejected that style, so I made the existence-check a separate test, much slower. (To those who say putting the assertion in a loop was already a violation, yes I could have set a flag upon error and checked it at the end of the loop, but that cuts out a lot of clue about which record violated it. Yes, I could have initted something to nil and set it to the offending record's id on error, but still....)

@myronmarston

This comment has been minimized.

Copy link

myronmarston commented Oct 3, 2012

I think this best practice, as written, is misleading and not helpful. Here's what I'd say instead:

  • In isolated unit specs, you want each example to specify one (and only one) behavior. Multiple expectations in the same example are a signal that you may be specifying multiple behaviors.
  • In tests that are not isolated (e.g. ones that integrate with a DB, an external webservice, or end-to-end-tests), you take a massive performance hit to do the same setup over and over again, just to set a different expectation in each test. In these sorts of slower tests, I think it's fine to specify more than one isolated behavior.

As in all things, software engineering is about tradeoffs, and this is no exception. It's bad to encourage cargo-cult thinking here.

@brycemcd

This comment has been minimized.

Copy link

brycemcd commented Oct 3, 2012

1 similar comment
@thermistor

This comment has been minimized.

Copy link

thermistor commented Oct 6, 2012

@coreyhaines

This comment has been minimized.

Copy link

coreyhaines commented Oct 18, 2012

One of the primary reasons for the "single assertion per test" is to provide easy and understandable traceability for what caused the failure, and keep multiple errors from mingling together and confusing this feedback.
While I do agree that single assertion tests are generally a good thing, I warn people to stay away from it as a guideline and instead focus on the goal: making test failures point directly to the cause of the error.

@jandudulski

This comment has been minimized.

Copy link

jandudulski commented Oct 31, 2012

👍 to @myronmarston

@mike-burns

This comment has been minimized.

Copy link

mike-burns commented Nov 9, 2012

Later you make the point that you should have custom matchers. That is actually a very pragmatic suggestion that accomplishes a similar goal to this suggestion.

@pelle

This comment has been minimized.

Copy link

pelle commented Dec 13, 2012

2 similar comments
@larrylv

This comment has been minimized.

Copy link

larrylv commented Dec 22, 2012

@rilian

This comment has been minimized.

Copy link

rilian commented Jan 15, 2013

@richardkmichael richardkmichael referenced this issue Feb 7, 2013

Open

Improve RSpec. #23

0 of 5 tasks complete
@andreareginato

This comment has been minimized.

Copy link
Collaborator

andreareginato commented Mar 9, 2013

Updated using the @myronmarston description.
Any correction and comment to the updated guideline is appreciated.

@uptrending

This comment has been minimized.

Copy link

uptrending commented May 31, 2013

GOOD (NOT ISOLATED) should say BAD (NOT ISOLATED)

@lucaspiller

This comment has been minimized.

Copy link

lucaspiller commented Jul 19, 2013

+1 to @myronmarston (I was going to say exactly that :D)

@alexandru-calinoiu

This comment has been minimized.

Copy link

alexandru-calinoiu commented Oct 20, 2013

Please consider updating this spec as assign_to matcher is no longer part of the latest version of shoulda. More details here http://robots.thoughtbot.com/post/47031676783/shoulda-matchers-2-0

@abuisman

This comment has been minimized.

Copy link

abuisman commented Nov 19, 2013

@uptrending

Why so? The text states that it should say 'GOOD', so why do you think it isn't? You could be right of course, but why?

@derick

This comment has been minimized.

Copy link

derick commented Feb 22, 2014

@andreareginato

This comment has been minimized.

Copy link
Collaborator

andreareginato commented Mar 10, 2014

Guys, if you think of any change, please send us a pull request with the update.

@dcorking

This comment has been minimized.

Copy link

dcorking commented Jul 26, 2014

Where,and in which versions, is the is_expected_to message defined? I can't find it in the rspec API docs. Is it meant to be locally defined by the user?

@dcorking

This comment has been minimized.

Copy link

dcorking commented Jul 26, 2014

@andreareginato according to Myron's reply, the message is is_expected. How did you get is_expected_to (with 2 underscores) to work for you? (Is it a typo?)

@andreareginato

This comment has been minimized.

Copy link
Collaborator

andreareginato commented Jul 28, 2014

Yep. It was a typo. Sorry for that. Now it's up and running with the needed corrections.

@telmofcosta

This comment has been minimized.

Copy link

telmofcosta commented Apr 2, 2015

subject { color }
it "is blue" do
  expect(subject.RGB.R).to be < 0.2
  expect(subject.RGB.G).to be < 0.2
  expect(subject.RGB.B).to be > 0.8
end

In this example, I just want to test that some action gives me the color blue. But the only way I have to test that it's blue is by its RBG components. If I don't break the single expectation test pattern, I loose the "why" I have this test. It is important for me that the reason I want this is that I want to make sure the color is some kind of blue.

What is wrong with this reasoning?

@myronmarston

This comment has been minimized.

Copy link

myronmarston commented Apr 2, 2015

What is wrong with this reasoning?

Absolutely nothing. Your example is specifying one behavior, and that's what's important. The fact that it takes 3 expectations to do isn't particularly important.

That said, in RSpec 3.1+, you could use a single expectation:

expect(subject.RGB).to have_attributes(
  R: a_value < 0.2,
  G: a_value < 0.2,
  B: a_value > 0.8
)
@dayyan

This comment has been minimized.

Copy link

dayyan commented Apr 21, 2015

FYI the example method respond_with_content_type has been deprecated in RSpec 2.0.

@xaviershay

This comment has been minimized.

Copy link

xaviershay commented Aug 30, 2015

While I do agree that single assertion tests are generally a good thing, I warn people to stay away from it as a guideline and instead focus on the goal: making test failures point directly to the cause of the error.

@coreyhaines would you agree that aggregate_failures (introduced in 3.3!) addresses this concern?

The "not isolated" example could be updated to include it:

it 'creates a resource', :aggregate_failures do
  expect(response).to respond_with_content_type(:json)
  expect(response).to assign_to(:resource)
end

Relevant docs: https://www.relishapp.com/rspec/rspec-expectations/v/3-3/docs/aggregating-failures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment