Refactor Strategies controller spec to strategy request spec #1859
Conversation
This commit refactors the first context of testing for the #index action within the strategies_spec file
There was a problem hiding this comment.
Overall looks good to me 🤔 pretty long spec lol
One thing I personally like to use is subject
https://www.betterspecs.org/#subject
What are your opinions on it?
|
👋 @tyler-wel thanks for reviewing! 😄 Yeah, I did not realize how long of a spec I was signing up to refactor 😆 In regards to |
Yaa, I did the same thing at first before switching which spec I assigned myself to 😂 I personally used |
spec/requests/strategies_spec.rb
Outdated
| it 'assigns @strategies' do | ||
| get strategies_path, params: { search: 'test' } | ||
| expect(assigns(:strategies)).to eq([strategy]) |
There was a problem hiding this comment.
I'm not 100% sold on assigns matcher for request specs, but I guess it really is no problem, because there are other render/data expectations in this context 🤔
I've always believed it's best to just test the input and output for each possible case. But I guess it isn't hurting anyone still being there 🤷 lol ... especially if we're not rendering json... I guess that's one valid way to check that the data was actually assigned...hmmmm TIL, actually kinda useful huh
There was a problem hiding this comment.
As I mentioned to someone else on Slack, one way to check is check the actual response body (in the case of json) and confirm the returned items are what's expected.
expect(JSON.parse(response.body['some_id']).to eq(expected_value)For html expectations I'm a little more rusty, as most of my experience is with JSON apis
There was a problem hiding this comment.
I used the strategy in that article in which I expect an attribute to be included in the body of the response.
There was a problem hiding this comment.
Thanks for taking this on! :D Great work!
Everything looks good to me but I'm curious whether the tests for "assigns @strategies" can be tested without assigns / is truly necessary.
| let(:user) { create(:user) } | ||
| let(:strategy) { create(:strategy, user: user) } | ||
|
|
||
| describe '#index' do | ||
| let(:strategy) { create(:strategy, name: 'test', user: user) } | ||
|
|
||
| context 'when the user is logged in' do | ||
| include_context :logged_in_user | ||
| before { sign_in user} |
spec/requests/strategies_spec.rb
Outdated
| it 'assigns @strategies' do | ||
| get strategies_path, params: { search: 'test' } | ||
| expect(assigns(:strategies)).to eq([strategy]) |
|
I will get the |
andrew-schutt
force-pushed
the
refactor-strategy-spec
branch
from
9048285
to
c0dfdde
Compare
Description
This refactors the tests associated with the
StrategiesControllerto use RSpecrequesttype tests instead ofcontrollerMore Details
Couple things of note:
include_context :logged_in_user->before { sign_in user }is usedbeforeblocks but could bring them back (I prefer explicitness 😸 ):focuson a test for running only the test focused.taggedaboveprint_remindersastaggedis a route in the controller vs anincludein the controllerCorresponding Issue
#1815 <- parent issue
#1840 <- directly related to this refactor
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!