On Sun, Apr 19, 2015 at 12:49 AM, Mads Kiilerich <m...@kiilerich.com> wrote: > On 04/18/2015 04:11 PM, Thomas De Schampheleire wrote: >> >> # HG changeset patch >> # User Thomas De Schampheleire <thomas.de.schamphele...@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? _______________________________________________ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general