[PATCH 1 of 3] changeset_status: add unit tests for calculation of overall status

[Mads Kiilerich]

On 04/18/2015 04:11 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
> # Date 1429274977 -7200
> #      Fri Apr 17 14:49:37 2015 +0200
> # Node ID 928837725c3c0d8e7e8fbef23b7e12d5a4ddb66c
> # Parent  6e760af6050e567239155a7a43b5ab02eddf877d
> changeset_status: add unit tests for calculation of overall status
>
> Add unit tests for the calculation of the review status of a changeset/pull
> request. To allow this, some reorganization of the ChangesetStatusModel is
> needed.
>
> diff --git a/kallithea/model/changeset_status.py b/kallithea/model/changeset_status.py
> --- a/kallithea/model/changeset_status.py
> +++ b/kallithea/model/changeset_status.py
> @@ -66,9 +66,27 @@
>           q = q.order_by(ChangesetStatus.version.asc())
>           return q
>   
> +    def _calculate_status(self, statuses):
> +        """
> +        Given a list of statuses, calculate the resulting status, according to
> +        the policy: approve if consensus.
> +        """
> +
> +        approved_votes = 0
> +        for st in statuses:
> +            if st and st.status == ChangesetStatus.STATUS_APPROVED:
> +                approved_votes += 1
> +
> +        result = ChangesetStatus.STATUS_UNDER_REVIEW
> +        if approved_votes and approved_votes == len(statuses):
> +            result = ChangesetStatus.STATUS_APPROVED
> +
> +        return result
> +
>       def calculate_pull_request_result(self, pull_request):
>           """
> -        Policy: approve if consensus. Only approve and reject counts as valid votes.
> +        Return a tuple (reviewers, pending reviewers, pull request status)
> +        Only approve and reject counts as valid votes.
>           """
>   
>           # collect latest votes from all voters
> @@ -77,24 +95,21 @@
>                                                pull_request=pull_request,
>                                                with_revisions=True)):
>               cs_statuses[st.author.username] = st
> +
>           # collect votes from official reviewers
>           pull_request_reviewers = []
>           pull_request_pending_reviewers = []
> -        approved_votes = 0
> +        relevant_statuses = []
>           for r in pull_request.reviewers:
>               st = cs_statuses.get(r.user.username)
> -            if st and st.status == ChangesetStatus.STATUS_APPROVED:
> -                approved_votes += 1
> +            relevant_statuses.append(st)
>               if not st or st.status in (ChangesetStatus.STATUS_NOT_REVIEWED,
>                                          ChangesetStatus.STATUS_UNDER_REVIEW):
>                   st = None
>                   pull_request_pending_reviewers.append(r.user)
>               pull_request_reviewers.append((r.user, st))
>   
> -        # calculate result
> -        result = ChangesetStatus.STATUS_UNDER_REVIEW
> -        if approved_votes and approved_votes == len(pull_request.reviewers):
> -            result = ChangesetStatus.STATUS_APPROVED
> +        result = self._calculate_status(relevant_statuses)
>   
>           return (pull_request_reviewers,
>                   pull_request_pending_reviewers,
> diff --git a/kallithea/tests/models/test_changeset_status.py b/kallithea/tests/models/test_changeset_status.py
> new file mode 100644
> --- /dev/null
> +++ b/kallithea/tests/models/test_changeset_status.py
> @@ -0,0 +1,39 @@
> +from kallithea.tests import *
> +from kallithea.model.changeset_status import ChangesetStatusModel
> +from kallithea.model.db import ChangesetStatus
> +
> +# shorthands
> +STATUS_APPROVED = ChangesetStatus.STATUS_APPROVED
> +STATUS_REJECTED = ChangesetStatus.STATUS_REJECTED
> +STATUS_NOT_REVIEWED = ChangesetStatus.STATUS_NOT_REVIEWED
> +STATUS_UNDER_REVIEW = ChangesetStatus.STATUS_UNDER_REVIEW

We could perhaps just import ChangesetStatus as CS?

> +
> +class ChangesetStatusMock(object):
> +
> +    def __init__(self, status):
> +        self.status = status
> +
> +S = ChangesetStatusMock

Perhaps use a less short abbreviation such as CSM? Perhaps name it that 
way when it is defined and make it clear what the full name would be?

> +
> +class TestChangesetStatusCalculation(BaseTestCase):
> +
> +    def setUp(self):
> +        self.m = ChangesetStatusModel()
> +
> +    @parameterized.expand([
> +        ('empty list', STATUS_UNDER_REVIEW, []),
> +        ('approve', STATUS_APPROVED, [S(STATUS_APPROVED)]),
> +        ('approve2', STATUS_APPROVED, [S(STATUS_APPROVED), S(STATUS_APPROVED)]),
> +        ('approve_reject', STATUS_UNDER_REVIEW, [S(STATUS_APPROVED), S(STATUS_REJECTED)]),
> +        ('approve_underreview', STATUS_UNDER_REVIEW, [S(STATUS_APPROVED), S(STATUS_UNDER_REVIEW)]),
> +        ('approve_notreviewed', STATUS_UNDER_REVIEW, [S(STATUS_APPROVED), S(STATUS_NOT_REVIEWED)]),
> +        ('underreview', STATUS_UNDER_REVIEW, [S(STATUS_UNDER_REVIEW), S(STATUS_UNDER_REVIEW)]),
> +        ('reject', STATUS_UNDER_REVIEW, [S(STATUS_REJECTED)]),
> +        ('reject_underreview', STATUS_UNDER_REVIEW, [S(STATUS_REJECTED), S(STATUS_UNDER_REVIEW)]),
> +        ('reject_notreviewed', STATUS_UNDER_REVIEW, [S(STATUS_REJECTED), S(STATUS_NOT_REVIEWED)]),
> +        ('notreviewed', STATUS_UNDER_REVIEW, [S(STATUS_NOT_REVIEWED)]),
> +    ])
> +    def test_result(self, name, expected_result, statuses):
> +        result = self.m._calculate_status(statuses)
> +        self.assertEqual(result, expected_result)
> +