everydayrails / rspec_rails_4 Goto Github PK

Hi @ruralocity, I encountered some issues in chapter 5 when translating.

1. P57, 2nd paragraph: "The examples using invalid attributes use the :invalid_message factory we set up way back at the beginning of this chapter.", here the :invalid_message should be :invalid_contact.
2. P60: "Testing DESTROY requests". Actually there are no HTTP request names "DESTROY". Should be "DELETE" as in the previous versions.
3. P62: link_to 'Export', messages_path(format: :csv), should be link_to 'Export', contacts_path(format: :csv).
4. P63:

defindex
  @contacts = Contact.all
  respond_to do |format| format.html # index.html.erb format.csv do
    send_data Contact.to_csv(@contacts),
      type: 'text/csv; charset=iso-8859-1; header=present',
      :disposition => 'attachment; filename=contacts.csv'
    end
  end
end

I think wen should sticky with the new Hash syntax here.

1. P64:

it "returnscommaseparatedvalues" do
  create(:contact,
    firstname: 'Aaron',
    lastname: 'Sumner',
    email: '[email protected]')
  expect(Message.to_csv).to match /Aaron Sumner,[email protected]/
end

Here the Message should be Contact.

1. P64:

it "returnsJSON-formattedcontent" do
  message = create(:message)
  get :index, format: :json
  expect(response.body).to have_content message.to_json
end

Since we do not manage messages in this new version, I think this example should use contact.

As the book has been updated to use RSpec's newer expect() syntax, would it be worth updating the Mocks and Stubs section to use the newer expectation syntax too?

e.g.

Contact.stub(:persisted?).and_return(true)

could now be

allow(Contact).to receive(:persisted?).and_return(true)

Of course both are fine, but it seems like RSpec is moving in the direction of the latter.

In the pdf version of the book, on page 33, there's a paragraph starting with:

Edit the parameters passed to the factory within the expectation.

But we're not actually using factories yet, are we?

Looks like selenium-webdriver versions before 2.35.1 had a loose dependency on rubyzip, and when the latter was update to 1.0.0 caused a LoadError in ActiveSupport. Fix this by either requiring selenium-webdriver ~> 2.35.1 or rubyzip < 1.0.0 in your Gemfile.

I will update the sample source and book this week. Thanks to reader Geoffroy for pointing this out.

There's a slight code style inconsistency between the GET #edit and other specs on page 69. Specifically, the #edit spec uses an intermediary variable to store the created contact while the #create spec and others don't.

Book version: 2013-10-29 (PDF)

The _redirects to contacts#show" test on page 55 has a subtle bug:

it "redirects to contacts#show" do
 post :create, contact: attributes_for(:contact, phones_attributes: @phones)
 expect(response).to redirect_to contact_path(assigns[:contact])
end

Look at what RSpec will output if ContactsController#create doesn't assign @contact:

2) ContactsController POST #create with valid attributes redirects to contacts#show
     Failure/Error: expect(response).to redirect_to contact_path(nil)
       Expected response to be a redirect to <http://test.host/contacts/> but was a redirect to <http://test.host/contacts/1>.
       Expected "http://test.host/contacts/" to be === "http://test.host/contacts/1".
     # ./spec/controllers/contacts_controller_spec.rb:101:in `block (4 levels) in <top (required)>'

This tells us nothing about what the failure is (well, it does as it says contact_path(nil) which hints to the problem, but the message is still confusing). It is misinforming because it says we expect to be redirected to "http://test.host/contacts/" but we were redirected to "http://test.host/contacts/1" which is the correct url.

The problem is subtle. By using contact_path(assigns[:contact]) the test implicitly expects assigns[:contact] to be not nil. The problem is the implicitness; if assigns[:contact] returns nil contact_path(nil) returns /contacts/ which is equivalent to contacts_path. I don't know if this is a bug or a feature in Rails - I don't see why contact_path(nil) should ever be a valid path since you can always use contacts_path instead? Anyway, this is why the test fails with a wrong message.

