Improve test coverage for Activities utilities #1050
Conversation
…into activities-utils-tests
…/zubhub into activities-utils-tests
| 'public_id': 'A sample image from the loren picsum website', | ||
| } | ||
|
|
||
| # create image instance |
There was a problem hiding this comment.
I don't think we should attempt to create the image instance ourselves. I think that is part of the things that create_inspiring_artist is expected to do if you take a closer look at the function. I think it should be enough to pass inspiring_artist_data to create_inspiring_artist function (with the image field being set to self.test_image_data)
| from activities.models import * #import all the models | ||
| from activities.utils import * #import all the utilities | ||
|
|
||
| class TestUtils(TestCase): |
There was a problem hiding this comment.
this class should not be named TestUtils since that suggests that all the functions in the utils.py file are being tested under this TestUtils class. Although that is the approach you followed, it is wrong. What we should do instead is to have a different test class for each function in the utils.py file. for example class TestCreateInspiringArtist, class TestUpdateImage, etc. This is because for example for update_image we'd probably need to test more than one condition like when image is truthy and image_data is truthy, image is truthy and image_data is falsy, image is falsy and image_data is truthy, image is falsy and image_data is falsy
There was a problem hiding this comment.
I truly appreciate your feedback on this. I understand the idea behind what you've said. Earlier on, I also considered doing this, but a slight problem with this approach is that every class will need to have its own setup (to avoid dependence on another class). This would lead to a significant amount of repetition, and it would also be harder to maintain in the long run: for example, if something is changed about how activities or images are created in the utils.py, every test class that has an activity or image in its setup would have to be modified accordingly.
Still following your approach though, would it be better if I grouped into two classes: class TestActivityOperations and TestImageOperations? It would allow shared setups.
This would also prevent making classes out of minor util functions
| self.assertTrue(InspiringArtist.objects.filter(name='John Doe').exists()) | ||
|
|
||
|
|
||
| def test_update_image(self): |
There was a problem hiding this comment.
here you are testing only for one condition, when image exists (truthy) and image_data is also provided and has necessary values (truthy). See my comment about class TestUtils.
Note that when I say that a variable is truthy, it means that evaluating that variable with an if conditional will result in true, and verse versa
| ] | ||
|
|
||
| # get activity images count & images count before function | ||
| activity_image_count_before = ActivityImage.objects.filter(activity=self.test_activity).count() |
There was a problem hiding this comment.
prev_activity_image_count
|
|
||
| # get activity images count & images count before function | ||
| activity_image_count_before = ActivityImage.objects.filter(activity=self.test_activity).count() | ||
| image_count_before = Image.objects.all().count() |
| ] | ||
|
|
||
| # get counts before calling function | ||
| activity_steps_count_before = ActivityMakingStep.objects.all().count() |
There was a problem hiding this comment.
you can also use old_acitivity_steps_count and new_activity_steps_count instead of activity_steps_count_before and activity_steps_count_after. old and new conveys the information better
There was a problem hiding this comment.
I agree. This is a much more descriptive term. The change will reflect in my next PR
| # check that there are now more images than there were before | ||
| self.assertGreater(image_count_after, image_count_before) | ||
|
|
||
| def test_create_inspiring_example(self): |
There was a problem hiding this comment.
you might also want to test the image field of Inspiring example 2 InspiringExample model
| # Assert only one image was created | ||
| self.assertEqual(images_count_after - images_count_before, 1) | ||
|
|
||
| def test_update_activity_image(self): |
There was a problem hiding this comment.
here you need to create some initial ActivityImages since we are testing an update operation.
We also need to mock out create_activity_images (this is to make sure that update_activity_image test can't be affected when change is made to create_activity_images. You can read more about mocking online).
Now after running update_activity_image, we check that the old ActivityImages that we manually created earlier no longer exists,
and check that the contents of the images array were passed to our mock.
| activity_images_count_after = ActivityImage.objects.filter(activity=self.test_activity).count() | ||
| self.assertEqual(activity_images_count_after, len(images)) | ||
|
|
||
| def test_update_making_steps(self): |
There was a problem hiding this comment.
here you need to create some initial ActivityMakingStep manually since we are testing an update operation.
We also need to mock out create_making_steps (this is to make sure that update_making_steps test can't be affected when change is made to create_making_steps. You can read more about mocking online).
Now after running update_making_steps, we check that the old ActivityMakingStep that we manually created earlier no longer exists,
and that the contents of making_steps array were passed to our mock
| activity_steps_count_after = ActivityMakingStep.objects.filter(activity=self.test_activity).count() | ||
| self.assertEqual(activity_steps_count_after, len(making_steps)) | ||
|
|
||
| def test_update_inspiring_examples(self): |
There was a problem hiding this comment.
here you need to create some initial InspiringExample manually since we are testing an update operation.
We also need to mock out create_inspiring_examples (this is to make sure that update_inspiring_examples test can't be affected when change is made to create_inspiring_examples. You can read more about mocking online).
Now after running update_inspiring_examples, we check that the old InspiringExample that we manually created earlier no longer exists,
and that the contents of inspiring_examples_data array were passed to our mock
| from activities.utils import * #import all the utilities | ||
|
|
||
| class TestUtils(TestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
function name in python are preferable when in snake casing and not Camel Casing or lower camel casing like in your case.
There was a problem hiding this comment.
For the most part, yes. However, note this example in the official Django documentation website.
| def test_create_making_steps(self): | ||
| making_steps = [ | ||
| { | ||
| 'title': 'Step 1', | ||
| 'description': 'Description for step 1', | ||
| 'step_order': 1, | ||
| 'image': [{'file_url': 'https://picsum.photos/710','public_id': '1',}] | ||
| }, | ||
| { | ||
| 'title': 'Step 2', | ||
| 'description': 'Description for step 2', | ||
| 'step_order': 2, | ||
| 'image': [{'file_url': 'https://picsum.photos/720','public_id': '2',}] | ||
| }, | ||
| { | ||
| 'title': 'Step 3', | ||
| 'description': 'Description for step 3', | ||
| 'step_order': 3, | ||
| 'image': [{'file_url': 'https://picsum.photos/730','public_id': '3',}] | ||
| }, | ||
| ] | ||
|
|
There was a problem hiding this comment.
When performing tests we need to have the images and other items created as instances of our models at runtime.
Summary
This PR adds tests for all
utils.pyfunctions for theactivitiesapp.It is a step in the direction of improving code coverage across the entire project.
PS: I had to delete the
tests.pyfile at the root of theactivitiesapp for the test folder to be recognized.