pre-upload: add unittests for commit message checks
The Change-Id checker was a little too forgiving, so tighten that up.
BUG=None
TEST=`./pre-upload_unittest.py` passes
Change-Id: I338ac029d51584dea87816aeb8f12647f7720589
Reviewed-on: https://chromium-review.googlesource.com/226681
Commit-Queue: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Steve Fung <stevefung@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index 064bb8f..b44cbc9 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -707,9 +707,10 @@
def _check_change_has_proper_changeid(_project, commit):
"""Verify that Change-ID is present in last paragraph of commit message."""
+ CHANGE_ID_RE = r'\nChange-Id: I[a-f0-9]+\n'
desc = _get_commit_desc(commit)
- loc = desc.rfind('\nChange-Id:')
- if loc == -1 or re.search('\n\s*\n\s*\S+', desc[loc:]):
+ m = re.search(CHANGE_ID_RE, desc)
+ if not m or desc[m.end():].strip():
return HookFailure('Change-Id must be in last paragraph of description.')
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 0a9959e..eddbb7e 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -471,5 +471,176 @@
self.assertNotEqual(None, pre_upload._check_license('proj', 'sha1'))
+class CommitMessageTestCase(cros_test_lib.MockTestCase):
+ """Test case for funcs that check commit messages."""
+
+ def setUp(self):
+ self.msg_mock = self.PatchObject(pre_upload, '_get_commit_desc')
+
+ @staticmethod
+ def CheckMessage(_project, _commit):
+ raise AssertionError('Test class must declare CheckMessage')
+ # This dummy return is to silence pylint warning W1111 so we don't have to
+ # enable it for all the call sites below.
+ return 1 # pylint: disable=W0101
+
+ def assertMessageAccepted(self, msg, project='project', commit='1234'):
+ """Assert _check_change_has_bug_field accepts |msg|."""
+ self.msg_mock.return_value = msg
+ ret = self.CheckMessage(project, commit)
+ self.assertEqual(ret, None)
+
+ def assertMessageRejected(self, msg, project='project', commit='1234'):
+ """Assert _check_change_has_bug_field rejects |msg|."""
+ self.msg_mock.return_value = msg
+ ret = self.CheckMessage(project, commit)
+ self.assertTrue(isinstance(ret, errors.HookFailure))
+
+
+class CheckCommitMessageBug(CommitMessageTestCase):
+ """Tests for _check_change_has_bug_field."""
+
+ @staticmethod
+ def CheckMessage(project, commit):
+ return pre_upload._check_change_has_bug_field(project, commit)
+
+ def testNormal(self):
+ """Accept a commit message w/a valid BUG."""
+ self.assertMessageAccepted('\nBUG=chromium:1234\n')
+ self.assertMessageAccepted('\nBUG=chrome-os-partner:1234\n')
+
+ def testNone(self):
+ """Accept BUG=None."""
+ self.assertMessageAccepted('\nBUG=None\n')
+ self.assertMessageAccepted('\nBUG=none\n')
+ self.assertMessageRejected('\nBUG=NONE\n')
+
+ def testBlank(self):
+ """Reject blank values."""
+ self.assertMessageRejected('\nBUG=\n')
+ self.assertMessageRejected('\nBUG= \n')
+
+ def testNotFirstLine(self):
+ """Reject the first line."""
+ self.assertMessageRejected('BUG=None\n\n\n')
+
+ def testNotInline(self):
+ """Reject not at the start of line."""
+ self.assertMessageRejected('\n BUG=None\n')
+ self.assertMessageRejected('\n\tBUG=None\n')
+
+ def testOldTrackers(self):
+ """Reject commit messages using old trackers."""
+ self.assertMessageRejected('\nBUG=chromium-os:1234\n')
+
+ def testNoTrackers(self):
+ """Reject commit messages w/invalid trackers."""
+ self.assertMessageRejected('\nBUG=booga:1234\n')
+
+ def testMissing(self):
+ """Reject commit messages w/no BUG line."""
+ self.assertMessageRejected('foo\n')
+
+ def testCase(self):
+ """Reject bug lines that are not BUG."""
+ self.assertMessageRejected('\nbug=none\n')
+
+
+class CheckCommitMessageCqDepend(CommitMessageTestCase):
+ """Tests for _check_change_has_valid_cq_depend."""
+
+ @staticmethod
+ def CheckMessage(project, commit):
+ return pre_upload._check_change_has_valid_cq_depend(project, commit)
+
+ def testNormal(self):
+ """Accept valid CQ-DEPENDs line."""
+ self.assertMessageAccepted('\nCQ-DEPEND=CL:1234\n')
+
+ def testInvalid(self):
+ """Reject invalid CQ-DEPENDs line."""
+ self.assertMessageRejected('\nCQ-DEPEND=CL=1234\n')
+ self.assertMessageRejected('\nCQ-DEPEND=None\n')
+
+
+class CheckCommitMessageTest(CommitMessageTestCase):
+ """Tests for _check_change_has_test_field."""
+
+ @staticmethod
+ def CheckMessage(project, commit):
+ return pre_upload._check_change_has_test_field(project, commit)
+
+ def testNormal(self):
+ """Accept a commit message w/a valid TEST."""
+ self.assertMessageAccepted('\nTEST=i did it\n')
+
+ def testNone(self):
+ """Accept TEST=None."""
+ self.assertMessageAccepted('\nTEST=None\n')
+ self.assertMessageAccepted('\nTEST=none\n')
+
+ def testBlank(self):
+ """Reject blank values."""
+ self.assertMessageRejected('\nTEST=\n')
+ self.assertMessageRejected('\nTEST= \n')
+
+ def testNotFirstLine(self):
+ """Reject the first line."""
+ self.assertMessageRejected('TEST=None\n\n\n')
+
+ def testNotInline(self):
+ """Reject not at the start of line."""
+ self.assertMessageRejected('\n TEST=None\n')
+ self.assertMessageRejected('\n\tTEST=None\n')
+
+ def testMissing(self):
+ """Reject commit messages w/no TEST line."""
+ self.assertMessageRejected('foo\n')
+
+ def testCase(self):
+ """Reject bug lines that are not TEST."""
+ self.assertMessageRejected('\ntest=none\n')
+
+
+class CheckCommitMessageChangeId(CommitMessageTestCase):
+ """Tests for _check_change_has_proper_changeid."""
+
+ @staticmethod
+ def CheckMessage(project, commit):
+ return pre_upload._check_change_has_proper_changeid(project, commit)
+
+ def testNormal(self):
+ """Accept a commit message w/a valid Change-Id."""
+ self.assertMessageAccepted('foo\n\nChange-Id: I1234\n')
+
+ def testBlank(self):
+ """Reject blank values."""
+ self.assertMessageRejected('\nChange-Id:\n')
+ self.assertMessageRejected('\nChange-Id: \n')
+
+ def testNotFirstLine(self):
+ """Reject the first line."""
+ self.assertMessageRejected('TEST=None\n\n\n')
+
+ def testNotInline(self):
+ """Reject not at the start of line."""
+ self.assertMessageRejected('\n Change-Id: I1234\n')
+ self.assertMessageRejected('\n\tChange-Id: I1234\n')
+
+ def testMissing(self):
+ """Reject commit messages missing the line."""
+ self.assertMessageRejected('foo\n')
+
+ def testCase(self):
+ """Reject bug lines that are not Change-Id."""
+ self.assertMessageRejected('\nchange-id: I1234\n')
+ self.assertMessageRejected('\nChange-id: I1234\n')
+ self.assertMessageRejected('\nChange-ID: I1234\n')
+
+ def testEnd(self):
+ """Reject Change-Id's that are not last."""
+ self.assertMessageRejected('\nChange-Id: I1234\nbar\n')
+
+
if __name__ == '__main__':
cros_test_lib.main()
