Add factoryboy Project and Allocation unit tests #546
Conversation
…o AllocationCreateView post request
| class Meta: | ||
| model = ProjectUserStatusChoice | ||
| django_get_or_create = ('name',) | ||
| name = 'Active' |
There was a problem hiding this comment.
Here the "name" value is hard-coded instead of taken from a list of possible values because it's the value that's most useful during testing. A different approach was used in ProjectStatusChoiceFactory (line 117), where possible choices were randomly assigned.
| from factory import SubFactory | ||
| ### Default values and Faker provider setup ### | ||
|
|
||
| project_status_choice_names = ['New', 'Active', 'Archived'] |
There was a problem hiding this comment.
The hardcoding of the default values here repeats the default values that are listed in project.utils.add_project_status_choices and project.management.commands.add_default_project_choices. It seems like it could be useful to encode default values for model objects in one location in an app-level config.py file or something similar - happy to open an issue for this and then make the change in this or another PR if it sounds good.
There was a problem hiding this comment.
I love this idea @claire-peters! I would definitely recommend we do this in another PR as long as @dsajdak and @aebruno agree as well.
There was a problem hiding this comment.
These look great, @claire-peters! I just left some small comments on some files pertaining to issue #541 about whether or not those were meant to go in this testing PR, or if they were supposed to go in a separate PR. Also, the changes to user/tests.py didn't seem to add any testing, so we could delete that unless the imports were placed there for a reason.
coldfront/core/user/tests.py
Outdated
There was a problem hiding this comment.
@claire-peters I was looking through the testing you added and it looks great! On this file (user/tests.py), I'm not seeing anything besides unused imports and some newlines that were added so I think we can safely discard changes to this file!
There was a problem hiding this comment.
If I'm understanding correctly, the changes on this file and templates/allocation/allocation_request_list.html appear to be addressing issue #541 rather than testing. Not sure if this was meant to be combined with this PR @claire-peters but just wanted to check!
There was a problem hiding this comment.
If I'm understanding correctly, the changes on this file and templates/allocation/allocation_change_list.html appear to be addressing issue #541 rather than testing. Not sure if this was meant to be combined with this PR @claire-peters but just wanted to check!
coldfront/core/allocation/views.py
Outdated
There was a problem hiding this comment.
If I'm understanding correctly, the changes on this file and the allocation templates above appear to be addressing issue #541 rather than testing. Not sure if this was meant to be combined with this PR @claire-peters but just wanted to check!
| from factory import SubFactory | ||
| ### Default values and Faker provider setup ### | ||
|
|
||
| project_status_choice_names = ['New', 'Active', 'Archived'] |
There was a problem hiding this comment.
I love this idea @claire-peters! I would definitely recommend we do this in another PR as long as @dsajdak and @aebruno agree as well.
| class Meta: | ||
| model = ProjectUserStatusChoice | ||
| django_get_or_create = ('name',) | ||
| name = 'Active' |
|
Thanks for catching the issues there - the parts for #541 should be in a separate PR, and I'm happy to put off any formatting changes to the user tests module until it can be paired with a more substantive contribution to said module. Just pushed my changes, let me know what you think. |
There was a problem hiding this comment.
@claire-peters thanks so much for this PR. All looks great. Should all the tests be passing? When I just try running coldfront test I'm getting 3 failures:
1. FAIL: test_allocationchangelistview_access (coldfront.core.allocation.test_views.AllocationChangeListViewTest)
AssertionError: '/?next=%2Fallocation%2Fchange-list' != '/user/login?next=%2Fallocation%2Fchange-list'
- /?next=%2Fallocation%2Fchange-list
+ /user/login?next=%2Fallocation%2Fchange-list
? ++++++++++
: Response redirected to '/?next=/allocation/change-list', expected '/user/login?next=/allocation/change-list'Expected '/?next=/allocation/change-list' to equal '/user/login?next=/allocation/change-list'.
2. FAIL: test_allocationcreateview_post (coldfront.core.allocation.test_views.AllocationCreateViewTest)
Test POST to the AllocationCreateView
----------------------------------------
AssertionError: False is not true : Couldn't find 'Allocation requested.' in response
3. FAIL: test_allocationcreateview_post_zeroquantity (coldfront.core.allocation.test_views.AllocationCreateViewTest)
Test POST to the AllocationCreateView
AssertionError: False is not true : Couldn't find 'Allocation requested.' in response
If these are expected failures I'm happy to merge. If not, perhaps we can get those passing?
…teView, remove login redirect for AllocationChangeListView
|
Whup, thanks for catching that. Added the changes to make the tests work - specifically, I made it so a success message would appear on the following page after an allocation request has been successfully submitted and I removed the line |
This PR includes:
There are also a couple of minor changes made to the Allocation views:
Looking forward to your comments!