Hi Stephen, So long as this doesn't decrease the coverage, I am all for simpler and clearer tests!
Regards, Daniel > - Combine anonymous user tests with authenticated user tests > - Rename and simplify some tests > - Improve documentation > > Signed-off-by: Stephen Finucane <[email protected]> > Cc: Andy Doan <[email protected]> > --- > patchwork/api/project.py | 1 - > patchwork/tests/test_rest_api.py | 218 > ++++++++++++++++----------------------- > 2 files changed, 89 insertions(+), 130 deletions(-) > > diff --git a/patchwork/api/project.py b/patchwork/api/project.py > index 3319339..881de2d 100644 > --- a/patchwork/api/project.py > +++ b/patchwork/api/project.py > @@ -36,7 +36,6 @@ class ProjectSerializer(HyperlinkedModelSerializer): > model = Project > fields = ('url', 'name', 'link_name', 'list_id', 'list_email', > 'web_url', 'scm_url', 'webscm_url') > - read_only_fields = ('name', 'maintainers') > extra_kwargs = { > 'url': {'view_name': 'api-project-detail'}, > } > diff --git a/patchwork/tests/test_rest_api.py > b/patchwork/tests/test_rest_api.py > index 667f9f3..4675191 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -24,7 +24,6 @@ from django.conf import settings > from django.core.urlresolvers import reverse > > from patchwork.models import Check > -from patchwork.models import Patch > from patchwork.models import Project > from patchwork.tests.utils import create_check > from patchwork.tests.utils import create_maintainer > @@ -52,7 +51,7 @@ class TestProjectAPI(APITestCase): > return reverse('api-project-list') > return reverse('api-project-detail', args=[item]) > > - def test_list_simple(self): > + def test_list(self): > """Validate we can list the default test project.""" > project = create_project() > > @@ -85,64 +84,53 @@ class TestProjectAPI(APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertEqual(project.name, resp.data['name']) > > - def test_anonymous_create(self): > - """Ensure anonymous POST operations are rejected.""" > - resp = self.client.post( > - self.api_url(), > - {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'}) > - self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > - > - def test_anonymous_update(self): > - """Ensure anonymous "PATCH" operations are rejected.""" > - project = create_project() > - > - resp = self.client.patch(self.api_url(project.id), > - {'linkname': 'foo'}) > - self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > - > - def test_anonymous_delete(self): > - """Ensure anonymous "DELETE" operations are rejected.""" > - project = create_project() > - > - resp = self.client.delete(self.api_url(project.id)) > - self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > - > def test_create(self): > """Ensure creations are rejected.""" > project = create_project() > + data = {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': > 'e'} > + > + # an anonymous user > + resp = self.client.post(self.api_url(), data) > + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > + # a superuser > user = create_maintainer(project) > user.is_superuser = True > user.save() > self.client.force_authenticate(user=user) > - resp = self.client.post( > - self.api_url(), > - {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'}) > + resp = self.client.post(self.api_url(), data) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > def test_update(self): > - """Ensure updates can be performed maintainers.""" > + """Ensure updates can be performed by maintainers.""" > project = create_project() > + data = {'linkname': 'TEST'} > > - # A maintainer can update > - user = create_maintainer(project) > - self.client.force_authenticate(user=user) > - resp = self.client.patch(self.api_url(project.id), > - {'linkname': 'TEST'}) > - self.assertEqual(status.HTTP_200_OK, resp.status_code) > + # an anonymous user > + resp = self.client.patch(self.api_url(project.id), data) > + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > - # A normal user can't > + # a normal user > user = create_user() > self.client.force_authenticate(user=user) > - resp = self.client.patch(self.api_url(project.id), > - {'linkname': 'TEST'}) > + resp = self.client.patch(self.api_url(project.id), data) > self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > + # a maintainer > + user = create_maintainer(project) > + self.client.force_authenticate(user=user) > + resp = self.client.patch(self.api_url(project.id), data) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + > def test_delete(self): > """Ensure deletions are rejected.""" > - # Even an admin can't remove a project > project = create_project() > > + # an anonymous user > + resp = self.client.delete(self.api_url(project.id)) > + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > + > + # a super user > user = create_maintainer(project) > user.is_superuser = True > user.save() > @@ -161,13 +149,13 @@ class TestPersonAPI(APITestCase): > return reverse('api-person-list') > return reverse('api-person-detail', args=[item]) > > - def test_anonymous_list(self): > - """The API should reject anonymous users.""" > + def test_list(self): > + """This API requires authenticated users.""" > + # anonymous user > resp = self.client.get(self.api_url()) > self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > - def test_authenticated_list(self): > - """This API requires authenticated users.""" > + # authenticated user > user = create_user() > self.client.force_authenticate(user=user) > > @@ -186,24 +174,22 @@ class TestPersonAPI(APITestCase): > resp = self.client.get(self.api_url()) > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertEqual(2, len(resp.data)) > - self.assertEqual(person.name, > - resp.data[0]['name']) > + self.assertEqual(person.name, resp.data[0]['name']) > self.assertIsNone(resp.data[0]['user']) > > - def test_readonly(self): > + def test_create_update_delete(self): > user = create_maintainer() > user.is_superuser = True > user.save() > self.client.force_authenticate(user=user) > > - resp = self.client.delete(self.api_url(user.id)) > + resp = self.client.post(self.api_url(), {'email': '[email protected]'}) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > - resp = self.client.patch(self.api_url(user.id), > - {'email': '[email protected]'}) > + resp = self.client.patch(self.api_url(user.id), {'email': > '[email protected]'}) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > - resp = self.client.post(self.api_url(), {'email': '[email protected]'}) > + resp = self.client.delete(self.api_url(user.id)) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > > @@ -216,13 +202,13 @@ class TestUserAPI(APITestCase): > return reverse('api-user-list') > return reverse('api-user-detail', args=[item]) > > - def test_anonymous_list(self): > - """The API should reject anonymous users.""" > + def test_list(self): > + """This API requires authenticated users.""" > + # anonymous users > resp = self.client.get(self.api_url()) > self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > - def test_authenticated_list(self): > - """This API requires authenticated users.""" > + # authenticated user > user = create_user() > self.client.force_authenticate(user=user) > > @@ -233,15 +219,6 @@ class TestUserAPI(APITestCase): > self.assertNotIn('password', resp.data[0]) > self.assertNotIn('is_superuser', resp.data[0]) > > - def test_update(self): > - user = create_maintainer() > - user.is_superuser = True > - user.save() > - self.client.force_authenticate(user=user) > - > - resp = self.client.patch(self.api_url(user.id), {'first_name': > 'Tan'}) > - self.assertEqual(status.HTTP_200_OK, resp.status_code) > - > def test_create_delete(self): > user = create_maintainer() > user.is_superuser = True > @@ -254,6 +231,18 @@ class TestUserAPI(APITestCase): > resp = self.client.post(self.api_url(user.id), {'email': > '[email protected]'}) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > + resp = self.client.delete(self.api_url(user.id)) > + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > + > + def test_update(self): > + user = create_maintainer() > + user.is_superuser = True > + user.save() > + self.client.force_authenticate(user=user) > + > + resp = self.client.patch(self.api_url(user.id), {'first_name': > 'Tan'}) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + > > @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') > class TestPatchAPI(APITestCase): > @@ -265,7 +254,7 @@ class TestPatchAPI(APITestCase): > return reverse('api-patch-list') > return reverse('api-patch-detail', args=[item]) > > - def test_list_simple(self): > + def test_list(self): > """Validate we can list a patch.""" > resp = self.client.get(self.api_url()) > self.assertEqual(status.HTTP_200_OK, resp.status_code) > @@ -316,93 +305,67 @@ class TestPatchAPI(APITestCase): > self.assertEqual(3, len(tags)) > self.assertEqual(1, tags['Reviewed-by']) > > - def test_anonymous_create(self): > - """Ensure anonymous "POST" operations are rejected.""" > + def test_create(self): > + """Ensure creations are rejected.""" > + project = create_project() > patch = { > - 'project': create_project().id, > + 'project': project, > 'submitter': create_person().id, > 'msgid': make_msgid(), > 'name': 'test-create-patch', > 'diff': 'patch diff', > } > > + # anonymous user > resp = self.client.post(self.api_url(), patch) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > - def test_anonymous_update(self): > - """Ensure anonymous "PATCH" operations are rejected.""" > - patch = create_patch() > - patch_url = self.api_url(patch.id) > - > - resp = self.client.get(patch_url) > - patch = resp.data > - patch['msgid'] = 'foo' > - patch['name'] = 'this should fail' > - > - resp = self.client.patch(patch_url, {'name': 'foo'}) > - self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > - > - def test_anonymous_delete(self): > - """Ensure anonymous "DELETE" operations are rejected.""" > - patch = create_patch() > - patch_url = self.api_url(patch.id) > - > - resp = self.client.delete(patch_url) > - self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > - > - def test_create(self): > - """Ensure creations are rejected.""" > - submitter = create_person() > - project = create_project() > + # superuser > user = create_maintainer(project) > user.is_superuser = True > user.save() > self.client.force_authenticate(user=user) > - > - patch = { > - 'project': project.id, > - 'submitter': submitter.id, > - 'msgid': make_msgid(), > - 'name': 'test-create-patch', > - 'diff': 'patch diff', > - } > - > resp = self.client.post(self.api_url(), patch) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > > def test_update(self): > """Ensure updates can be performed by maintainers.""" > - # A maintainer can update > project = create_project() > patch = create_patch(project=project) > state = create_state() > - user = create_maintainer(project) > - self.client.force_authenticate(user=user) > > - resp = self.client.patch(self.api_url(patch.id), > - {'state': state.name}) > - self.assertEqual(status.HTTP_200_OK, resp.status_code) > + # anonymous user > + resp = self.client.patch(self.api_url(patch.id), {'state': > state.name}) > + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > - # A normal user can't > + # authenticated user > user = create_user() > self.client.force_authenticate(user=user) > - > - resp = self.client.patch(self.api_url(patch.id), > - {'state': state.name}) > + resp = self.client.patch(self.api_url(patch.id), {'state': > state.name}) > self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > + # maintainer > + user = create_maintainer(project) > + self.client.force_authenticate(user=user) > + resp = self.client.patch(self.api_url(patch.id), {'state': > state.name}) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + > def test_delete(self): > """Ensure deletions are always rejected.""" > project = create_project() > patch = create_patch(project=project) > + > + # anonymous user > + resp = self.client.delete(self.api_url(patch.id)) > + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > + > + # superuser > user = create_maintainer(project) > user.is_superuser = True > user.save() > self.client.force_authenticate(user=user) > - > resp = self.client.delete(self.api_url(patch.id)) > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > - self.assertEqual(1, Patch.objects.all().count()) > > > @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API') > @@ -428,7 +391,7 @@ class TestCheckAPI(APITestCase): > } > return create_check(**values) > > - def test_list_simple(self): > + def test_list(self): > """Validate we can list checks on a patch.""" > resp = self.client.get(self.api_url()) > self.assertEqual(status.HTTP_200_OK, resp.status_code) > @@ -452,22 +415,6 @@ class TestCheckAPI(APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertEqual(check.target_url, resp.data['target_url']) > > - def test_update_delete(self): > - """Ensure updates and deletes aren't allowed""" > - check = self._create_check() > - self.user.is_superuser = True > - self.user.save() > - self.client.force_authenticate(user=self.user) > - > - # update > - resp = self.client.patch(self.api_url(check), > - {'target_url': 'fail'}) > - self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > - > - # delete > - resp = self.client.delete(self.api_url(check)) > - self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > - > def test_create(self): > """Ensure creations can be performed by user of patch.""" > check = { > @@ -500,3 +447,16 @@ class TestCheckAPI(APITestCase): > resp = self.client.post(self.api_url(), check) > self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code) > self.assertEqual(0, Check.objects.all().count()) > + > + def test_update_delete(self): > + """Ensure updates and deletes aren't allowed""" > + check = self._create_check() > + self.user.is_superuser = True > + self.user.save() > + self.client.force_authenticate(user=self.user) > + > + resp = self.client.patch(self.api_url(check), {'target_url': 'fail'}) > + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > + > + resp = self.client.delete(self.api_url(check)) > + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, > resp.status_code) > -- > 2.7.4 > > _______________________________________________ > Patchwork mailing list > [email protected] > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
