Refactor DirectoryOwnersExtractor to make it more useful

DirectoryOwnersExtractor.find_owners_file is made more generic to
accommodate the new usage in ImportNotifier:
* It does not skip "empty" OWNERS (those without emails) any more, which
  allows "# WPT-NOTIFY" to be added to OWNERS files with comments only
  (e.g. "# TEAM", "# COMPONENT" lines; there are a bunch in wpt).
* It may now return LayoutTests/external/OWNERS. This is to make sure
  owners can always be found for WPT files so that ImportNotifier can
  generate and log bugs for all failures (but we will never turn on
  WPT-NOTIFY there).
* It can now take four variants of paths: (absolute, relative) X (file,
  directory), to make it easier to use.

Previous heuristics like skipping empty OWNERS are moved to list_owners
instead, which is only used for generating a list of owners in commit
messages and may eventually be removed.

Also improve the setup of the unit test of this module.

Bug: 765334
Change-Id: I01bd1fa85d2602299eaf1e0d8e0872b04b93a008
Reviewed-on: https://chromium-review.googlesource.com/822834
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523781}

third_party/WebKit/LayoutTests/external/OWNERS

+# Please do not remove this file. It is needed by webkitpy/w3c/directory_owners_extractor.py
+
 # TEAM: ecosystem-infra@chromium.org
 # COMPONENT: Blink>Infra>Ecosystem
 foolip@chromium.org

third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor.py

@@ -2,6 +2,13 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+"""A limited finder & parser for Chromium OWNERS files.
+
+This module is intended to be used within LayoutTests/external and is
+informative only. For authoritative uses, please rely on `git cl owners`.
+For example, it does not support directives other than email addresses.
+"""
+
 import collections
 import re
 
@@ -12,8 +19,8 @@ from webkitpy.common.system.filesystem import FileSystem
 
 # Format of OWNERS files can be found at //src/third_party/depot_tools/owners.py
 # In our use case (under external/wpt), we only process the first enclosing
-# non-empty OWNERS file for any given path (i.e. always assuming "set noparent"),
-# and we only care about lines that are valid email addresses.
+# OWNERS file for any given path (i.e. always assuming "set noparent"), and we
+# ignore "per-file:" lines, "file:" directives, etc.
 
 # Recognizes 'X@Y' email addresses. Very simplistic. (from owners.py)
 BASIC_EMAIL_REGEXP = r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$'
@@ -39,60 +46,59 @@ class DirectoryOwnersExtractor(object):
             owned directories (paths relative to the root of layout tests).
         """
         email_map = collections.defaultdict(set)
+        external_root_owners = self.finder.path_from_layout_tests('external', 'OWNERS')
         for relpath in changed_files:
+            # Try to find the first *non-empty* OWNERS file.
             absolute_path = self.finder.path_from_chromium_base(relpath)
-            if not absolute_path.startswith(self.finder.layout_tests_dir()):
-                continue
-            owners_file = self.find_owners_file(self.filesystem.dirname(relpath))
-            if not owners_file:
-                continue
+            owners = None
+            owners_file = self.find_owners_file(absolute_path)
+            while owners_file:
                 owners = self.extract_owners(owners_file)
+                if owners:
+                    break
+                # Found an empty OWNERS file. Try again from the parent directory.
+                absolute_path = self.filesystem.dirname(self.filesystem.dirname(owners_file))
+                owners_file = self.find_owners_file(absolute_path)
+            # Skip LayoutTests/external/OWNERS.
+            if not owners or owners_file == external_root_owners:
+                continue
+
             owned_directory = self.filesystem.dirname(owners_file)
             owned_directory_relpath = self.filesystem.relpath(owned_directory, self.finder.layout_tests_dir())
             email_map[tuple(owners)].add(owned_directory_relpath)
         return {owners: sorted(owned_directories) for owners, owned_directories in email_map.iteritems()}
 
-    # TODO(robertma): Do we really need to worry about empty OWNERS files?
-    def find_owners_file(self, start_directory):
-        """Find the first enclosing OWNERS file for a given path.
+    def find_owners_file(self, start_path):
+        """Finds the first enclosing OWNERS file for a given path.
 
-        Starting from the given directory, walks up the directory tree until the
-        first non-empty OWNERS file is found or LayoutTests/external is reached.
-        (OWNERS files with no valid emails are also considered empty.)
+        Starting from the given path, walks up the directory tree until the
+        first OWNERS file is found or LayoutTests/external is reached.
 
         Args:
-            start_directory: A relative path from the root of the repository, or
-                an absolute path.
+            start_path: A relative path from the root of the repository, or an
+                absolute path. The path can be a file or a directory.
 
         Returns:
-            The absolute path to the first non-empty OWNERS file found, or None
-            if not found.
+            The absolute path to the first OWNERS file found; None if not found
+            or if start_path is outside of LayoutTests/external.
         """
-        if self.filesystem.isabs(start_directory):
-            directory = start_directory
-        else:
-            directory = self.finder.path_from_chromium_base(start_directory)
+        abs_start_path = (start_path if self.filesystem.isabs(start_path)
+                          else self.finder.path_from_chromium_base(start_path))
+        directory = (abs_start_path if self.filesystem.isdir(abs_start_path)
+                     else self.filesystem.dirname(abs_start_path))
         external_root = self.finder.path_from_layout_tests('external')
