Issue 7334 - Remove handling of "Fixes XXXX - ..." commit messages

sitescripts/hg/test/test_update_issues.py

Index: sitescripts/hg/test/test_update_issues.py
===================================================================
rename from sitescripts/hg/test/update_issues.py
rename to sitescripts/hg/test/test_update_issues.py
--- a/sitescripts/hg/test/update_issues.py
+++ b/sitescripts/hg/test/test_update_issues.py
@@ -8,235 +8,86 @@
 # 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 ConfigParser
+"""Test Mercurial Trac integration hook."""
+
 import mock
-import unittest
+import pytest
 
 import sitescripts.hg.bin.update_issues as update_issues
 
 
-def _issue(component, milestone='', can_resolve=False):
-    issue = {
-        'attrs': {'_ts': 1, 'milestone': milestone, 'component': component},
-        'actions': [['leave', '', '', []]],
-    }
-    if can_resolve:
-        issue['actions'].append(['resolve', '', '', []])
-    return issue
-
+class RepoMock(list):
+    """Very simple mock of a Mercurial repository."""

[mathias, 2019/05/14 09:32:02]

If you call the class "MercurialRepositoryMock" there will be no comment needed.
Especially not something that tells the reader how simple it is.

[Vasily Kuznetsov, 2019/05/14 10:29:00]

Done.

 
-ISSUES = {
-    1337: _issue(component='one', can_resolve=True),
-    2448: _issue(component='two'),
-    3559: _issue(component='one', milestone='other'),
-    4670: _issue(component='three', can_resolve=True),
-    5781: _issue(component='four', can_resolve=True),
-}
+    def __init__(self, message):
+        change_mock = mock.MagicMock()
+        change_mock.rev.return_value = 0
+        change_mock.hex.return_value = '000011112222'

[mathias, 2019/05/14 09:32:02]

This magic number string should be a constant.

[Vasily Kuznetsov, 2019/05/14 10:29:00]

Done.

+        change_mock.description.return_value = message
+        list.__init__(self, [change_mock])
 
-MILESTONES = {
-    'completed': {'completed': True, 'name': 'completed'},
-    'current': {'completed': None, 'name': 'current'},
-    'other': {'completed': None, 'name': 'other'},
-}
+    def url(self):
+        return 'repo/mock'
 
 
-class _TestBase(unittest.TestCase):
-    """Base class for hook tests that prepares the environment."""
-
-    def _patchWith(self, target, return_value):
-        patcher = mock.patch(target, return_value=return_value)
-        patcher.start()
-        self.addCleanup(patcher.stop)
-
-    def _create_mock_milestone_multicall(self):
-        ret = []
-        multicall = mock.Mock(return_value=ret)
-        multicall.ticket.milestone.get = lambda i: ret.append(MILESTONES[i])
-        return multicall
+@pytest.fixture
+def server_proxy_mock(mocker):
+    return mocker.patch('xmlrpclib.ServerProxy')
 
-    def _mock_trac(self):
-        trac = mock.Mock()
-        trac.ticket.get = lambda i: [i, mock.ANY, mock.ANY, ISSUES[i]['attrs']]
-        trac.ticket.getActions = lambda i: ISSUES[i]['actions']
-        trac.ticket.milestone.getAll = lambda: sorted(MILESTONES.keys())
-        self.trac_proxy_mock = trac
-        self._patchWith('xmlrpclib.ServerProxy', trac)
-        self._patchWith('xmlrpclib.MultiCall',
-                        self._create_mock_milestone_multicall())
 
