Spec coverage on Settings/ controllers specs #24221
Conversation
mjankowski
force-pushed
the
settings-controllers-specs
branch
3 times, most recently
from
33eb31d to
e905aeb
Compare
There was a problem hiding this comment.
In most cases, the tests you added only check for the HTTP response code. While that's still an improvement given these methods aren't currently tested at all, that makes the test descriptions misleading, whereas similar test descriptions read something like “returns http success” in our other tests.
I'd like that either the tests get changed to actually test what they claim, or their description be changed to reflect what they are actually testing.
| it 'removes an alias' do | ||
| delete :destroy, params: { id: account_alias.id } | ||
|
|
||
| expect(response).to redirect_to(settings_aliases_path) |
There was a problem hiding this comment.
I think it should also check that the record actually gets deleted.
| it 'creates an alias for the user' do | ||
| post :create, params: { account_alias: { acct: '[email protected]' } } | ||
|
|
||
| expect(response).to redirect_to(settings_aliases_path) |
There was a problem hiding this comment.
I think it should also check that a record gets actually created.
mjankowski
force-pushed
the
settings-controllers-specs
branch
from
e905aeb to
4a19d6d
Compare
|
Pushed update with a few spec changes for better creation/deletion checks, and also some wording changes to make some of the descriptions less inaccurate. You are correct that my current goal here is broad coverage and just getting all the lines executing in the spec. I'll continue to go deeper on actual behavior specs once the broad coverage is solid. |
spec/controllers/settings/migration/redirects_controller_spec.rb
Outdated
Show resolved
Hide resolved
Adds more coverage to the /settings/ controllers.
Does not quite get to 100% on this group ... but pretty close. Mostly failure edge cases left to do, but I think the full happy path is covered after these changes.