-        # Changes to both LayoutTests/TestExpectations and the entire
-        # LayoutTests/FlagExpectations/ directory should be skipped and not
-        # raise an assertion.
-        if directory == self.finder.layout_tests_dir() or \
-           directory.startswith(self.finder.path_from_layout_tests('FlagExpectations')):
+        if not directory.startswith(external_root):
             return None
-        assert directory.startswith(external_root), '%s must start with %s' % (
-            directory, external_root)
-        while directory != external_root:
+        # Stop at LayoutTests, which is the parent of external_root.
+        while directory != self.finder.layout_tests_dir():
             owners_file = self.filesystem.join(directory, 'OWNERS')
             if self.filesystem.isfile(self.finder.path_from_chromium_base(owners_file)):
-                # TODO(robertma): Avoid parsing the file twice (find_owners_file
-                # only returns the path, which is read and parsed again later).
-                if self.extract_owners(owners_file):
                 return owners_file
             directory = self.filesystem.dirname(directory)
         return None
 
     def extract_owners(self, owners_file):
-        """Extract owners from an OWNERS file.
+        """Extracts owners from an OWNERS file.
 
         Args:
             owners_file: An absolute path to an OWNERS file.
@@ -110,7 +116,7 @@ class DirectoryOwnersExtractor(object):
         return addresses
 
     def extract_component(self, owners_file):
-        """Extract the component from an OWNERS file.
+        """Extracts the component from an OWNERS file.
 
         Args:
             owners_file: An absolute path to an OWNERS file.

third_party/WebKit/Tools/Scripts/webkitpy/w3c/directory_owners_extractor_unittest.py

@@ -14,77 +14,120 @@ REL_WPT_BASE = 'third_party/WebKit/LayoutTests/external/wpt'
 class DirectoryOwnersExtractorTest(unittest.TestCase):
 
     def setUp(self):
-        self.filesystem = MockFileSystem()
+        # We always have an OWNERS file at LayoutTests/external.
+        self.filesystem = MockFileSystem(files={
+            '/mock-checkout/third_party/WebKit/LayoutTests/external/OWNERS': 'ecosystem-infra@chromium.org'
+        })
         self.extractor = DirectoryOwnersExtractor(self.filesystem)
 
-    def test_list_owners(self):
-        self.filesystem.files = {
+    def _write_files(self, files):
+        # Use write_text_file instead of directly assigning to filesystem.files
+        # so that intermediary directories are correctly created, too.
+        for path, contents in files.iteritems():
+            self.filesystem.write_text_file(path, contents)
+
+    def test_list_owners_combines_same_owners(self):
+        self._write_files({
             ABS_WPT_BASE + '/foo/x.html': '',
             ABS_WPT_BASE + '/foo/OWNERS': 'a@chromium.org\nc@chromium.org\n',
             ABS_WPT_BASE + '/bar/x/y.html': '',
             ABS_WPT_BASE + '/bar/OWNERS': 'a@chromium.org\nc@chromium.org\n',
-            ABS_WPT_BASE + '/baz/x/y.html': '',
-            ABS_WPT_BASE + '/baz/x/OWNERS': 'b@chromium.org\n',
-            ABS_WPT_BASE + '/quux/x/y.html': '',
-        }
+        })
         changed_files = [
-            # Same owners:
             REL_WPT_BASE + '/foo/x.html',
             REL_WPT_BASE + '/bar/x/y.html',
-            # Same owned directories:
+        ]
+        self.assertEqual(
+            self.extractor.list_owners(changed_files),
+            {('a@chromium.org', 'c@chromium.org'): ['external/wpt/bar', 'external/wpt/foo']}
+        )
+
+    def test_list_owners_combines_same_directory(self):
+        self._write_files({
+            ABS_WPT_BASE + '/baz/x/y.html': '',
+            ABS_WPT_BASE + '/baz/x/y/z.html': '',
+            ABS_WPT_BASE + '/baz/x/OWNERS': 'foo@chromium.org\n',
+        })
+        changed_files = [
             REL_WPT_BASE + '/baz/x/y.html',
-            REL_WPT_BASE + '/baz/x/z.html',
-            # Owners not found:
-            REL_WPT_BASE + '/quux/x/y.html',
+            REL_WPT_BASE + '/baz/x/y/z.html',
         ]
         self.assertEqual(
             self.extractor.list_owners(changed_files),
-            {('a@chromium.org', 'c@chromium.org'): ['external/wpt/bar', 'external/wpt/foo'],
-             ('b@chromium.org',): ['external/wpt/baz/x']}
+            {('foo@chromium.org',): ['external/wpt/baz/x']}
         )
 