-    def _mock_config(self):
-        config = ConfigParser.ConfigParser()
-        config.add_section('hg')
-        config.set('hg', 'trac_xmlrpc_url', 'foo')
-        config.set('hg', 'issue_url_template', '#{id}')
-        config.add_section('hg_module_milestones')
-        config.set('hg_module_milestones', 'one', '.*')
-        config.set('hg_module_milestones', 'two', 'other')
-        config.set('hg_module_milestones', 'four', 'completed')
-        self._patchWith('sitescripts.hg.bin.update_issues.get_config', config)
-
-    def setUp(self):
-        self.ui = mock.Mock()
-        self._mock_trac()
-        self._mock_config()
+@pytest.fixture
+def ui_mock():
+    return mock.MagicMock()
 
 
-class _MockRepo(list):
-    def __init__(self, commit_messages):
-        list.__init__(self)
-        for i, message in enumerate(commit_messages):
-            mock_commit = mock.MagicMock()
-            mock_commit.rev.return_value = i
-            mock_commit.hex.return_value = '{:010x}'.format(i) + '0' * 30
-            mock_commit.description.return_value = message
-            self.append(mock_commit)
-        self.changelog = mock.Mock()
-        self.changelog.findmissingrevs = self._findmissingrevs
-
-    def _findmissingrevs(self, olds, news):
-        return range(olds[0] + 1, news[0] + 1)
-
-    def __getitem__(self, commit_id):
-        if isinstance(commit_id, str):
-            return [commit for commit in self if commit.hex() == commit_id][0]
-        return list.__getitem__(self, commit_id)
-
-    def url(self):
-        return 'mock/repo'
+@pytest.mark.parametrize('message', [
+    '', 'Issue #1337', 'Tissue 1337', 'Issue 13b',
+])
+def test_invalid_message_format(message, ui_mock, server_proxy_mock):
+    """Check that commits with invalid messages are ignored with a warning."""
+    repo_mock = RepoMock(message)
+    update_issues.hook(ui_mock, repo_mock, 0)
+    ui_mock.warn.assert_called_once()
+    assert not server_proxy_mock.called
 
 
-class TestChangegroupHook(_TestBase):
-
-    def _run_hook(self, commit_messages, warning_count=0, update_count=0):
-        repo = _MockRepo(commit_messages)
-        update_issues.changegroup_hook(self.ui, repo, 0)
-        warnings = self.ui.warn.call_args_list
-        updates = self.trac_proxy_mock.ticket.update.call_args_list
-        self.assertEqual(len(warnings), warning_count)
-        self.assertEqual(len(updates), update_count)
-        return updates
-
-    def test_commits_with_invalid_message_format_ignored(self):
-        self._run_hook([
-            '',
-            'Issue #1337 - Extraneous characters in issue number',
-            'Issue 1337',  # No dash, no message.
-            'Issue 1337: Colon instead of dash',
-            'Noissue no dash',
-            'Issue 1337-No space around dash',
-            'Fixes 1337 no dash',
-        ], warning_count=7)
-
-    def test_noissue_commits_ignored(self):
-        self._run_hook(['Noissue - Foo', 'noissue - Bar'])  # No updates.
-
-    def test_single_issue_referenced(self):
-        updates = self._run_hook(['Issue 1337 - Foo'], update_count=1)
-        self.assertEqual(updates[0][0][0], 1337)
-
-    def test_multiline_commit_message(self):
-        updates = self._run_hook(['Issue 1337 - Foo\nBar',
-                                  'Issue 1337 - Bar.\nBaz',
-                                  'Fixes 2448 - Foo\n\nBar',
-                                  'Fixes 2448 - Bar\n \nBaz'],
-                                 update_count=2)
-        comment_1337 = updates[0][0][1]
-        self.assertIn('Issue 1337 - Foo...]', comment_1337)
-        self.assertIn('Issue 1337 - Bar...]', comment_1337)
-        comment_2448 = updates[1][0][1]
-        self.assertIn('Fixes 2448 - Foo]', comment_2448)
-        self.assertIn('Fixes 2448 - Bar]', comment_2448)
-
-    def test_multiline_commit_message_crlf(self):
-        updates = self._run_hook(['Issue 1337 - Foo\r\nBar',
-                                  'Issue 1337 - Bar.\r\nBaz',
-                                  'Fixes 2448 - Foo\r\n\r\nBar',
-                                  'Fixes 2448 - Bar\r\n \r\nBaz'],
-                                 update_count=2)
-        comment_1337 = updates[0][0][1]
-        self.assertIn('Issue 1337 - Foo...]', comment_1337)
-        self.assertIn('Issue 1337 - Bar...]', comment_1337)
-        comment_2448 = updates[1][0][1]
-        self.assertIn('Fixes 2448 - Foo]', comment_2448)
-        self.assertIn('Fixes 2448 - Bar]', comment_2448)
-
-    def test_missing_issue_referenced(self):
-        self._run_hook(['Issue 42 - Bar'], warning_count=1)
-
-    def test_multiple_issues_referenced(self):
-        updates = self._run_hook(['Issue 1337, fixes 2448 - Foo'],
-                                 update_count=2)
-        self.assertEqual(updates[0][0][0], 1337)
-        self.assertEqual(updates[1][0][0], 2448)
-
-    def test_multiple_commits_for_issue(self):
-        updates = self._run_hook(['Issue 1337 - Foo', 'Fixes 1337 - Bar'],
-                                 update_count=1)
-        comment = updates[0][0][1]
-        self.assertIn('000000000000', comment)
-        self.assertIn('000000000100', comment)
+@pytest.mark.parametrize('message', ['Noissue - foobar', 'noissue'])
+def test_noissue(message, ui_mock, server_proxy_mock):
+    """Check that noissue commits are ignored without warning."""
+    repo_mock = RepoMock(message)
+    update_issues.hook(ui_mock, repo_mock, 0)
+    assert not ui_mock.warn.called
+    assert not server_proxy_mock.called
 
 
-class TestPushkeyHook(_TestBase):
-
-    def _run_hook(self, commit_messages, bookmark='master',
-                  warning_count=0, update_count=0):
-        repo = _MockRepo(['Base', 'Old'] + commit_messages)
-        update_issues.pushkey_hook(self.ui, repo,
-                                   namespace='bookmarks', key=bookmark,
-                                   old=1, new=1 + len(commit_messages))
-        warnings = self.ui.warn.call_args_list
-        updates = self.trac_proxy_mock.ticket.update.call_args_list
-        self.assertEqual(len(warnings), warning_count)
-        self.assertEqual(len(updates), update_count)
-        return updates
-
-    def _check_update(self, update, issue_id, action='resolve',
-                      milestone='current'):
-        self.assertEqual(update[0][0], issue_id)
-        changes = update[0][2]
-        self.assertEqual(changes['action'], action)
-        if milestone is None:
-            self.assertNotIn('milestone', changes)
-        else:
-            self.assertEqual(changes['milestone'], milestone)
-
-    def test_move_other_bookmark(self):
-        self._run_hook(['Fixes 1337 - Foo'], bookmark='other')  # No updates.
-
-    def test_one_issue_fixed(self):
-        updates = self._run_hook(['Fixes 1337 - Foo'], update_count=1)
-        self._check_update(updates[0], 1337)
-
-    def test_fix_closed_issue(self):
-        updates = self._run_hook(['fixes 2448 - Foo'], update_count=1)
-        self._check_update(updates[0], 2448, action='leave', milestone='other')
-
-    def test_fix_issue_noregexp(self):
-        updates = self._run_hook(['Fixes 4670 - Foo'], update_count=1)
-        self._check_update(updates[0], 4670, milestone=None)
-
-    def test_fix_issue_no_matching_milestones(self):
-        updates = self._run_hook(['Fixes 5781 - Foo'], update_count=1)
-        self._check_update(updates[0], 5781, milestone=None)
-
-    def test_fix_many(self):
-        updates = self._run_hook(['Fixes 1337 - Foo', 'Fixes 2448 - Bar'],
-                                 update_count=2)
-        self._check_update(updates[0], 1337)
-        self._check_update(updates[1], 2448, action='leave', milestone='other')
-
-    def test_fix_nonexistent(self):
-        self._run_hook(['Fixes 7331 - Foo'], warning_count=1)
-
-    def test_fix_closed_with_assigned_milestone(self):
-        self._run_hook(['fixes 3559 - Foo'])  # No updates.
+@pytest.mark.parametrize('message', ['Issue 1337 - foo', 'issue 1337 - foo'])
+def test_single_issue(message, ui_mock, server_proxy_mock):
+    """Check that a commit referencing a single issue gets linked."""
+    server_proxy = server_proxy_mock.return_value
+    repo_mock = RepoMock(message)
+    update_issues.hook(ui_mock, repo_mock, 0)
+    assert not ui_mock.warn.called
+    server_proxy.ticket.update.assert_called_once()
+    call = server_proxy.ticket.update.call_args
+    assert call[0][0] == 1337
+    comment = call[0][1]
+    assert comment.startswith('A commit referencing this issue has landed')
+    assert '000011112222' in comment

[mathias, 2019/05/14 09:32:02]

This magic number string should be a constant.

[Vasily Kuznetsov, 2019/05/14 10:29:00]

Done.

+    assert comment.endswith('- foo]')
 
 
-if __name__ == '__main__':
-    unittest.main()
+def test_multiple_issues(ui_mock, server_proxy_mock):
+    """Check that a commit referencing two issues gets linked twice."""
+    server_proxy = server_proxy_mock.return_value
+    repo_mock = RepoMock('Issue 1337, issue 2448')
+    update_issues.hook(ui_mock, repo_mock, 0)
+    assert not ui_mock.warn.called
+    calls = server_proxy.ticket.update.call_args_list
+    assert len(calls) == 2
+    assert calls[0][0][0] == 1337
+    assert calls[1][0][0] == 2448