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

Easy to read matcher #12

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

Comments

Projects
None yet
6 participants
@andreareginato
Collaborator

andreareginato commented Oct 3, 2012

Write your thoughts about the "Easy to read matcher" best practice.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Oct 3, 2012

Your example uses the have(n).things matcher, which we've been talking about deprecating. It's probably better to use an example we're not considering deprecating :).

myronmarston commented Oct 3, 2012

Your example uses the have(n).things matcher, which we've been talking about deprecating. It's probably better to use an example we're not considering deprecating :).

@imajes

This comment has been minimized.

Show comment
Hide comment
@imajes

imajes Oct 3, 2012

should == n is pretty straight forward. have(n).things seems overly sacharin and doesn't really add much value.

imajes commented Oct 3, 2012

should == n is pretty straight forward. have(n).things seems overly sacharin and doesn't really add much value.

@brycemcd

This comment has been minimized.

Show comment
Hide comment
@brycemcd

brycemcd Oct 3, 2012

I'd assert that a better practice would be instead:

describe "#save!" do
   context "with invalid data" do
      subject { model = User.new(:email => 'a@invalid.com) }
      lambda do
          subject.save!
          end.should change(User, :count).by(0)
    end
  end
end 

Setting integers in a spec is a code smell to me

brycemcd commented Oct 3, 2012

I'd assert that a better practice would be instead:

describe "#save!" do
   context "with invalid data" do
      subject { model = User.new(:email => 'a@invalid.com) }
      lambda do
          subject.save!
          end.should change(User, :count).by(0)
    end
  end
end 

Setting integers in a spec is a code smell to me

@imajes

This comment has been minimized.

Show comment
Hide comment
@imajes

imajes Oct 3, 2012

i think we can argue that point, i'd say that your lambda and it's chain is way too complex for many developers to quickly scan and grasp the intent.

i understand how you want to restrict magic numbers, and brittle code, but the idea (imho) is that a test should be the simplest expression of intent, even if it's convoluted and non performant -- because this should be as good as documentation for your project.

whilst i understand what your test does, i had to read it carefully to manually unroll it in my head -- and i'm not a compiler.

imajes commented Oct 3, 2012

i think we can argue that point, i'd say that your lambda and it's chain is way too complex for many developers to quickly scan and grasp the intent.

i understand how you want to restrict magic numbers, and brittle code, but the idea (imho) is that a test should be the simplest expression of intent, even if it's convoluted and non performant -- because this should be as good as documentation for your project.

whilst i understand what your test does, i had to read it carefully to manually unroll it in my head -- and i'm not a compiler.

@brycemcd

This comment has been minimized.

Show comment
Hide comment
@brycemcd

brycemcd Oct 3, 2012

yeah, I agree that the test should be the simplest expression of intent. There's a lot of value in that. Perhaps the lambda should be written as a custom matcher (or added rspec proper) to read more like:

User.should_change_total_count by(0)

brycemcd commented Oct 3, 2012

yeah, I agree that the test should be the simplest expression of intent. There's a lot of value in that. Perhaps the lambda should be written as a custom matcher (or added rspec proper) to read more like:

User.should_change_total_count by(0)

@warmwaffles

This comment has been minimized.

Show comment
Hide comment
@warmwaffles

warmwaffles Oct 3, 2012

I think you should utilize the new RSpec expectations. They read much nicer. This is how I have been testing some of my things with ActiveRecord but it's probably not the best solution, but it makes sense.

expect { model.save! }.to raise_error Mongoid::Errors::DocumentNotFound
expect(collection.count).to eq(2)

warmwaffles commented Oct 3, 2012

I think you should utilize the new RSpec expectations. They read much nicer. This is how I have been testing some of my things with ActiveRecord but it's probably not the best solution, but it makes sense.

expect { model.save! }.to raise_error Mongoid::Errors::DocumentNotFound
expect(collection.count).to eq(2)
@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Oct 6, 2012

+1 to @myronmarston's comment. Elaborating on why ...

collection.size.should == 2
collection.should have(2).items

The latter was added to RSpec back in the day when there was no Cucumber and RSpec was trying to walk the line between readability for tech and non-tech folks. Today I'm pretty convinced that a) nearly no non-tech folk read rspec code, b) some non-tech folk do read the output, but that's all from the docstrings, and c) size.should == 2 is immediately understandable to first-time tech readers, whereas collection.should have(2).items is not.

dchelimsky commented Oct 6, 2012

+1 to @myronmarston's comment. Elaborating on why ...

collection.size.should == 2
collection.should have(2).items

The latter was added to RSpec back in the day when there was no Cucumber and RSpec was trying to walk the line between readability for tech and non-tech folks. Today I'm pretty convinced that a) nearly no non-tech folk read rspec code, b) some non-tech folk do read the output, but that's all from the docstrings, and c) size.should == 2 is immediately understandable to first-time tech readers, whereas collection.should have(2).items is not.

@andreareginato

This comment has been minimized.

Show comment
Hide comment
@andreareginato

andreareginato Oct 6, 2012

Collaborator

I agree with @myronmarston, thanks for pointing it out. Right now I've just removed the second example and I've left only the lambda/expect one. If you have some more good examples, I'll be to add them.

Collaborator

andreareginato commented Oct 6, 2012

I agree with @myronmarston, thanks for pointing it out. Right now I've just removed the second example and I've left only the lambda/expect one. If you have some more good examples, I'll be to add them.

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