-    def test_find_owners_file_current_dir(self):
-        self.filesystem.files = {
+    def test_list_owners_skips_empty_owners(self):
+        self._write_files({
+            ABS_WPT_BASE + '/baz/x/y/z.html': '',
+            ABS_WPT_BASE + '/baz/x/y/OWNERS': '# Some comments\n',
+            ABS_WPT_BASE + '/baz/x/OWNERS': 'foo@chromium.org\n',
+        })
+        changed_files = [
+            REL_WPT_BASE + '/baz/x/y/z.html',
+        ]
+        self.assertEqual(
+            self.extractor.list_owners(changed_files),
+            {('foo@chromium.org',): ['external/wpt/baz/x']}
+        )
+
+    def test_list_owners_not_found(self):
+        self._write_files({
+            # Although LayoutTests/external/OWNERS exists, it should not be listed.
+            ABS_WPT_BASE + '/foo/bar.html': '',
+            # Files out of external.
+            '/mock-checkout/third_party/WebKit/LayoutTests/TestExpectations': '',
+            '/mock-checkout/third_party/WebKit/LayoutTests/OWNERS': 'foo@chromium.org',
+        })
+        changed_files = [
+            REL_WPT_BASE + '/foo/bar.html',
+            'third_party/WebKit/LayoutTests/TestExpectations',
+        ]
+        self.assertEqual(self.extractor.list_owners(changed_files), {})
+
+    def test_find_owners_file_at_current_dir(self):
+        self._write_files({
             ABS_WPT_BASE + '/foo/OWNERS': 'a@chromium.org'
-        }
+        })
         self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/foo'), ABS_WPT_BASE + '/foo/OWNERS')
 
-    def test_find_owners_file_ancestor(self):
-        self.filesystem.files = {
+    def test_find_owners_file_at_ancestor(self):
+        self._write_files({
             ABS_WPT_BASE + '/x/OWNERS': 'a@chromium.org',
             ABS_WPT_BASE + '/x/y/z.html': '',
-        }
+        })
         self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/x/y'), ABS_WPT_BASE + '/x/OWNERS')
 
-    def test_find_owners_file_not_found(self):
-        self.filesystem.files = {
-            ABS_WPT_BASE + '/foo/OWNERS': 'a@chromium.org',
-            '/mock-checkout/third_party/WebKit/LayoutTests/external/OWNERS': 'a@chromium.org',
-            ABS_WPT_BASE + '/x/y/z.html': '',
-        }
-        self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/x/y'), None)
-
-    def test_find_owners_file_skip_empty(self):
-        self.filesystem.files = {
-            ABS_WPT_BASE + '/x/OWNERS': 'a@chromium.org',
-            ABS_WPT_BASE + '/x/y/OWNERS': '# b@chromium.org',
+    def test_find_owners_file_stops_at_external_root(self):
+        self._write_files({
             ABS_WPT_BASE + '/x/y/z.html': '',
-        }
-        self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/x/y'), ABS_WPT_BASE + '/x/OWNERS')
-
-    def test_find_owners_file_absolute_path(self):
-        self.filesystem.files = {
-            ABS_WPT_BASE + '/foo/OWNERS': 'a@chromium.org'
-        }
-        self.assertEqual(self.extractor.find_owners_file(ABS_WPT_BASE + '/foo'), ABS_WPT_BASE + '/foo/OWNERS')
-
-    def test_find_owners_file_out_of_tree(self):
-        with self.assertRaises(AssertionError):
-            self.extractor.find_owners_file('third_party/WebKit/LayoutTests/other')
-        self.assertEqual(
-            self.extractor.find_owners_file('third_party/WebKit/LayoutTests'), None)
-        self.assertEqual(
-            self.extractor.find_owners_file('third_party/WebKit/LayoutTests/FlagExpectations/foo-bar'), None)
+        })
+        self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/x/y'),
+                         '/mock-checkout/third_party/WebKit/LayoutTests/external/OWNERS')
+
+    def test_find_owners_file_takes_four_kinds_of_paths(self):
+        owners_path = ABS_WPT_BASE + '/foo/OWNERS'
+        self._write_files({
+            owners_path: 'a@chromium.org',
+            ABS_WPT_BASE + '/foo/bar.html': '',
+        })
+        # Absolute paths of directories.
+        self.assertEqual(self.extractor.find_owners_file(ABS_WPT_BASE + '/foo'), owners_path)
+        # Relative paths of directories.
+        self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/foo'), owners_path)
+        # Absolute paths of files.
+        self.assertEqual(self.extractor.find_owners_file(ABS_WPT_BASE + '/foo/bar.html'), owners_path)
+        # Relative paths of files.
+        self.assertEqual(self.extractor.find_owners_file(REL_WPT_BASE + '/foo/bar.html'), owners_path)
+
+    def test_find_owners_file_out_of_external(self):
+        self._write_files({
+            '/mock-checkout/third_party/WebKit/LayoutTests/OWNERS': 'foo@chromium.org',
+            '/mock-checkout/third_party/WebKit/LayoutTests/other/some_file': '',
+        })
+        self.assertIsNone(self.extractor.find_owners_file('third_party/WebKit/LayoutTests'))
+        self.assertIsNone(self.extractor.find_owners_file('third_party/WebKit/LayoutTests/other'))
+        self.assertIsNone(self.extractor.find_owners_file('third_party'))
 
     def test_extract_owners(self):
         self.filesystem.files = {