Refactor: class-based view for Core Index - #3063
Conversation
github-actions
Bot
added
tests
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
| assert view.template_name == "core/index.html" | ||
|
|
||
| def test_get(self, view, app_request, mocked_session_reset): | ||
| response = view.get(app_request) |
There was a problem hiding this comment.
This seems perfectly fine for this test case, but as a note...
We found in other cases, like where you have view mixins, it is more appropriate (necessary even) to call view.dispatch() in a test, and then assert that your intended method was called (or not, depending on e.g. the mixin behavior). This is also more similar to how the framework calls into CBVs too.
Here's an example: https://github.com/cal-itp/benefits/blob/main/tests/pytest/eligibility/test_views.py#L355
In that case, calling view.post() directly would completely skip the *SessionRequiredMixins and thus e.g. self.flow in the view would not be set.
There was a problem hiding this comment.
Yeah, I noticed that we were doing that with view.dispatch() in tests for other CBVs. I think in a dev workshop discussion, we said the test should call the actual method we're implementing so that it is as close to a unit test as possible. It makes sense why the mixin doesn't get called if we do that though (the whole MRO thing).
What do you think about the approach taken in https://github.com/cal-itp/benefits/blob/main/tests/pytest/enrollment_switchio/test_views.py#L39-L40, where the variables that would be set by the mixins are set up in the fixture? I'm fine with doing it whichever way you think is best.
There was a problem hiding this comment.
I'll go ahead and merge this PR since the issue doesn't apply here! Thanks for noting this so that we can get alignment on it.
There was a problem hiding this comment.
I think in a dev workshop discussion, we said the test should call the actual method we're implementing so that it is as close to a unit test as possible.
I recall this conversation as well, and yeah I realize what I just said above goes the other way...
What do you think about the approach taken in...
This could work too. I guess when I was working on the Eligibility CBVs, I was loathe to mock more things and just wanted to run my damn logic 😅 We can discuss in another workshop!
There was a problem hiding this comment.
Just putting this follow-up here for posterity.
We've spoken again about this in some various calls, and have implemented a few more CBVs, and this approach seems better overall. That is:
- Initialize the CBV attributes like
agencyorflowas part of fixture setup - Test the actual CBV function:
get(),post(), etc.
2 participants
Closes #2996
Reviewing
Index page loads as normal, and session is reset.