Make files matcher match changes with no files

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 <radoslaw.piliszek@gmail.com>

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.

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), '<MatchAll>')
 
 
-class TestMatchAllFiles(BaseTestMatcher):
+class BaseTestFilesMatcher(BaseTestMatcher):
-
-    def setUp(self):
-        super(TestMatchAllFiles, self).setUp()
-        self.matcher = cm.MatchAllFiles([cm.FileMatcher('^docs/.*$')])
 
     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):

zuul/change_matcher.py

@@ -85,13 +85,7 @@ class ImpliedBranchMatcher(AbstractChangeMatcher):
 
 class FileMatcher(AbstractChangeMatcher):
 
-    def matches(self, change):
+    pass
-        if not hasattr(change, 'files'):
-            return False
-        for file_ in change.files:
-            if self.regex.match(file_):
-                return True
-        return False
 
 
 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):

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