Message ID | 20210826171831.547578-5-stephen@that.guru |
---|---|
State | Accepted |
Headers | show |
Series | Make addressed/unaddressed workflow opt-in | expand |
Related | show |
Not really something for this exact patch, but I do wonder as API versions proliferate if we should have infrastructure to test something on all supported API versions. [Perhaps it's my lack of a formal CS education showing here but I feel like there's probably some software design pattern for this, and I have a weird intuition that I'm describing a Factory, but the wikipedia article left me more confused than enlightened.] Anyway, patch itself LGTM. I'm trying not to have PW take over my whole work day so I haven't done a detailed review. Kind regards, Daniel Stephen Finucane <stephen@that.guru> writes: > We were missing tests for 'GET /patch/{patch_id}/comment' (list patch > comments) and 'GET /cover/{cover_id}/comment' (list cover comments) when > using API version 1.2. In addition, we had effectively duplicated tests > by including explicit tests for API 1.3. These are unnecessary since we > default to testing against the latest version. Address both issues. > > Signed-off-by: Stephen Finucane <stephen@that.guru> > --- > patchwork/tests/api/test_comment.py | 40 ++++++++++++++++------------- > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git patchwork/tests/api/test_comment.py patchwork/tests/api/test_comment.py > index 74acf447..5c035e82 100644 > --- patchwork/tests/api/test_comment.py > +++ patchwork/tests/api/test_comment.py > @@ -69,6 +69,17 @@ class TestCoverComments(utils.APITestCase): > self.assertEqual(1, len(resp.data)) > self.assertSerialized(comment, resp.data[0]) > self.assertIn('list_archive_url', resp.data[0]) > + self.assertIn('addressed', resp.data[0]) > + > + def test_list_version_1_2(self): > + """List cover letter comments using API v1.2.""" > + create_cover_comment(cover=self.cover) > + > + resp = self.client.get(self.api_url(self.cover, version='1.2')) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertEqual(1, len(resp.data)) > + self.assertIn('list_archive_url', resp.data[0]) > + self.assertNotIn('addressed', resp.data[0]) > > def test_list_version_1_1(self): > """List cover letter comments using API v1.1.""" > @@ -110,15 +121,6 @@ class TestCoverComments(utils.APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertSerialized(comment, resp.data) > > - def test_detail_version_1_3(self): > - """Show a cover letter comment using API v1.3.""" > - comment = create_cover_comment(cover=self.cover) > - > - resp = self.client.get( > - self.api_url(self.cover, version='1.3', item=comment)) > - self.assertEqual(status.HTTP_200_OK, resp.status_code) > - self.assertSerialized(comment, resp.data) > - > def test_detail_version_1_2(self): > """Show a cover letter comment using API v1.2.""" > comment = create_cover_comment(cover=self.cover) > @@ -292,6 +294,17 @@ class TestPatchComments(utils.APITestCase): > self.assertEqual(1, len(resp.data)) > self.assertSerialized(comment, resp.data[0]) > self.assertIn('list_archive_url', resp.data[0]) > + self.assertIn('addressed', resp.data[0]) > + > + def test_list_version_1_2(self): > + """List patch comments using API v1.2.""" > + create_patch_comment(patch=self.patch) > + > + resp = self.client.get(self.api_url(self.patch, version='1.2')) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertEqual(1, len(resp.data)) > + self.assertIn('list_archive_url', resp.data[0]) > + self.assertNotIn('addressed', resp.data[0]) > > def test_list_version_1_1(self): > """List patch comments using API v1.1.""" > @@ -333,15 +346,6 @@ class TestPatchComments(utils.APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertSerialized(comment, resp.data) > > - def test_detail_version_1_3(self): > - """Show a patch comment using API v1.3.""" > - comment = create_patch_comment(patch=self.patch) > - > - resp = self.client.get( > - self.api_url(self.patch, version='1.3', item=comment)) > - self.assertEqual(status.HTTP_200_OK, resp.status_code) > - self.assertSerialized(comment, resp.data) > - > def test_detail_version_1_2(self): > """Show a patch comment using API v1.2.""" > comment = create_patch_comment(patch=self.patch) > -- > 2.31.1
On Thu, 2021-09-02 at 12:24 +1000, Daniel Axtens wrote: > Not really something for this exact patch, but I do wonder as API > versions proliferate if we should have infrastructure to test something > on all supported API versions. > > [Perhaps it's my lack of a formal CS education showing here but I feel > like there's probably some software design pattern for this, and I have > a weird intuition that I'm describing a Factory, but the wikipedia > article left me more confused than enlightened.] We _could_ but that's a lot of extra tests to run for very little gain. So long as we're doing boundary testing I _think_ what we have is good enough. That means we should test for a field's absence in N-1, presence in N, and presence in N+1 (if and when that version exists). I think we're doing relatively good job of this at the moment so I wouldn't be inclined to change too much, but of course I'm open to other suggestions :) Stephen > Anyway, patch itself LGTM. I'm trying not to have PW take over my whole work > day so I haven't done a detailed review. > > Kind regards, > Daniel >
diff --git patchwork/tests/api/test_comment.py patchwork/tests/api/test_comment.py index 74acf447..5c035e82 100644 --- patchwork/tests/api/test_comment.py +++ patchwork/tests/api/test_comment.py @@ -69,6 +69,17 @@ class TestCoverComments(utils.APITestCase): self.assertEqual(1, len(resp.data)) self.assertSerialized(comment, resp.data[0]) self.assertIn('list_archive_url', resp.data[0]) + self.assertIn('addressed', resp.data[0]) + + def test_list_version_1_2(self): + """List cover letter comments using API v1.2.""" + create_cover_comment(cover=self.cover) + + resp = self.client.get(self.api_url(self.cover, version='1.2')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(1, len(resp.data)) + self.assertIn('list_archive_url', resp.data[0]) + self.assertNotIn('addressed', resp.data[0]) def test_list_version_1_1(self): """List cover letter comments using API v1.1.""" @@ -110,15 +121,6 @@ class TestCoverComments(utils.APITestCase): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(comment, resp.data) - def test_detail_version_1_3(self): - """Show a cover letter comment using API v1.3.""" - comment = create_cover_comment(cover=self.cover) - - resp = self.client.get( - self.api_url(self.cover, version='1.3', item=comment)) - self.assertEqual(status.HTTP_200_OK, resp.status_code) - self.assertSerialized(comment, resp.data) - def test_detail_version_1_2(self): """Show a cover letter comment using API v1.2.""" comment = create_cover_comment(cover=self.cover) @@ -292,6 +294,17 @@ class TestPatchComments(utils.APITestCase): self.assertEqual(1, len(resp.data)) self.assertSerialized(comment, resp.data[0]) self.assertIn('list_archive_url', resp.data[0]) + self.assertIn('addressed', resp.data[0]) + + def test_list_version_1_2(self): + """List patch comments using API v1.2.""" + create_patch_comment(patch=self.patch) + + resp = self.client.get(self.api_url(self.patch, version='1.2')) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(1, len(resp.data)) + self.assertIn('list_archive_url', resp.data[0]) + self.assertNotIn('addressed', resp.data[0]) def test_list_version_1_1(self): """List patch comments using API v1.1.""" @@ -333,15 +346,6 @@ class TestPatchComments(utils.APITestCase): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertSerialized(comment, resp.data) - def test_detail_version_1_3(self): - """Show a patch comment using API v1.3.""" - comment = create_patch_comment(patch=self.patch) - - resp = self.client.get( - self.api_url(self.patch, version='1.3', item=comment)) - self.assertEqual(status.HTTP_200_OK, resp.status_code) - self.assertSerialized(comment, resp.data) - def test_detail_version_1_2(self): """Show a patch comment using API v1.2.""" comment = create_patch_comment(patch=self.patch)
We were missing tests for 'GET /patch/{patch_id}/comment' (list patch comments) and 'GET /cover/{cover_id}/comment' (list cover comments) when using API version 1.2. In addition, we had effectively duplicated tests by including explicit tests for API 1.3. These are unnecessary since we default to testing against the latest version. Address both issues. Signed-off-by: Stephen Finucane <stephen@that.guru> --- patchwork/tests/api/test_comment.py | 40 ++++++++++++++++------------- 1 file changed, 22 insertions(+), 18 deletions(-)