Merge lp:~dpb/latch-test/dont-test-so-much into lp:latch-test

[David Britton]

When the testing starts, set review to Abstain, when scanner runs, don't launch more tests if it notices 'Abstain' on the bot's slot.

When the testing starts, set review to Abstain, when scanner runs, don't launch
more tests if it notices 'Abstain' on the bot's slot.

Testing instructions:

it's not super easy, since you really need an LP project and an MP, but please
feel free to check out this MP, which I used for testing:

https://code.launchpad.net/~davidpbritton/dpb-code/test-8/+merge/214602

Notice the abstain was put there by 'test', and the 'scan' job ignored this MP
because of it. Here is the config I use for testing on the 'dpb-code' project,
and you could set up anything similar on your project as well.

[lp:dpb-code]
verify_command = sleep 60; /bin/false
voting_criteria = Approve >= 1, Disapprove == 0
commit_message_template = Merged <branch_name>. [f=<bugs_fixed>] [r=<approved_by_nicks>]\n<commit_message>
artifacts = tarmac.conf

[🤖 Landscape Builder (landscape-builder)]

Command: make test
Result: Success
Revno: 15
Branch: lp:~davidpbritton/latch-test/dont-test-so-much
Jenkins: https://ci.lscape.net/job/latch-test/4066/

review: Approve (test results)

[Benji York (benji)]

Given the complexity of setting up a test environment, the low probability of damage due to a bug in this branch, and the fact that misbehavior will be obvious once landed, I did not run the code in this branch.

The code looks good. I had a couple questions/small suggestions that should be addressed before landing, but I'm sure the responses will be land-worthy, so I'm approving presuming the tweaks.

review: Approve

[lp:~dpb/latch-test/dont-test-so-much]

16. By David Britton on 2016-03-25

revert change of 'config' file, accident.

17. By David Britton on 2016-03-25

turn add_votes into add_vote and call multiple times to clean up usage.

[🤖 Landscape Builder (landscape-builder)]

Command: make test
Result: Success
Revno: 17
Branch: lp:~davidpbritton/latch-test/dont-test-so-much
Jenkins: https://ci.lscape.net/job/latch-test/4079/

review: Approve (test results)

[lp:~dpb/latch-test/dont-test-so-much]

18. By David Britton on 2016-03-25

s/with multiple//

[🤖 Landscape Builder (landscape-builder)]

Command: make test
Result: Success
Revno: 18
Branch: lp:~davidpbritton/latch-test/dont-test-so-much
Jenkins: https://ci.lscape.net/job/latch-test/4080/

review: Approve (test results)

[Данило Шеган (danilo)]

I am getting a lint error on "make lint":

./latch.py:197:33: E901 SyntaxError: invalid syntax
./test_latch.py:73:1: E302 expected 2 blank lines, found 1
Makefile:8: recipe for target 'lint' failed
make: *** [lint] Error 1

This might be due to python3 being default on xenial, but it's easy enough to fix, so please do that (wrap print parameters on line 197 inside parentheses).

A few other nits inline, but overall a nice change: thanks for doing this work!

review: Approve

[lp:~dpb/latch-test/dont-test-so-much]

19. By David Britton on 2016-03-28

s/thisvote/this_vote/

20. By David Britton on 2016-03-28

danilo[diff]: use vote_lookup, not 'Abstain' directly.

21. By David Britton on 2016-03-28

danilo[diff]: restructure conditional

=== modified file 'latch.py'
--- latch.py	2015-10-01 16:16:38 +0000
+++ latch.py	2016-03-28 14:06:24 +0000
@@ -205,6 +205,7 @@
         self.lp_candidate = lp_candidate
         self.readonly = readonly
         self.vote_lookup = {
+            "Testing": "Abstain",
             "Fail": "Needs Fixing",
             "Success": "Approve"}
 
@@ -235,8 +236,27 @@
             return True
         return False
 
