Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve test coverage for Activities utilities #1050

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Improve test coverage for Activities utilities #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f46ab4
Write first two tests
Chiemezuo Dec 19, 2023
a5bb98c
write 3 more tests
Chiemezuo Dec 21, 2023
e055d2f
Merge branch 'unstructuredstudio:master' into activities-utils-tests
Chiemezuo Dec 21, 2023
d5915b6
Merge branch 'master' of https://github.com/unstructuredstudio/zubhub…
Chiemezuo Dec 21, 2023
9c861e3
finish up tests for utils
Chiemezuo Dec 21, 2023
1fceb27
Merge branch 'activities-utils-tests' of https://github.com/Chiemezuo…
Chiemezuo Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions zubhub_backend/zubhub/activities/tests.py
View file
Open in desktop

This file was deleted.

Empty file added zubhub_backend/zubhub/activities/tests/__init__.py
View file
Open in desktop
Empty file.
223 changes: 223 additions & 0 deletions zubhub_backend/zubhub/activities/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
from django.test import TestCase
from activities.models import * #import all the models
from activities.utils import * #import all the utilities

class TestUtils(TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def setUp(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function name in python are preferable when in snake casing and not Camel Casing or lower camel casing like in your case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part, yes. However, note this example in the official Django documentation website.

# sample image url from the internet
self.test_image_data = {
'file_url': 'https://picsum.photos/300',
'public_id': 'A sample image from the loren picsum website',
}

# create image instance
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

self.test_image = Image.objects.create(**self.test_image_data)

# create activity
self.test_activity = Activity.objects.create(
title='A dummy activity',
introduction='This is the activity for John/Jane Doe',
video='https://www.youtube.com/watch?v=CmzKQ3PSrow',
slug='a-dummy-activity',
# the other fields are not compulsory
)

def test_create_inspiring_artist(self):
# get a dictionary out of the created image instance
image_map = self.test_image.__dict__
# remove data that doesn't need to be passed to artist
image_map.pop('_state', None)
image_map.pop('id', None)

# create artist data
inspiring_artist_data = {
'name': 'John Doe',
'short_biography': 'A short biography about John Doe',
'image': image_map
}

# call the function being tested
artist = create_inspiring_artist(inspiring_artist_data)

self.assertIsInstance(artist, InspiringArtist)
self.assertEqual(artist.name, inspiring_artist_data['name'])
self.assertNotEqual(artist.name, 'Jane Doe')
self.assertEqual(artist.image, inspiring_artist_data['image'])
self.assertTrue(InspiringArtist.objects.filter(name='John Doe').exists())


def test_update_image(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

new_data = {
'file_url': 'https://picsum.photos/400', # notice the 400 instead of 300
'public_id': self.test_image_data['public_id'],
}

new_image = update_image(self.test_image, new_data)

self.assertIsInstance(new_image, Image)
db_query = Image.objects.get(public_id=new_data['public_id'])
# assert that the changes have reflected in the database
self.assertEqual(db_query.file_url, new_data['file_url'])
self.assertNotEqual(new_image.file_url, self.test_image_data['file_url'])

def test_create_activity_images(self):
images = [
{
'image': {
'file_url': 'https://picsum.photos/500',
'public_id': 'Activity image 1',
}
},
{
'image': {
'file_url': 'https://picsum.photos/600',
'public_id': 'Activity image 2',
}
}
]

# get activity images count & images count before function
activity_image_count_before = ActivityImage.objects.filter(activity=self.test_activity).count()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prev_activity_image_count

image_count_before = Image.objects.all().count()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prev_image_count

# create activity images
create_activity_images(activity=self.test_activity, images=images)
# get activity images count & images count after function
activity_image_count_after = ActivityImage.objects.filter(activity=self.test_activity).count()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activity_image_count

image_count_after = Image.objects.all().count()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image_count

# assert that images and activity images were created
self.assertEqual(activity_image_count_after - activity_image_count_before, len(images))
self.assertEqual(image_count_after - image_count_before, len(images))

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',}]
},
]

Comment on lines +91 to +112
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When performing tests we need to have the images and other items created as instances of our models at runtime.

# get counts before calling function
activity_steps_count_before = ActivityMakingStep.objects.all().count()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This is a much more descriptive term. The change will reflect in my next PR

image_count_before = Image.objects.all().count()
# call function to be tested
create_making_steps(self.test_activity, making_steps)
# get counts after function call
activity_steps_count_after = ActivityMakingStep.objects.all().count()
image_count_after = Image.objects.all().count()
self.assertEquals(activity_steps_count_after - activity_steps_count_before, len(making_steps))
# 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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might also want to test the image field of Inspiring example 2 InspiringExample model

inspiring_examples_data = [
{
'description': 'Inspiring example 1',
'credit': 'credit',
'image': {
'file_url': 'https://picsum.photos/300',
'public_id': 'A sample image from the loren picsum website',
}
},
{
'description': 'Inspiring example 2',
'credit': 'credit',
# no image in this inspiring example
},
]

# get number of examples and images before function call
examples_count_before = InspiringExample.objects.filter(activity=self.test_activity).count()
images_count_before = Image.objects.all().count()
# function call
create_inspiring_examples(self.test_activity, inspiring_examples=inspiring_examples_data)
# get number of examples and images after function call
examples_count_after = InspiringExample.objects.filter(activity=self.test_activity).count()
images_count_after = Image.objects.all().count()
# Assert new InspiringExample instances were created
self.assertGreater(examples_count_after, examples_count_before)
# Assert only one image was created
self.assertEqual(images_count_after - images_count_before, 1)

def test_update_activity_image(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

images = [
{
'image': {
'file_url': 'https://picsum.photos/500',
'public_id': 'Activity image 1',
}
},
{
'image': {
'file_url': 'https://picsum.photos/600',
'public_id': 'Activity image 2',
}
}
]

update_activity_images(self.test_activity, images_to_save=images)
# get count of activity images after function call
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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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',}]
},
]

update_making_steps(self.test_activity, making_steps)
# get counts after function call
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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

inspiring_examples_data = [
{
'description': 'Inspiring example 1',
'credit': 'credit',
'image': {
'file_url': 'https://picsum.photos/300',
'public_id': 'A sample image from the loren picsum website',
}
},
{
'description': 'Inspiring example 2',
'credit': 'credit',
# no image in this inspiring example
},
]

update_inspiring_examples(self.test_activity, inspiring_examples_data)
# get counts after function call
examples_count_after = InspiringExample.objects.filter(activity=self.test_activity).count()
self.assertEqual(examples_count_after, len(inspiring_examples_data))