To solve this one should at least make the assumption explicit by explicitly expecting assigns[:contact] to be not nil:

it "redirects to contacts#show" do
  post :create, contact: attributes_for(:contact, phones_attributes: @phones)

  assigned_contact = assigns(:contact)
  >> expect(assigned_contact).to_not be_nil <<
  expect(response).to redirect_to contact_path(assigned_contact)
end

This test will fail in the correct way when @contact isn't assigned and also expresses the intent better.

A related question is whether expecting @contact to be not nil is good enough. Do we not want to test that it's assigned the correct contact and not just any contact? For example with a test like this:

it "assigns the new contact to @contact" do
  contact_attrs = attributes_for(:contact)
  post :create, contact: contact_attrs

  assigned_contact = assigns(:contact)
  expect(assigned_contact).to_not be_nil 
  expect(assigned_contact.attributes).to include(contact_attrs.stringify_keys)
end

Note that this doesn't test for correctness of phones - I couldn't figure out how to do that.

Book version: 2013-10-29 (PDF)

The spec on page 59 (and the corresponding source code) is missing a # (e.g. compared to the original spec on page 50):

describe 'DELETE destroy' do

-->

describe 'DELETE #destroy' do

Book version: 2013-10-29 (PDF)

The spec on page 59 is different from the one originally outlined on page 50. For example the one on page 50 has a test redirects to users#index but the one on page 57 doesn't. Instead it has a redirects to contacts#index test. Is this intentional?

Book version: 2013-10-29 (PDF)

"So it Try Git." should probably be "So is Try Git."

In "Automation with Guard and Spork" section, there is a warning box, says "the version of Spork included in the guard-spork gem published on Rubygems isn't quite ready for Ruby 2.0.0", and gives a link to source code, but the link is broken, and maybe should mention which files are modified.

The Gemfile contains the following outdated gems:

• faker 1.1.2 (newest is 1.2.0)
• database_cleaner 1.0.1 (newest is 1.2.0)
• selenium-webdriver 2.35.1 (newest is 2.37.0)

The lines just below the Gemfile state that:

These are the current versions of each gem as of this writing.

which is not entirely true since the book was updated a few days ago, and some of these gems were updated much longer ago.

I've made it to chapter 4 with the newer versions without any problems so far.

Book version: 2013-10-29 (PDF)

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)

Two issues with the phone_spec.rb example on page 22:

• Why is the phone_type: symbols not coloered blueish as the other symbols (e.g. firstname:, lastname:)?
• In the first test, why is the created home phone stored in the variable home_phone? It's not used in the test.

Book version: 2013-10-29 (PDF)

Page 31, section "How DRY is too DRY"?, third-last line:

Even better, when we get into testing users with specific roles in chapter 6, might be variables like @admin_user and @guest_user.

There's a word or two missing in this sentence. Did you mean "..there might be variables like.."?

Book version: 2013-10-29 (PDF)

In the code example on page 50, the #update test is referred to as PUT #update, but it should really be PATCH #update as mentioned on page 57.

Book version: 2013-10-29 (PDF)