-    def add_result(self, result):
-        """Add this test result to the launchpad candidate."""
+    def get_my_vote(self):
+        """Return my most recent vote, or None, if I have not voted."""
+        for vote in fetch_collection(self.lp, self.lp_candidate.votes):
+            if vote.reviewer != self.lp.me:
+                continue
+            if vote.comment and vote.comment.vote:
+                return vote.comment.vote
+            else:
+                break
+        return None
+
+    def add_result(self, result, content=None, review_type="test results"):
+        """
+        Add this test result to the launchpad candidate.
+
+        @param content: if None, add the default comment content, which is a
+                        combination of test results and metadata.  Otherwise,
+                        add content given to the comment.
+        @param review_type: add this string as the review_type given.
+        @param result: dict containing the test result.
+        """
         if not self.readonly:
             log.debug("-> Adding result: %s to %s" % (
                 result['status'], self.lp_candidate.web_link))
@@ -244,11 +264,13 @@
             # my results don't claim a team membership review, for instance.
             if not self.is_reviewer_added():
                 self.add_reviewer()
+            if content is None:
+                content = format_test_result(result)
             self.lp_candidate.createComment(
-                content=format_test_result(result),
+                content=content,
                 vote=self.vote_lookup[result["status"]],
                 subject="Latch Results: %(branch)s r%(revno)s" % result,
-                review_type="test results")
+                review_type=review_type)
         else:
             log.debug("-> Would be adding result: %s to %s" % (
                 result['status'], self.lp_candidate.web_link))
@@ -298,6 +320,12 @@
             log.warning("Skipping candidate: %s")
             return False
 
+        # Results are currently being tested.
+        if self.get_my_vote() == self.vote_lookup["Testing"]:
+            log.debug("-> Already being tested, vote == %s" % (
+                self.vote_lookup["Testing"]))
+            return False
+
         if revision_tested == 0:
             log.debug("-> results are stale, tested_revision == 0")
             return True
@@ -371,13 +399,16 @@
             "%s 2>&1" % command, shell=True,
             cwd=self.source_dir, env=self.environ).decode('utf-8')
 
-    def _verify(self):
-        """Run the tests in this environment, returns result dict."""
-        result = {
+    def _get_base_result(self):
+        return {
             "status": u"Fail",
             "command": self.verify_command,
             "branch": self.lp_candidate.source_branch.bzr_identity,
             "output": u""}
+
+    def _verify(self):
+        """Run the tests in this environment, returns result dict."""
+        result = self._get_base_result()
         result["revno"] = self._get_revno(self.source_dir)
         log.info("Running: %s" % result["command"])
         try:
@@ -407,6 +438,11 @@
 
     def execute_test(self):
         """Test the local copy of the code, report on results in the MP."""
+        result = self._get_base_result()
+        result["status"] = "Testing"
+        result["revno"] = 0
+        self.candidate.add_result(
+            result=result, content="", review_type="executing tests")
         result = self._verify()
         self._copy_artifacts()
         self.candidate.add_result(result)

=== modified file 'test_latch.py'
--- test_latch.py	2015-10-01 16:16:38 +0000
+++ test_latch.py	2016-03-28 14:06:24 +0000
@@ -61,17 +61,16 @@
         candidate.all_comments.append(comment)
 
 
-def add_votes(candidate, user=None):
-    """
-    Add some fake votes to a candidate. The last vote will be
-    assigned to the specified user.
-    """
-    candidate.votes = [MagicMock(), MagicMock(), MagicMock()]
-    if user is not None:
-        candidate.votes[-1].reviewer = user
-
-
-def get_candidate(user):
+def add_vote(candidate, reviewer="nobody", vote="Approved"):
+    """Add a fake vote to a candidate."""
+    this_vote = MagicMock()
+    this_vote.reviewer = reviewer
+    this_vote.comment.vote = vote
+    if not candidate.votes:
+        candidate.votes = []
+    candidate.votes.append(this_vote)
+
+def get_candidate(reviewer):
     """Return a fake candidate that needs testing."""
     candidate = MagicMock()
     candidate.self_link = "self_link"
@@ -79,8 +78,10 @@
     candidate.source_branch.revision_count = 2
     candidate.target_branch.bzr_identity = "target_branch_id"
     candidate.source_branch.bzr_identity = "source_branch_id"
-    add_results(candidate, 1, user)
-    add_votes(candidate, user)
+    add_results(candidate, 1, reviewer)
+    add_vote(candidate)
+    add_vote(candidate)
+    add_vote(candidate, reviewer)
     return candidate
 
 
@@ -251,6 +252,7 @@
         self.lp = MagicMock()
         self.lp.me = FAKE_ME
         self.candidate = latch.Candidate(self.lp, MagicMock())
+        self.candidate.lp_candidate.votes = []
 
     def test_add_reviewer_readonly(self):
         """If readonly is set, nothing should be returned."""
@@ -266,27 +268,35 @@
 
     def test_is_reviewer_added(self):
         """lp user is added to a candidate."""
-        add_votes(self.candidate.lp_candidate, self.lp.me)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate, self.lp.me)
         self.assertTrue(self.candidate.is_reviewer_added())
 
     def test_is_reviewer_added_negative(self):
         """lp user is not added to a candidate."""
         not_me = MagicMock()
         not_me.name = "another-fake-user"
