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

[Thomas De Schampheleire]

On Sun, Apr 19, 2015 at 12:49 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 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)
>> +
>
>

Both of your comments sounded fine to me, but I see the commits are
already applied. Do you expect a follow-up on this patch or did you
change your mind?