This is more a question than an actual issue (and related to one of my other issues with creating contacts, see #38).

Does the deletes the contact test on page 59 really test that the contact is deleted?

it "deletes the contact" do
  expect{
  delete :destroy, id: @contact
  }.to change(Contact,:count).by(-1)
end

In my opinion this test does nothing but test that some contact is deleted (and thus should really be named deletes some contact). The deleted contact could be any contact, but there's nothing that ensures it's the one with the correct id. Should we not test this? Isn't it a plausible bug to delete a wrong contact?. I know we can't discover ALL bugs with tests, but in this case it seems obvious that the test doesn't test what it says it does and can be improved.

Book version: 2013-10-29 (PDF)

The spec on page 57 is completely different from the one originally outlined on page 50. For example the one on page 50 has a test updates the contact in the database but the one on page 57 doesn't. Instead it has a changes @contact's attributes that the one on page 57 doesn't have. Is this intentional?

Book version: 2013-10-29 (PDF)

This code example on page 67 should just be the previous spec code (from chapter 5) surrounded by a describe block that logs in as admin. However, the spec code illustrated inside the describe block is vastly different from the one in chapter 5 and thus quite confusing. Just to mention a few differences:

• This code uses a @contact variable in all tests instead of the local variables in chapter 5
• Some of the test names don't match (e.g. the ch.5 spec has populates an array of contacts starting with the letter but the equivalent test in this code seems to be populates an array of contacts with a different body)

Book version: 2013-10-29 (PDF)

This is more a question than an actual issue.

Regarding the spec on page 53, what about the requirement that contacts should be returned in sorted (ascending) order according to the last name? There's no spec to check this in the controller spec. Should there be? The contact model spec covers this requirement, but should it also be tested in the controller? Why/why not?

In general it seems the book doesn't address this issue at all - e.g. which features should we test at which layers? I'm only at chapter 6 so it may do so later, but if not, consider adding a note about this.

Book version: 2013-10-29 (PDF)

The contacts.rb and phones.rb examples on page 41 and 42 both have the require 'faker' statement. I accidentally left it out in my Phone factory, but it seems everything works fine without it. I have no idea why, but I have now also removed it from the Contact factory and everything seems to work fine - less is more, right :-)

Book version: 2013-10-29 (PDF)

In the Introduction is this bullet:

• RSpec 2.14: I began writing this book while version 2.8 was current, so anything you see here should apply to versions of RSpec at least that far back.

Since 2.8 is > 2.14, I assume this is meant to say 1.8?

It seems the book has no consistency in its use of ' vs " for strings. Sometimes ' is used in describe blocks, sometimes " is used, sometimes ' is used in context blocks and sometimes not. I feel it would be more professional and less confusing to chose a convention and stick to it.

Book version: 2013-10-29 (PDF)

On page 42, line 1:

I could keep using sequences and my specs would still pass. But does give us a bit more realistic..

One or more words are missing somewhere around "But".

Book version: 2013-10-29 (PDF)

On page 39, line 5 we have the following:

So far, if we wanted to specify a non-home phone in a spec we've done it like this:[code example follows]

There are two issues here:

• The example following this sentence is an updated version of the duplicate contact numbers test which uses FactoryGirl. However, the sentence refers to the fact that we have been using FactoryGirl all along which is confusing. Perhaps the problem lies in the fact that present tense is used; "we've done it like this"., which we of course haven't since this is a newly introduced example. A possible solution may be to use past tense, e.g. "we would have done it like this"
• The sentence talks about specifying non-home phone numbers and provides the following code as example, but the example code doesn't specify non-home phone numbers.

To sum up: rephrase the sentence to focus on the fact that IF we wanted to use different phone number types, THEN it would look like the example. Also, use an example that actually illustrates the point by creating two different phone number types.

Book version: 2013-10-29 (PDF)

我又新写了一章,通过为演示程序添加新功能,延时了 TDD 流程

应该是

我又新写了一章,通过为演示程序添加新功能,演示了 TDD 流程

Page 57, contacts_controller_spec.rb:

it located the requested @contact

-->

it locates the requested @contact

Book version: 2013-10-29 (PDF)

In the pdf version of the book, on page 43, there's a sentence starting with:

But does give us a bit more realistic data...

Shouldn't it be as such?:

But this does give us a bit more realistic data...

ffaker author claims it is orders of magnitude faster then the original faker. Maybe use it in the book as well?

Not a real issue though, just a suggestion.

Just a small issue, but there's this in chapter 3. Model specs

