[4/4] tests: Tweak comment API tests

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(-)

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
>