rspec_rails_4 icon indicating copy to clipboard operation
rspec_rails_4 copied to clipboard

Ch3, p.26, code example - Three issues

Open olerass opened this issue 12 years ago • 2 comments
trafficstars

The following issues are regarding the code example on the top of page 26 as well as the first line after this example:

This spec uses RSpec's include matcher to determine if the array returned by Contact.by_-letter("J")-and it passes!

Three issues here:

  • The code excerpt Contact.by.. in the text is incorrectly hyphenated which makes it look like the hyphen is a part of the method name. I've seen this in another LeanPub book recently - it may be more than a local issue?
  • I don't understand what the cited sentence is trying to convey: If the array what? Whatever the included matcher does, it's not explained here (and it sounds like it should have been). It seems like something's missing between Contact.by_-letter("J"). and "and it passes!".
  • Either I misunderstood something or this spec doesn't make sense compared to what you describe it should do. From what I understand, what you're trying to test is the situation in which a selection returns NO records (e.g. no matches / empty array) - an edge case test. This is what you write on page 25: "what about occasions where a selected letter returns no results?". Examples of such situations are (with the data provided in the example): Contact.by_letter("K"), Contact.by_letter("X") and so on. Clearly these fit your description of returning no results. However, this is not what your spec is testing. Your spec is testing that the by_letter method returns only the correct results. (and by the way shouldn't the test name thus be something like 'doesn't return non-matching results'?). In practice this is tested by checking that smith is not returned when we ask for the letter "J". This is a perfectly valid test, and without it, an implementation that just returns all records sorted all the time would pass our tests. I believe this is the reason you want to have this test, but it's not what you say in the book. Instead I got very confused when I read this and the following sections because I was expecting a spec that checked for an empty array, but the actual spec tests something completely different.

Book version: 2013-10-29 (PDF)

olerass avatar Oct 31 '13 13:10 olerass

I felt this part was a little strange too.

expect(Contact.by_letter("J")).to_not include smith does not match "This spec uses RSpec's include matcher to determine if the array returned by Contact.by_letter("J")"

The code uses to_NOT.

JunichiIto avatar Feb 04 '14 23:02 JunichiIto

In addition, I think expect(Contact.by_letter("J")).to_not include @smith is useless because expect(Contact.by_letter("J")).to eq [@johnson, @jones] means not include @smith.

Therefore, dividing with context block is also useless. And I tried to show alternative example for it -- but I couldn't :(

However, as for the validation examples, context might be used effectively like this:

require 'spec_helper'

describe Contact do
  describe 'validation' do
    before :each do
      @contact = Contact.new(
          firstname: 'Junichi',
          lastname: 'Ito',
          email: '[email protected]'
      )
    end

    context 'with a firstname, lastname and email' do
      it 'is valid' do
        expect(@contact).to be_valid
      end
    end

    context 'without a firstname' do
      before :each do
        @contact.firstname = nil
      end

      it 'is invalid' do
        expect(@contact).to have(1).error_on(:firstname)
      end
    end
  end
end

But in this case using context might be verbose. It can be written without context:

require 'spec_helper'

describe Contact do
  describe 'validation' do
    before :each do
      @contact = Contact.new(
          firstname: 'Junichi',
          lastname: 'Ito',
          email: '[email protected]'
      )
    end

    it 'is valid with a firstname, lastname and email' do
      expect(@contact).to be_valid
    end

    it 'is invalid without a firstname' do
      @contact.firstname = nil
      expect(@contact).to have(1).error_on(:firstname)
    end
  end
end

It is difficult to show simple and easy-to-understand alternative, hmm ...

JunichiIto avatar Mar 13 '14 00:03 JunichiIto