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.

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

Reminder tests #322

Merged
merged 16 commits into from Feb 3, 2020
Merged

Reminder tests #322

merged 16 commits into from Feb 3, 2020

Conversation

@bendiller
Copy link
Contributor

bendiller commented Jan 13, 2020

I believe this is ready for review. I'm not very familiar with Django (let alone the REST framework module), nor am I very familiar with testing overall, but I tried to model my efforts after the examples already present for other ViewSets.

I'll be happy to make any changes necessary, thanks for your time!

Copy link
Member

MarkKoz left a comment

Needs a test case for the list endpoint to test retrieval of existing objects and also test searching and filtering.

url = reverse('bot:reminder-list', host='api')
response = self.client.post(url, data=self.data)
self.assertEqual(response.status_code, 201)
Comment on lines 64 to 66

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Feb 1, 2020

Member

This should be in a test function instead of in the setup fixture.

This comment has been minimized.

Copy link
@bendiller

bendiller Feb 2, 2020

Author Contributor

I thought that was odd too. I used a couple existing files created by respected contributors as models for mine, and this "assert within setUp()" appeared in a couple places. Would it be appropriate to move this into the test_reminder_in_full_list test? Or is it better to test creation using a post against the list endpoint in one test, and use another test to check that a get for the list endpoint returns expected reminder instances?

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Feb 2, 2020

Member

Put it in a new test function. As per this comment, put that assertion in this new function too and test_reminder_in_full_list can be removed completely.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Feb 2, 2020

Member

Actually, test_reminder_in_full_list may be moved to another test case since retrieval tests are needed. It just doesn't belong with the creation tests at least.

Needs a test case for the list endpoint to test retrieval of existing objects and also test searching and filtering.

pydis_site/apps/api/tests/test_reminders.py Outdated Show resolved Hide resolved
pydis_site/apps/api/tests/test_reminders.py Outdated Show resolved Hide resolved
pydis_site/apps/api/tests/test_reminders.py Outdated Show resolved Hide resolved
@jos-b jos-b requested review from python-discord/core-developers, eivl, fiskenslakt, MarkKoz and dementati and removed request for python-discord/core-developers, eivl and fiskenslakt Feb 2, 2020
@bendiller

This comment has been minimized.

Copy link
Contributor Author

bendiller commented Feb 2, 2020

Needs a test case for the list endpoint to test retrieval of existing objects and also test searching and filtering.

I think I've got most of these issues sorted, but I'm not certain I've done what you asked regarding searching and filtering. I adapted what looked like the requested behavior from test_infractions.test_filter_search(). If what I've done is correct, can you help me understand when this searching is actually used in practice? Is this the mechanism by which the bot lists the reminders for a particular user, when commanded?

I'm going to push what I've got for your further comments - thanks for your feedback and help!

Oh, and because I didn't mention it in the original PR, this addresses issue #244

Copy link
Member

MarkKoz left a comment

If what I've done is correct, can you help me understand when this searching is actually used in practice? Is this the mechanism by which the bot lists the reminders for a particular user, when commanded?

I don't know - I didn't make the viewset. I would have expected search_fields to be specified so it only searches on a subset of the fields. It doesn't seem to have practical use to me in this case considering the viewset also supports filtering by fields. At least for infractions it was useful because reasons could be searched with regex.

pydis_site/apps/api/tests/test_reminders.py Outdated Show resolved Hide resolved
pydis_site/apps/api/tests/test_reminders.py Outdated Show resolved Hide resolved
pydis_site/apps/api/tests/test_reminders.py Outdated Show resolved Hide resolved
@bendiller bendiller requested a review from python-discord/core-developers as a code owner Feb 3, 2020
response = self.client.get(url)

self.assertEqual(response.status_code, 200)
self.assertEqual(response.json(), [self.rem_dict_one, self.rem_dict_two])

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 3, 2020

Member

I think this test is a bit fragile. Formally, the model has no default ordering defined, which makes the order in which the objects are returned "unspecified":

If a query doesn’t have an ordering specified, results are returned from the database in an unspecified order. A particular ordering is guaranteed only when ordering by a set of fields that uniquely identify each object in the results. For example, if a name field isn’t unique, ordering by it won’t guarantee objects with the same name always appear in the same order. (source)

I'm not sure what the best approach is to making these tests robust against ordering issues, as I don't think they're sortable by default. (Otherwise we could sort both lists and compare them after a sort.)

A way around that could be to use TestCase.assertCountEqual which tests if the lists contain the same elements with the same counts regardless of order.

Suggested change
self.assertEqual(response.json(), [self.rem_dict_one, self.rem_dict_two])
self.assertCountEqual(response.json(), [self.rem_dict_one, self.rem_dict_two])

This comment has been minimized.

Copy link
@bendiller

bendiller Feb 3, 2020

Author Contributor

Whoops, I've never really thought too much about list equality comparison in Python. Considering how convenient everything else tends to be, I just assumed it'd be dealt with automagically. TestCase.assertCountEqual seems like a good solution!

response = self.client.get(f'{url}?search={self.author.name}')

self.assertEqual(response.status_code, 200)
self.assertEqual(response.json(), [self.rem_dict_one, self.rem_dict_two])

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Feb 3, 2020

Member

Same as above

Suggested change
self.assertEqual(response.json(), [self.rem_dict_one, self.rem_dict_two])
self.assertCountEqual(response.json(), [self.rem_dict_one, self.rem_dict_two])
@MarkKoz
MarkKoz approved these changes Feb 3, 2020
@MarkKoz MarkKoz merged commit 5b5b417 into master Feb 3, 2020
2 checks passed
2 checks passed
Site Build #20200203.5 succeeded
Details
Site (Test & Lint) Test & Lint succeeded
Details
@MarkKoz MarkKoz deleted the reminder-tests branch Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.