7
\$\begingroup\$

I am currently developing a Django Rest Framework API with TDD. I have 15 tests and counting for two views, which doesn't seem right to me. It takes me a lot of time to write the tests and a lot of the testing logic seems redundant to me, especially because I have to check permissions for each test request. Furthermore I am unsure how to test permissions. If every view has about 3 permissions, it takes an enormous amount of tests to cover it. My questions are:

  • Should my tests be so much?
  • Am I testing in an efficient way?

Please comment on my way of testing and give me any advice regarding how I can improve it.

Here are my Views:

class UserListView(APIView):
    permission_classes = [IsAuthenticated, IsAdminUser]

    def get(self, request, format=None):

        users = User.objects.all()
        serializer = UserSerializer(users, many=True)
        return Response(serializer.data) #if you only return the data, it is automatically HTTP_200_OK

    def post(self, request, format=None):

        serializer = UserSerializer(data=request.data)
        if serializer.is_valid():
            serializer.save()
            return Response(serializer.data, status=status.HTTP_201_CREATED)

        return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

class UserDetailView(APIView):
    permission_classes = [IsAuthenticated, IsAdminUser]

    def get_object(self, pk):
        try:
            return User.objects.get(pk=pk)
        except User.DoesNotExist:
            raise Http404

    def get(self, request, pk, format=None):
        user = self.get_object(pk)
        serializer = UserSerializer(user)
        return Response(serializer.data)

    def put(self, request, pk, format=None):
        user = self.get_object(pk)
        serializer = UserSerializer(user, data=request.data)
        if serializer.is_valid():
            serializer.save()
            return Response(serializer.data)

        return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

And here is my tests:

class TestUserBase(APITestCase):

    def setUp(self):
        self.client = APIClient()

        self.regular_user = User.objects.create_user(email="testuser1@example.com", password="1234")
        self.super_user = User.objects.create_superuser(email="supertestuser1@example.com", password="1234")

    def _super_auth(self):
        self.client.force_authenticate(user=self.super_user)

    def _regular_auth(self):
        self.client.force_authenticate(user=self.regular_user)

