Factory girl - #25
Conversation
|
This looks great at a glance @bradgreen3, thanks so much. I'll do a real review soon. So happy to have your interest in the project and would love to chat sometime about what brought you to it. If you're interested, please join us on waffle.io and assign tickets to yourself as you pick them up: https://waffle.io/lbraun/thanksgiving/join |
lbraun
left a comment
lbraun
left a comment
There was a problem hiding this comment.
This is fantastic @bradgreen3, thanks. I hope all my comments aren't overwhelming. I like to do thorough reviews, especially at first, not to have control but just to get on the same page with collaborators. Please let me what is helpful and what isn't.
One minor thing I couldn't add as an inline comment: please remove spec/features/.DS_Store from your PR. You may want to add .DS_Store files to your global gitignore.
Lastly, I would appreciate your help as a new contributor to improve our documentation. Did you find any of it particularly useful when getting started? How could we make it better? Feel free to update it as you go!
Thanks again, and just let me know if you have any questions.
-Lucas
| visit "/" | ||
| click_link "Sign in" | ||
| within(:css, ".super-button") do | ||
| click_link "Sign in or sign up" |
| sequence :name do |n| | ||
| "Fake User#{n}" | ||
| end | ||
| approval_at "2017-02-25 23:28:13" |
There was a problem hiding this comment.
From my experience with Factory Girl, the less you do in the main factory, the better. My rule of thumb is to only define attributes that are required (by a validation, for example). So if needed, we could add an approved_user factory, but I would take this line out here. What do you think?
There was a problem hiding this comment.
That makes perfect sense! Default approval status removed 😄
| FactoryGirl.define do | ||
| factory :user do | ||
| provider "google_oauth2" | ||
| sequence :uid do |n| |
There was a problem hiding this comment.
Could you write this using a more compact single-line block instead?
| context "when an admin visits the users index" do | ||
| let!(:admin) { login_new_admin_user } | ||
| let!(:users) { create_list(:user, 2) } | ||
| let!(:unapproved_user) { create(:user, approval_at: nil)} |
| } | ||
| ) | ||
| it "sees a table with proper headers and rows" do | ||
| expect(page).to have_css("#user_#{admin.id}", :text => "#{admin.name}") |
There was a problem hiding this comment.
Please use newer hash syntax instead of hash rockets:
https://github.com/bbatsov/ruby-style-guide#hash-literals
| @@ -0,0 +1,3 @@ | |||
| FactoryGirl.define do | |||
| # <factories for each model go here> | |||
There was a problem hiding this comment.
I think we can get rid of this file since we are creating separate files for each model, don't you?
| def login_new_user(options = {}) | ||
| visit "/" | ||
| click_link "Sign in" | ||
| within(:css, ".super-button") do |
There was a problem hiding this comment.
Thanks for fixing the specs: I really shouldn't have pushed my last update without running them. Do you practice TDD? I'd like to make it the convention for this project.
CSS is the default selector for Capybara, so you can leave it out in cases like this: within(".super-button") do
There was a problem hiding this comment.
No problem! It was a good way to get familiar with the code base! I do practice TDD, so I'll certainly use it if I add any new features.
Good catch. I'm not sure why I put that in there 😕
| n | ||
| end | ||
| sequence :name do |n| | ||
| "Fake User#{n}" |
| gem 'capybara' | ||
| gem 'launchy' | ||
| gem 'rspec-rails' | ||
| gem 'factory_girl_rails' |
|
|
||
| before { visit users_index_path } | ||
| it "sees a table with proper headers and rows" do | ||
| expect(page).to have_css("#user_#{users.first.id}", :text => "#{users.first.name}") |
There was a problem hiding this comment.
There's no reason to use interpolation when you want to use the whole string ("#{x}" == x)
There was a problem hiding this comment.
Yep, you're absolutely right.
2 participants
About half of the tests were failing, so I fixed them all and got Factory Girl set up in the process.
Closes #18