-        add_votes(self.candidate.lp_candidate, not_me)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate, not_me)
         self.assertFalse(self.candidate.is_reviewer_added())
 
     def test_does_candidate_need_testing_stale_revision(self):
         """Test results revno < source_branch, multiple comments."""
         self.candidate.lp_candidate.source_branch.revision_count = 2
-        add_votes(self.candidate.lp_candidate, self.lp.me)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate, self.lp.me)
         add_results(self.candidate.lp_candidate, 1, self.lp.me)
         self.assertTrue(self.candidate.needs_testing())
 
     def test_does_candidate_need_testing_false(self):
         """Test results revno == source branch, multiple comments."""
         self.candidate.lp_candidate.source_branch.revision_count = 3
-        add_votes(self.candidate.lp_candidate, self.lp.me)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate, self.lp.me)
         add_results(self.candidate.lp_candidate, 3, self.lp.me)
         self.assertFalse(self.candidate.needs_testing())
 
@@ -325,6 +335,18 @@
             "This is a test.")
         self.assertTrue(self.candidate.are_test_results_stale())
 
+    def test_are_test_results_stale_currently_testing(self):
+        """My vote is abstain, which means I'm testing."""
+        self.candidate.lp_candidate.source_branch.revision_count = 1
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(
+            self.candidate.lp_candidate,
+            vote="Abstain",
+            reviewer=self.lp.me)
+        self.assertFalse(self.candidate.are_test_results_stale())
+        self.assertInLogs("Already being tested")
+
     def test_are_test_results_stale_lp_error(self):
         """Test results revno == source branch."""
         self.candidate.lp_candidate.source_branch.revision_count = 1
@@ -362,6 +384,22 @@
         self.assertEqual(self.candidate._get_revision_from_comment(comment), 0)
         self.assertInLogs("revno parse error, skipping")
 
+    def test_get_my_vote_none(self):
+        """Test none is returned if I have not voted, others have."""
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        vote = self.candidate.get_my_vote()
+        self.assertIsNone(vote)
+
+    def test_get_my_vote_exists_multiple_votes(self):
+        """Test vote is returned if I have voted."""
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate)
+        add_vote(self.candidate.lp_candidate, self.lp.me)
+        vote = self.candidate.get_my_vote()
+        self.assertEqual(vote, "Approved")
+
     def test_add_result_readonly(self):
         """if readonly, should NOT try to create a comment."""
         result = {