Issue 3894 - Add tests for the update_issues hook

sitescripts/hg/test/update_issues.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import mock
 import unittest
 
 import sitescripts.hg.bin.update_issues as update_issues
 
-def _create_mock_repo(node, message):

[Vasily Kuznetsov, 2016/04/07 14:17:22]

It seems that `repo` is essentially a list of changes as far as the hook is
concerned. Could we just `return [mock_change]`?

This approach would also work well if we want to test the hook for multiple
changes, which I think might be a useful test case too.

[Felix Dahlke, 2016/04/07 15:42:00]

It's unfortunately not just a list, there are some other properties/methods on
it that the code uses. We could of course change _create_mock_repo to accept a
list of changes. I thought about that and then thought I'll just keep it simple,
until we actually need to test something with multiple changes. But no strong
opinion, if you prefer that (or have another idea).

[Vasily Kuznetsov, 2016/04/07 21:16:49]

Ok, yeah, I see, it does repo.url(). Then you'd have to subclass list to make it
work and your way is easier for just one change. Makes sense.

+def _create_mock_repo(message):
   mock_repo = mock.MagicMock()
   mock_repo.__len__.return_value = 1
   mock_change = mock.MagicMock()
-  mock_change.rev.return_value = node
+  mock_change.rev.return_value = 0
   mock_change.description.return_value = message
   mock_repo.__getitem__.return_value = mock_change
   return mock_repo
 
 class TestUpdateIssues(unittest.TestCase):
   def setUp(self):
     self.ui = mock.Mock()
 
   @mock.patch("xmlrpclib.ServerProxy")
-  def test_commits_with_invalid_message_format_ignored(self, mock_server_proxy):

[Vasily Kuznetsov, 2016/04/07 14:17:22]

Nit: this line is 80 characters long. PEP8 recommends 79 max.

[Felix Dahlke, 2016/04/07 15:42:00]

Done.

+  def test_commits_with_invalid_message_format_ignored(self,
+                                                       mock_server_proxy):
     messages = ["", "Issue #1337", "Tissue 1337", "Issue 13b"]
     for message in messages:
-      node = 0

[Vasily Kuznetsov, 2016/04/07 14:17:22]

I think the `node` variable is a bit redundant here and in other tests. You
essentially always create a mock repo with one change and then test the hook on
this change. So `_create_mock_repo` doesn't really need `node` and you could
just give `0` to the hook as a last parameter, I think it's clear enough.

[Felix Dahlke, 2016/04/07 15:42:00]

Done.

-      mock_repo = _create_mock_repo(node, message)
-      update_issues.hook(self.ui, mock_repo, node)
+      mock_repo = _create_mock_repo(message)
+      update_issues.hook(self.ui, mock_repo, 0)
       self.ui.warn.assert_called_once()

[Vasily Kuznetsov, 2016/04/07 14:17:22]

Does this work for you? When I run the test it fails on the second iteration of
the loop, which actually kind of makes sense: `self.ui` is only initialised once
per test and is not reset between the iterations.

Maybe we have different version of `mock` (mine is 2.0.0)?

[Felix Dahlke, 2016/04/07 15:42:00]

Done.

I originally did reset the mocks. Then checked if they were needed and removed
them because they weren't in mock 1.0.1 for some reason. But in mock 2.0.0 it
fails indeed, fixed that.

       self.assertFalse(mock_server_proxy.called)
+      self.ui.warn.reset_mock()
 
   @mock.patch("xmlrpclib.ServerProxy")
   def test_noissue_commits_ignored(self, mock_server_proxy):
     messages = ["Noissue", "noissue"]
     for message in messages:
-      node = 0
-      mock_repo = _create_mock_repo(node, message)
-      update_issues.hook(self.ui, mock_repo, node)
+      mock_repo = _create_mock_repo(message)
+      update_issues.hook(self.ui, mock_repo, 0)
       self.assertFalse(self.ui.warn.called)
       self.assertFalse(mock_server_proxy.called)
 
   @mock.patch("xmlrpclib.ServerProxy")
   def test_single_issue_referenced(self, mock_server_proxy):

[Vasily Kuznetsov, 2016/04/07 14:17:22]

This also fails for me. Looks like the same problem as above.

[Felix Dahlke, 2016/04/07 15:42:00]

Done.

     server_proxy_instance = mock_server_proxy.return_value
     messages = ["Issue 1337", "issue 1337"]
     for message in messages:
-      node = 0
-      mock_repo = _create_mock_repo(node, message)
-      update_issues.hook(self.ui, mock_repo, node)
+      mock_repo = _create_mock_repo(message)
+      update_issues.hook(self.ui, mock_repo, 0)
       self.assertFalse(self.ui.warn.called)
       server_proxy_instance.ticket.update.assert_called_once()
       self.assertEqual(server_proxy_instance.ticket.update.call_args[0][0],
                        1337)
+      server_proxy_instance.reset_mock()
 
   @mock.patch("xmlrpclib.ServerProxy")
   def test_multiple_issues_referenced(self, mock_server_proxy):
     server_proxy_instance = mock_server_proxy.return_value
-    node = 0
-    mock_repo = _create_mock_repo(node, "Issue 1337, issue 2448")
-    update_issues.hook(self.ui, mock_repo, node)
+    mock_repo = _create_mock_repo("Issue 1337, issue 2448")
+    update_issues.hook(self.ui, mock_repo, 0)
     self.assertFalse(self.ui.warn.called)
     calls = server_proxy_instance.ticket.update.call_args_list
     self.assertEqual(len(calls), 2)
     self.assertEqual(calls[0][0][0], 1337)
     self.assertEqual(calls[1][0][0], 2448)
 
 if __name__ == "__main__":
   unittest.main()