Controller Tests#970
Conversation
paroga
left a comment
paroga
left a comment
There was a problem hiding this comment.
first: once again many thanks for the great work!
i just skimmed the changes and didn't look into detail for now. most of comments apply multiple times, but i didn't mark every single occurrence
the rubucop checks for it are not enforced, since we have a lot of existing code, but it would be great to stick to the Ruby Style Guide for string literals
i know that's it's a hard to split this PR, when all parts would require the Gemfile changes, but it would make the review process easier if the PR would be smaller. maybe start with a single controller test?
| def cancel_membership | ||
| if params[:membership_id] | ||
| membership = @current_user.memberships.find!(params[:membership_id]) | ||
| membership = @current_user.memberships.find(params[:membership_id]) |
There was a problem hiding this comment.
why is it ok to remove the ! here and keep it in line 69?
There was a problem hiding this comment.
i think a find! method does not exists?
NoMethodError:
undefined method `find!' for #<ActiveRecord::Associations::CollectionProxy []>
Did you mean? find
find_by! returns a ActiveRecord::RecordNotFound instead of nil compared to find_by
I added another test to check that find also returns a ActiveRecord::RecordNotFound Exception
| before do | ||
| order_article | ||
| end |
There was a problem hiding this comment.
i'd suggest to add a second order at line 15 instead. IMHO there should be no need for a dedicated order_article factory.
| login user | ||
| routes.draw { get "deny_access" => "dummy_auth#call_deny_access" } | ||
| get :call_deny_access, params: { foodcoop: FoodsoftConfig[:default_scope] } | ||
| expect(response).to redirect_to(root_url) |
There was a problem hiding this comment.
when do you use _path and when _url?
There was a problem hiding this comment.
in that specific case the the controller redirects to root_url ... in general not sure, is there a convention?
yksflip
force-pushed
the
8_increase_test_coverage_controllers
branch
from
ad0a138 to
e34bf55
Compare
|
thanks for your feedback! We'll look into your comments and I'll change the scope of the PR to fit just the Gemfile and one ControllerSpec at first. |
yksflip
force-pushed
the
8_increase_test_coverage_controllers
branch
3 times, most recently
from
433f74d to
78936c3
Compare
yksflip
force-pushed
the
8_increase_test_coverage_controllers
branch
from
e5870b5 to
d7591d4
Compare
Co-authored-by: viehlieb <pf@pragma-shift.net> Co-authored-by: Tobias Kneuker <tk@pragma-shift.net>
Co-authored-by: viehlieb <pf@pragma-shift.net> Co-authored-by: Tobias Kneuker <tk@pragma-shift.net> seperate expects refactor login user calls add more articles to test sorting with fix: fix test for rails upgrade
yksflip
force-pushed
the
8_increase_test_coverage_controllers
branch
from
d7591d4 to
d3d7acc
Compare
3 participants
Keeping this atm to track upcoming smaller PR's
We found that the
articles_controllersync method throws a DoubleRenderError, should we open a new Issue/PR for that?see spec/controllers/articles_controller_spec.rb:179
Edit: split into seperate PR's