class TestUserListView(TestUserBase):
    url = reverse('user-list')

    valid_user = {
        "email": "testuser@valid.com",
        "password": "validvalid",
        "first_name": "test",
        "last_name": "user"
    }

    invalid_user = {
        "email": "",
        "password": "validvalid",
        "first_name": "test",
        "last_name": "user"
    }

    def test_unauthorized_users_view(self):
        response = self.client.get(self.url)

        self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

    def test_authorized_but_forbidden_users_view(self):
        self._regular_auth()
        response = self.client.get(self.url)

        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

    def test_authorized_and_permitted_users_view(self):
        self._super_auth()
        response = self.client.get(self.url)

        expected_data = UserSerializer(User.objects.all(), many=True).data

        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertEqual(response.data, expected_data)
        self.assertEqual(User.objects.count(), 2)

    def test_unauthorized_user_create(self):
        response = self.client.post(self.url, self.valid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

    def test_authorized_but_forbidden_user_create(self):
        self._regular_auth()
        response = self.client.post(self.url, self.valid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

    def test_authorized_and_permitted_invalid_user_create(self):
        self._super_auth()
        response = self.client.post(self.url, self.invalid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)

    def test_authorized_and_permitted_valid_user_create(self):
        self._super_auth()
        response = self.client.post(self.url, self.valid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
        self.assertEqual(response.data, self.valid_user)
        self.assertEqual(User.objects.count(), 3)

class TestUserDetailView(TestUserBase):
    valid_url = reverse('user-detail', kwargs={"pk": 1})
    invalid_url = reverse('user-detail', kwargs={"pk": 5})

    valid_user = {
        "email": "testuser1@example.com",
        "first_name": "bob",
        "last_name": "von peer"
    }

    invalid_user = {
        "email": "johnny@nevergonnaupdate.com"
    }

    def test_unauthorized_user_view(self):
        response = self.client.get(self.valid_url)

        self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

    def test_authorized_but_forbidden_user_view(self):
        self._regular_auth()
        response = self.client.get(self.valid_url)

        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

    def test_authorized_and_permitted_valid_user_view(self):
        self._super_auth()
        response = self.client.get(self.valid_url)

        expected_data = UserSerializer(User.objects.get(pk=1)).data

        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertEqual(response.data, expected_data)

    def test_authorized_and_permitted_invalid_user_view(self):
        self._super_auth()
        response = self.client.get(self.invalid_url)

        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)

    def test_unauthorized_user_update(self):
        response = self.client.post(self.valid_url, self.valid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

    def test_authorized_but_forbidden_user_update(self):
        self._regular_auth()
        response = self.client.post(self.valid_url, self.valid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

    def test_authorized_and_permitted_valid_user_update(self):
        self._super_auth()
        response = self.client.put(self.valid_url, self.valid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertEqual(response.data, self.valid_user)

    def test_authorized_and_permitted_invalid_user_update(self):
        self._super_auth()
        response = self.client.put(self.valid_url, self.invalid_user, format='json')

        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
\$\endgroup\$
  • \$\begingroup\$ you could reduce the code with data driven tests \$\endgroup\$ – Ewan Mar 27 '19 at 14:55
  • \$\begingroup\$ You no need to rewrite all tests again, just put all your repetible code inside a class and then each test class can inherits from this new class. I think that is totally nessesary test all about permissions on each view and it doesn't redundat \$\endgroup\$ – German Alzate Mar 27 '19 at 18:55
  • \$\begingroup\$ I will you share a repository on Github, you can see how I implement TDD, and I have another repos with similar implementation, but they are in other languages \$\endgroup\$ – German Alzate Mar 27 '19 at 18:59
3
\$\begingroup\$

As a side note, including your "includes" in the code you want reviewed makes it much easier to do so, this way we don't have to check if, for example APIClient and User are parts of the django library, because it's not really mandatory to understand Django to review unit testing.

Now, for the review :

test_authorized_and_permitted_users_view :

You test that User.objects contains two items, but the code you are testing has nothing to do with this. By writing this, you are testing your unit test kind of like this :

def some_method(parameter):
    return paramter+1

def some_test():
    parameter = 1
    result = some_method(parameter)

    self.assertEqual(parameter, 1)
    ....

You already test that expected_data = response.data and that's plenty enough. It's a "risk" to test more than your test should test because if something ever changes that isn't related to the tested code and the test fails, well you might search for awhile what's the problem and bad tests are worst than no test (citation needed).

test_authorized_and_permitted_invalid_user_create

This test isn't "unit", it tests two things. It tests that an authorized user can access the page and it tests that the creation fails with an invalid user. It should be two tests, because if your test ever fails you don't want to wonder if it's because of a change in the authentication module or in the user creation module (That's pretty much the point of a unit test)

For your other tests, there are also places where both these comments apply. Apart from these points, I think you've pretty much nailed the principle of unit testing. What isn't shown in your code though, is if you use dependency injection. This is a pretty valuable tool when it comes to unit testing, because you don't want to have complex mechanisms running when your unit tests run. There are two important (I really felt the need to bold that word :p) reasons for this :

  1. If your tests are slow, chances are devs won't run them. And if you force them to run before pushing or stuff like that, they'll a) loose time while the tests are running b) won't push as often as they should.
  2. It makes another thing that could make your test fail for the wrong reason. If, for example, you have

    def some_simple_unit_test():
        real_database.connect()
    
        # do things and assert
    

If the database connection fails for whatever reason, your test will fail, but was it really what you wanted? Does your test really need to test access to a database or could it access some simple in-memory data storage for a simple unit test? Wouldn't you expect whichever database provider who wrote the package to have tested it first, so you don't need to?

The whole point of this last point is to tell you that it's important to keep tests simple and I don't think it's been done enough in your case (But maybe there're some tests configuration I'm missing)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.