"We’ll expand this outline in a few minutes, but this gives us quite a bit for starters. It’s a simple spec for an admittedly simple model, but points to our first three best practices:"

Then you list four best practices.

If i happen to find anything else i'll let you know.
Thanks for the good work so far!

The contact.rb example on page 43 (bottom) is a bit confusing.

According to the book we can "just add" the validation in the example to the contact model, and everything will work. However this was not the case for me. When I added the validation rule the filter last name by letter test failed. This test isn't mentioned anywhere in the chapter, and thus hasn't been updated to use FactoryGirl unlike all the other tests. I was a bit confused by this, but in the end I converted it to FactoryGirl myself. Perhaps it would be a good idea to have the converted version in the book with the other tests to avoid any confusion.

In relation to this issue I looked in the source code on branch 04_factories and discovered the following FactoryGirl version of the test:

describe "filter last name by letter" do
    before :each do
      @smith = create(:contact,
        lastname: 'Smith', email: '[email protected]')
      @jones = create(:contact,
        lastname: 'Jones', email: '[email protected]')
      @johnson = create(:contact,
        lastname: 'Johnson', email: '[email protected]')
    end
    ....
end

This was different from my own test in that I didn't manually specify email adresses for each contact. Is this necessary? In my optics this test doesn't care about email addresses and will be happy with those randomly generated by Faker & FactoryGirl.

Book version: 2013-10-29 (PDF)

On page 29 in the code example contact_spec.rb the spec constructed through the chapter is shown in its full. But, where did the "is invalid without email address" spec body come from? It's block is included in the skeleton created in the beginning of the chapter (page 17), but it isn't mentioned anywhere until here where the full body appears. All the other specs are mentioned and the reader is guided through writing them in the chapter. At least there should be a note about leaving this spec to the reader as an exercise if that's the intention.

Book version: 2013-10-29 (PDF)

There is a mistake in the following code. Where you said "message", maybe you wanted to say "contact".

it "redirects to the home page upon save" do
post :create, message: FactoryGirl.attributes_for(:contact) expect(response).to redirect_to root_url
end

Page 59, second-last line:

...hiding contacts from view without deleted them (I'll..

-->

...hiding contacts from view without deleting them (I'll..

Book version: 2013-10-29 (PDF)

Should this be Rails 4 in the updated version?

"In Everyday Rails Testing with RSpec I’ll walk you through taking a basic Rails 3.2"

In new version of book, chapter 5, the spec example names in page 49 and the names given with implementation in following pages are different...

In line 3 of the code in this section, expect(attribute).to should instead read expect(actual).to, since you're testing what actually happened instead of an expected attribute (which isn't even passed in in this example).

It becomes clearer if you look at RSpec Wiki's sample code that you link to at the end of the section: https://github.com/dchelimsky/rspec/wiki/Custom-Matchers They use expected instead of attribute, which makes the distinction a little clearer.

Thanks for a really great book!

The PUT #update spec on page 50 has a minor inconsistency in naming:

describe 'PUT #update' do
  context "with valid attributes" do
    it "updates the contact in the database"
    it "redirects to the contact"
  end

  context "with invalid attributes" do
    it "does not update the contact"
    it "re-renders the #edit template"
  end
end

In the first spec it says updates the contact in the database while the second spec just says does not update the contact but fails to specify where the update should not occur (in the database). This is a very minor thing, really.

Book version: 2013-10-29 (PDF)

In the first test in phone_spec.rb on page 40 the created home phone is stored in the variable home_phone, but the variable is not used in the test.

Book version: 2013-10-29 (PDF)

Some code examples use nicely aligned method arguments (e.g. page 20) while others have no alignment at all (e.g. page. 22, 24, 25). The aligned versions are much easier to read.
Consider aligning all examples in the book to both be consistent and clear.

Book version: 2013-10-29 (PDF)

The spec on page 70 has no space between the HTTP method and the controller action, e.g. GET#index instead of GET #index (which the previous part of the book has used consistently).

Book version: 2013-10-29 (PDF)

In the contact_spec.rb code example on page 38 the has a valid factory test suddenly uses create instead of build compared to the not-so-lean test on page 36. I don't believe create is necessary here.

Book version: 2013-10-29 (PDF)

In the following code, where you said "Message", maybe you wanted to say "Contact".

expect(Message.to_csv).to match /Aaron Sumner,[email protected]/

In the chapter 5, there are other places where I guess you exchanged "contact" for "message", but these other mistakes do not compromise the understanding of the text.

This is a great book, thank you!

On page 62 the to_csv method takes an argument:

format.csv do
  send_data Contact.to_csv(@contacts),
  type: 'text/csv; charset=iso-8859-1; header=present',  
  disposition: 'attachment; filename=contacts.csv'
end

but on page 63 it doesn't:

expect(Contact.to_csv).to match /Aaron Sumner,[email protected]/

How does the method know which contact to convert to csv here? If it just converts ALL contacts without arguments, then why is an argument given on page 62? Maybe add a note about how this works to clarify.

Book version: 2013-10-29 (PDF)

In the test populates an array of contacts starting with the letter on page 53, the Jones contact is assigned to an unused jones variable:

it "populates an array of contacts starting with the letter" do
  smith = create(:contact, lastname: 'Smith')
  jones = create(:contact, lastname: 'Jones')
  get :index, letter: 'S'
  expect(assigns(:contacts)).to match_array([smith])
end

The assignment is unnecessary.

Furthermore, I suggest reversing the order of the creates. As it is now the test would pass if the production code always returns a one-element array with the first contact in the database (a plausible mistake), because the test first creates smith and then tests that [smith] is returned. Instead, by first creating jones and checking that [smith] is returned, you require the production code to at least do something intelligent because it needs to pass over jones and only return smith.

To be honest an even better test would create four contacts and check that the two in the middle are returned. That would cover more edge cases than the current one and be more representative of the desired behavior.

Book version: 2013-10-29 (PDF)

Hi @ruralocity, I found one issue when translating Chapter 7.

P83, exercise 2:

expect(@message.reload.is_inappropriate?).to be_true

should be

expect(@contact.reload.hidden?).to be_true

On page 42, section Advanced associations, second-last line:

..contact factory uses the after callback make sure that a new contact.."

A to is missing between callback and make.

Book version: 2013-10-29 (PDF)

Filename extension should be shared_db_connection.rb instead of shared_db_connection.db. Great book!

The Gemfile in the 02_setup branch of the code uses version 2.33.0 of selenium-webdriver, but the corresponding code in the book (page 9) uses version 2.35.1.
Both versions are outdated (see my earlier issue report), but they should at least be consistent.

Book version: 2013-10-29 (PDF)

Is there a difference between a template and a view? If not then only one of them should be used for consistency, if yes, then maybe add a note about it in the book to avoid confusion. It seems like the two are used interchangeably, e.g on page 49 in contacts_controller_spec.rb the following test says 'renders the :index view':

context 'with params[:letter]' do
  it "populates an array of contacts starting with the letter"
  it "renders the :index view"
end

but a couple of lines later it says 'renders the :show template':

describe 'GET #show' do
  it "assigns the requested contact to @contact"
  it "renders the :show template"
end

This is more a question than an actual issue.

In the spec on page 55, does the saves the new contact in the database test really test that the new contact is saved?:

it "saves the new contact in the database" do
  expect{
  post :create, contact: attributes_for(:contact, phones_attributes: @phones)
  }.to change(Contact, :count).by(1)
end

In my opinion this test does nothing but test that some contact is saved (and thus should really be named saves some new contact in the database). The saved contact could be any valid contact, but there's nothing that checks the saved contact is actually the one submitted. Should we not test this?

Book version: 2013-10-29 (PDF)