From e95791dda3d57243be2510ac820555a4c1847d82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Piliszek?= Date: Fri, 23 Aug 2019 18:50:06 +0200 Subject: [PATCH] Make files matcher match changes with no files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds exceptions that were already applied to the irrelevant files matcher. It implements some version of option D discussed in [1]. Added comments to both matchers implementations. The FileMatcher is now a stub holding relevant regex only. [1] https://review.opendev.org/660856 Change-Id: Icf5df145e4cd351ffd04b1e417e9f7ab8c5ccd12 Story: 2005040 Task: 29531 Signed-off-by: Radosław Piliszek --- ...r-pass-when-no-files-f3b4466d8107895e.yaml | 6 ++ tests/unit/test_change_matcher.py | 57 +++++++++++-------- zuul/change_matcher.py | 34 ++++++++--- zuul/model.py | 2 +- 4 files changed, 66 insertions(+), 33 deletions(-) create mode 100644 releasenotes/notes/file-matcher-pass-when-no-files-f3b4466d8107895e.yaml diff --git a/releasenotes/notes/file-matcher-pass-when-no-files-f3b4466d8107895e.yaml b/releasenotes/notes/file-matcher-pass-when-no-files-f3b4466d8107895e.yaml new file mode 100644 index 0000000000..cae7437ff0 --- /dev/null +++ b/releasenotes/notes/file-matcher-pass-when-no-files-f3b4466d8107895e.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Files matcher has been fixed to act like irrelevant files matcher + when no files are present in the tested change, i.e. it now matches + such empty changes instead of rejecting them. diff --git a/tests/unit/test_change_matcher.py b/tests/unit/test_change_matcher.py index 3d5345f5a9..b4e3f0c45a 100644 --- a/tests/unit/test_change_matcher.py +++ b/tests/unit/test_change_matcher.py @@ -70,24 +70,6 @@ class TestBranchMatcher(BaseTestMatcher): self.assertFalse(self.matcher.matches(self.change)) -class TestFileMatcher(BaseTestMatcher): - - def setUp(self): - super(TestFileMatcher, self).setUp() - self.matcher = cm.FileMatcher('filename') - - def test_matches_returns_true(self): - self.change.files = ['filename'] - self.assertTrue(self.matcher.matches(self.change)) - - def test_matches_returns_false_when_no_files(self): - self.assertFalse(self.matcher.matches(self.change)) - - def test_matches_returns_false_when_files_attr_missing(self): - delattr(self.change, 'files') - self.assertFalse(self.matcher.matches(self.change)) - - class TestAbstractMatcherCollection(BaseTestMatcher): def test_str(self): @@ -99,17 +81,20 @@ class TestAbstractMatcherCollection(BaseTestMatcher): self.assertEqual(repr(matcher), '') -class TestMatchAllFiles(BaseTestMatcher): - - def setUp(self): - super(TestMatchAllFiles, self).setUp() - self.matcher = cm.MatchAllFiles([cm.FileMatcher('^docs/.*$')]) +class BaseTestFilesMatcher(BaseTestMatcher): def _test_matches(self, expected, files=None): if files is not None: self.change.files = files self.assertEqual(expected, self.matcher.matches(self.change)) + +class TestMatchAllFiles(BaseTestFilesMatcher): + + def setUp(self): + super(TestMatchAllFiles, self).setUp() + self.matcher = cm.MatchAllFiles([cm.FileMatcher('^docs/.*$')]) + def test_matches_returns_false_when_files_attr_missing(self): delattr(self.change, 'files') self._test_matches(False) @@ -133,6 +118,32 @@ class TestMatchAllFiles(BaseTestMatcher): self._test_matches(True, files=['docs/foo']) +class TestMatchAnyFiles(BaseTestFilesMatcher): + + def setUp(self): + super(TestMatchAnyFiles, self).setUp() + self.matcher = cm.MatchAnyFiles([cm.FileMatcher('^docs/.*$')]) + + def test_matches_returns_true_when_files_attr_missing(self): + delattr(self.change, 'files') + self._test_matches(True) + + def test_matches_returns_true_when_no_files(self): + self._test_matches(True) + + def test_matches_returns_true_when_only_commit_message(self): + self._test_matches(True, files=['/COMMIT_MSG']) + + def test_matches_returns_true_when_some_files_match(self): + self._test_matches(True, files=['/COMMIT_MSG', 'docs/foo', 'foo/bar']) + + def test_matches_returns_true_when_single_file_matches(self): + self._test_matches(True, files=['docs/foo']) + + def test_matches_returns_false_when_no_matching_files(self): + self._test_matches(False, files=['/COMMIT_MSG', 'foo/bar']) + + class TestMatchAll(BaseTestMatcher): def test_matches_returns_true(self): diff --git a/zuul/change_matcher.py b/zuul/change_matcher.py index dc4fe37c34..a788369443 100644 --- a/zuul/change_matcher.py +++ b/zuul/change_matcher.py @@ -85,13 +85,7 @@ class ImpliedBranchMatcher(AbstractChangeMatcher): class FileMatcher(AbstractChangeMatcher): - def matches(self, change): - if not hasattr(change, 'files'): - return False - for file_ in change.files: - if self.regex.match(file_): - return True - return False + pass class AbstractMatcherCollection(AbstractChangeMatcher): @@ -113,7 +107,7 @@ class AbstractMatcherCollection(AbstractChangeMatcher): return self.__class__(self.matchers[:]) -class MatchAllFiles(AbstractMatcherCollection): +class AbstractMatchFiles(AbstractMatcherCollection): commit_regex = re.compile('^/COMMIT_MSG$') @@ -121,9 +115,13 @@ class MatchAllFiles(AbstractMatcherCollection): def regexes(self): for matcher in self.matchers: yield matcher.regex - yield self.commit_regex + + +class MatchAllFiles(AbstractMatchFiles): def matches(self, change): + # NOTE(yoctozepto): make irrelevant files matcher match when + # there are no files to check - return False (NB: reversed) if not (hasattr(change, 'files') and change.files): return False if len(change.files) == 1 and self.commit_regex.match(change.files[0]): @@ -134,11 +132,29 @@ class MatchAllFiles(AbstractMatcherCollection): if regex.match(file_): matched_file = True break + if self.commit_regex.match(file_): + matched_file = True if not matched_file: return False return True +class MatchAnyFiles(AbstractMatchFiles): + + def matches(self, change): + # NOTE(yoctozepto): make files matcher match when + # there are no files to check - return True + if not (hasattr(change, 'files') and change.files): + return True + if len(change.files) == 1 and self.commit_regex.match(change.files[0]): + return True + for file_ in change.files: + for regex in self.regexes: + if regex.match(file_): + return True + return False + + class MatchAll(AbstractMatcherCollection): def matches(self, change): diff --git a/zuul/model.py b/zuul/model.py index 30739a1a35..4a6c701325 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -1420,7 +1420,7 @@ class Job(ConfigObject): matchers = [] for fn in files: matchers.append(change_matcher.FileMatcher(fn)) - self.file_matcher = change_matcher.MatchAny(matchers) + self.file_matcher = change_matcher.MatchAnyFiles(matchers) def setIrrelevantFileMatcher(self, irrelevant_files): # Set the irrelevant file matcher to match any of the change files