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

Easy to read matcher #12

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

Easy to read matcher #12

andreareginato opened this issue Oct 3, 2012 · 8 comments
Labels

Comments

@andreareginato
Copy link
Collaborator

@andreareginato andreareginato commented Oct 3, 2012

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

@myronmarston
Copy link

@myronmarston 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
Copy link

@imajes 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
Copy link

@brycemcd 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
Copy link

@imajes 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
Copy link

@brycemcd 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
Copy link

@warmwaffles 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
Copy link

@dchelimsky 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
Copy link
Collaborator Author

@andreareginato 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
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants