Enable expand_owners_unittest.py on the testing bots.

Several of the tests depended on 'src' being a directory in the path to
expand_owners_unittest.py. However, on testing bots, this may not be the
case.

Note that _ExtractComponentViaDirmd() had to be mocked in several tests
due to http://crbug.com/1164985. One test was skipped due to this.

Also, removed testGetHigherLevelPathGivenPathInSrcDirectory since it
assumed 'src' would be in the path. It's okay to remove this because
we have a test checking that getting higher-level OWNERS file paths
doesn't result in a recursive loop.

Bug: 1126653
Change-Id: I5b6d4289dff3b5c9afe0fcc8249328d9059cedc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622320
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844079}

tools/metrics/histograms/expand_owners.py

@@ -17,8 +17,8 @@ _OWNERS = 'OWNERS'
 # module's directory, histograms, and the directory above tools, which may or
 # may not be src depending on the machine running the code, is up three
 # directory levels from the histograms directory.
-_DIR_ABOVE_TOOLS = [os.path.dirname(__file__), '..', '..', '..']
-_SRC = 'src/'
+DIR_ABOVE_TOOLS = [os.path.dirname(__file__), '..', '..', '..']
+SRC = 'src/'
 
 
 class Error(Exception):
@@ -91,7 +91,7 @@ def _IsWellFormattedFilePath(path):
   Args:
     path: The path to an OWNERS file, e.g. 'src/gin/OWNERS'.
   """
-  return path.startswith(_SRC) and path.endswith(_OWNERS)
+  return path.startswith(SRC) and path.endswith(_OWNERS)
 
 
 def _GetHigherLevelOwnersFilePath(path):
@@ -111,7 +111,7 @@ def _GetHigherLevelOwnersFilePath(path):
   # The highest directory that is searched for component information is one
   # directory lower than the directory above tools. Depending on the machine
   # running this code, the directory above tools may or may not be src.
-  path_to_limiting_dir = os.path.abspath(os.path.join(*_DIR_ABOVE_TOOLS))
+  path_to_limiting_dir = os.path.abspath(os.path.join(*DIR_ABOVE_TOOLS))
   limiting_dir = path_to_limiting_dir.split(os.sep)[-1]
   owners_file_limit = (os.sep).join([limiting_dir, _OWNERS])
   if path.endswith(owners_file_limit):
@@ -138,10 +138,10 @@ def _GetOwnersFilePath(path):
   if _IsWellFormattedFilePath(path):
     # _SRC is removed because the file system on the machine running the code
     # may not have a(n) src directory.
-    path_without_src = path[len(_SRC):]
+    path_without_src = path[len(SRC):]
 
     return os.path.abspath(
-        os.path.join(*(_DIR_ABOVE_TOOLS + path_without_src.split(os.sep))))
+        os.path.join(*(DIR_ABOVE_TOOLS + path_without_src.split(os.sep))))
 
   raise Error(
       'The given path {} is not well-formatted. Well-formatted paths begin '
@@ -184,7 +184,7 @@ def _ExtractEmailAddressesFromOWNERS(path, depth=0):
 
       elif first_word.startswith(directive):
         next_path = _GetOwnersFilePath(
-          os.path.join(_SRC, first_word[len(directive):]))
+            os.path.join(SRC, first_word[len(directive):]))
 
         if os.path.exists(next_path) and os.path.isfile(next_path):
           extracted_emails.extend(
@@ -241,7 +241,7 @@ def _ExtractComponentViaDirmd(path):
   """
   # Verify that the paths are absolute and the root is a parent of the
   # passed in path.
-  root_path = os.path.abspath(os.path.join(*_DIR_ABOVE_TOOLS))
+  root_path = os.path.abspath(os.path.join(*DIR_ABOVE_TOOLS))
   path = os.path.abspath(path)
   if not path.startswith(root_path):
     raise Error('Path {} is not a subpath of the root path {}.'.format(
@@ -250,7 +250,7 @@ def _ExtractComponentViaDirmd(path):
   dirmd_exe = 'dirmd'
   if sys.platform == 'win32':
     dirmd_exe = 'dirmd.bat'
-  dirmd_path = os.path.join(*(_DIR_ABOVE_TOOLS +
+  dirmd_path = os.path.join(*(DIR_ABOVE_TOOLS +
                               ['third_party', 'depot_tools', dirmd_exe]))
   dirmd = subprocess.Popen([dirmd_path, 'compute', '--root', root_path, path],
                            stdout=subprocess.PIPE)

tools/metrics/histograms/expand_owners_unittest.py

@@ -5,6 +5,7 @@
 import unittest
 
 import expand_owners
+import mock
 import os
 import shutil
 import tempfile
@@ -13,43 +14,38 @@ import xml.dom.minidom
 _DEFAULT_COMPONENT = '# COMPONENT: Default>Component'
 
 
-def _DirnameN(path, n):
-  """Calls os.path.dirname() on the argument n times."""
-  path = os.path.abspath(path)
-  for _ in range(n):
-    path = os.path.dirname(path)
-  return path
+def _GetToolsParentDir():
+  """Returns an absolute path to the the tools directory's parent directory.
 
-
-assert __file__.endswith('tools/metrics/histograms/expand_owners_unittest.py')
-
-_PATH_TO_CHROMIUM_DIR = _DirnameN(__file__, 5)
+  Example: 'C:\a\n\ff\' or '/opt/n/ff/'.
+  """
+  return os.path.abspath(os.path.join(*expand_owners.DIR_ABOVE_TOOLS))
 
 
 def _GetFileDirective(path):
   """Returns a file directive line.
 
   Args:
-    path: An absolute path, e.g. '/some/directory/chromium/src/tools/OWNERS'.
+    path: An absolute path, e.g. '/some/directory/subdirectory/tools/OWNERS'.
 
   Returns:
     A file directive that can be used in an OWNERS file, e.g.
     file://tools/OWNERS.
   """
-  return ''.join(['file://', path.split('src/')[1]])
+  return ''.join(['file://', path[len(_GetToolsParentDir()) + 1:]])
 
 
 def _GetSrcRelativePath(path):
   """Returns a(n) src-relative path for the given file path.
 
   Args:
-    path: An absolute path, e.g. '/some/directory/chromium/src/tools/OWNERS'.
+    path: An absolute path, e.g. '/some/directory/subdirectory/tools/OWNERS'.
 
   Returns:
     A src-relative path, e.g.'src/tools/OWNERS'.
   """
-  assert path.startswith(_PATH_TO_CHROMIUM_DIR)
-  return path[len(_PATH_TO_CHROMIUM_DIR) + 1:]
+  assert path.startswith(_GetToolsParentDir())
+  return expand_owners.SRC + path[len(_GetToolsParentDir()) + 1:]
 
 
 def _MakeOwnersFile(filename, directory):
@@ -75,10 +71,17 @@ class ExpandOwnersTest(unittest.TestCase):
     self.temp_dir = tempfile.mkdtemp(
         dir=os.path.abspath(os.path.join(os.path.dirname(__file__))))
 
+    # The below construction is used rather than __file__.endswith() because
+    # the file extension could be .py or .pyc.
+    assert os.sep.join(
+        ['tools', 'metrics', 'histograms',
+         'expand_owners_unittest.py']) in __file__
+
   def tearDown(self):
     super(ExpandOwnersTest, self).tearDown()
     shutil.rmtree(self.temp_dir)
 
+  @unittest.skip("http://crbug.com/1164985")
   def testExpandOwnersUsesMetadataOverOwners(self):
     """Checks that DIR_METADATA is used if available"""
     with open(os.path.join(self.temp_dir, 'DIR_METADATA'), "w+") as md:
@@ -134,8 +137,10 @@ class ExpandOwnersTest(unittest.TestCase):
     expand_owners.ExpandHistogramsOWNERS(histograms)
     self.assertMultiLineEqual(histograms.toxml(), expected_histograms.toxml())
 
-  def testExpandOwnersWithSimpleOWNERSFilePath(self):
+  @mock.patch('expand_owners._ExtractComponentViaDirmd')
+  def testExpandOwnersWithSimpleOWNERSFilePath(self, mock_dirmd_extract):
     """Checks that OWNERS files are expanded."""
+    mock_dirmd_extract.return_value = None
     absolute_path = _MakeOwnersFile('simple_OWNERS', self.temp_dir)
     src_relative_path = _GetSrcRelativePath(absolute_path)
 
@@ -188,11 +193,15 @@ class ExpandOwnersTest(unittest.TestCase):
     expand_owners.ExpandHistogramsOWNERS(histograms)
     self.assertMultiLineEqual(histograms.toxml(), expected_histograms.toxml())
 
-  def testExpandOwnersWithLongFilePath(self):
-    """
-    Check that long file path which forces <owner> tags to separate lines is
-    supported.
+  @mock.patch('expand_owners._ExtractComponentViaDirmd')
+  def testExpandOwnersWithLongFilePath(self, mock_dirmd_extract):
+    """Checks that long OWNERS file paths are supported.
+
+    Most OWNERS file paths appear between owners tags on the same line, e.g.
+    <owner>src/chrome/browser</owner>. However, especially long paths may appear
+    on their own line between the tags.
     """
+    mock_dirmd_extract.return_value = None
     absolute_path = _MakeOwnersFile('simple_OWNERS', self.temp_dir)
     src_relative_path = _GetSrcRelativePath(absolute_path)
 
@@ -229,8 +238,10 @@ class ExpandOwnersTest(unittest.TestCase):
     expand_owners.ExpandHistogramsOWNERS(histograms)
     self.assertMultiLineEqual(histograms.toxml(), expected_histograms.toxml())
 
-  def testExpandOwnersWithDuplicateOwners(self):
+  @mock.patch('expand_owners._ExtractComponentViaDirmd')
+  def testExpandOwnersWithDuplicateOwners(self, mock_dirmd_extract):
     """Checks that owners are unique."""
+    mock_dirmd_extract.return_value = None
     absolute_path = _MakeOwnersFile('simple_OWNERS', self.temp_dir)
     src_relative_path = _GetSrcRelativePath(absolute_path)
 
@@ -266,8 +277,10 @@ class ExpandOwnersTest(unittest.TestCase):
     expand_owners.ExpandHistogramsOWNERS(histograms)
     self.assertMultiLineEqual(histograms.toxml(), expected_histograms.toxml())
 
-  def testExpandOwnersWithFileDirectiveOWNERSFilePath(self):
+  @mock.patch('expand_owners._ExtractComponentViaDirmd')
+  def testExpandOwnersWithFileDirectiveOWNERSFilePath(self, mock_dirmd_extract):
     """Checks that OWNERS files with file directives are expanded."""
+    mock_dirmd_extract.return_value = None
     simple_absolute_path = _MakeOwnersFile('simple_OWNERS', self.temp_dir)
 
     with open(simple_absolute_path, 'w') as owners_file:
@@ -314,8 +327,11 @@ class ExpandOwnersTest(unittest.TestCase):
     expand_owners.ExpandHistogramsOWNERS(histograms)
     self.assertEqual(histograms.toxml(), expected_histograms.toxml())
 
-  def testExpandOwnersForOWNERSFileWithDuplicateComponents(self):
+  @mock.patch('expand_owners._ExtractComponentViaDirmd')
+  def testExpandOwnersForOWNERSFileWithDuplicateComponents(
+      self, mock_dirmd_extract):
     """Checks that only one component tag is added if there are duplicates."""
+    mock_dirmd_extract.return_value = None
     absolute_path = _MakeOwnersFile('OWNERS', self.temp_dir)
     src_relative_path = _GetSrcRelativePath(absolute_path)
 
@@ -463,8 +479,7 @@ class ExpandOwnersTest(unittest.TestCase):
 """)
 
     with self.assertRaisesRegexp(
-        expand_owners.Error,
-        r'The file at .*src/medium/medium/roast/OWNERS does not exist\.'):
+        expand_owners.Error, r'The file at .*medium.*OWNERS does not exist\.'):
       expand_owners.ExpandHistogramsOWNERS(histograms_with_fake_file_path)
 
   def testExpandOwnersWithoutOwnersFromFile(self):
@@ -595,28 +610,21 @@ class ExpandOwnersTest(unittest.TestCase):
       expand_owners._ExtractEmailAddressesFromOWNERS(
           file_directive_absolute_path)
 
+  def testGetHigherLevelPath(self):
+    """Checks that higher directories are recursively checked for OWNERS.
 
-class GetHigherLevelOwnersFilePathTest(unittest.TestCase):
-
-  def testGetHigherLevelPathDerivedPathInSrcDirectory(self):
-    """Checks that higher directories are recursively checked for OWNERS."""
-    path = expand_owners._GetOwnersFilePath('src/banana/chocolate/OWNERS')
-    self.assertRegexpMatches(
-        expand_owners._GetHigherLevelOwnersFilePath(path), r'.*src/OWNERS')
-
-  def testGetHigherLevelPathGivenPathInSrcDirectory(self):
-    """Checks that '' is returned when the last directory is reached.
-
-    If the directory above the tools directory is src, then receiving
-    'src/OWNERS' is the point at which recursion stops. However, this directory
-    may not always be src.
+    Also, checks that there isn't a recursive loop.
     """
-    path_to_chromium_directory = [
-        os.path.dirname(__file__), '..', '..', '..', '..'
-    ]
-    path = os.path.abspath(
-        os.path.join(*(path_to_chromium_directory + ['src/OWNERS'])))
-    self.assertEqual(expand_owners._GetHigherLevelOwnersFilePath(path), '')
+    path = expand_owners._GetOwnersFilePath('src/banana/chocolate/OWNERS')
+    result = expand_owners._GetHigherLevelOwnersFilePath(path)
+
+    # The condition is true when the tools directory's parent directory is src,
+    # which is generally the case locally. However, the parent directory is not
+    # always src, e.g. on various testing bots.
+    if os.path.basename(_GetToolsParentDir()) == 'src':
+      self.assertRegexpMatches(result, r'.*OWNERS')
+    else:
+      self.assertEqual(result, '')
 
 
 if __name__ == '__main__':

tools/metrics/metrics_python_tests.py

@@ -23,8 +23,7 @@ def resolve(*paths):
 sys.exit(
     typ.main(tests=resolve(
         'actions/extract_actions_test.py',
-        # TODO(crbug/1126653): Turn back on once tests can pass again.
-        # 'histograms/expand_owners_unittest.py',
+        'histograms/expand_owners_unittest.py',
         'histograms/extract_histograms_test.py',
         'histograms/generate_expired_histograms_array_unittest.py',
         'histograms/pretty_print_test.py',