Skip to content

Conversation

@Yash0219
Copy link

This pull request fixes a small bug and improves the structure of the test cases.

Fixed:

  • Removed incorrect usage of the json module as a format parameter
  • All requests now use proper JSON encoding and content type
  • Added status code assertions for request types: GET, PATCH, PUT, DELETE

Clean-up:

  • Improved clarity in Unicode and escaped character test cases
  • Made the tests more readable and reliable

Let me know if anything else is needed. Happy to help!

Copy link
Owner

@gunthercox gunthercox left a comment

Choose a reason for hiding this comment

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

Overall these changes seem good but there are a number of deletions that seem arbitrary. I'd be happy to merge this in once the notes I left are addressed.

from django.test import TestCase
from django.urls import reverse


Copy link
Owner

Choose a reason for hiding this comment

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

Two blank lines before a top level function is preferred to adhere to pep8:

https://peps.python.org/pep-0008/#blank-lines

def test_post(self):
"""
Test that a response is returned.
"""
Copy link
Owner

Choose a reason for hiding this comment

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

Could the docstrings in these functions have been improved instead of removed?

response = self.client.post(
self.api_url,
data=json.dumps({
'text': u'سلام'
Copy link
Owner

Choose a reason for hiding this comment

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

I had to look this one up, but this seems safe to remove ✅
(Python 3 no longer needs the u bytemark, good to know)


def test_delete(self):
response = self.client.delete(self.api_url)

Copy link
Owner

Choose a reason for hiding this comment

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

A space between test calls and assertions is preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants