Reland "Support formatting metrics xml(s) in the subfolders."

This reverts commit bfe1a9282dd124d0748145995602880b0dbf92b5.

Reason for revert: reland with a fix.
- Find the diff between ps#1 and ps#2.
- Tested at https://paste.googleplex.com/4670451708854272

Original change's description:
> Revert "Support formatting metrics xml(s) in the subfolders."
>
> This reverts commit 597ba08be5eb953914ba48d2dc85f1f41dbbec31.
>
> Reason for revert: it broke git_cl.py. Need further patch
>
> Original change's description:
> > Support formatting metrics xml(s) in the subfolders.
> >
> > https://crrev.com/c/6072565 assumed that the XMLs are located under
> > tools/metrics/{actions,ukm,structured,histograms} directly, such as
> > tools/metrics/histograms/enums.xml.
> >
> > However, its subfolders may have XML files, and it should format
> > the files. This CL fixes it.
> >
> > Bug: 384940858
> > Change-Id: I56484144e6f72f41eb5bc37a5ad462a0de1ec0e3
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6111994
> > Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
> > Auto-Submit: Scott Lee <ddoman@chromium.org>
> > Commit-Queue: Scott Lee <ddoman@chromium.org>
>
> Bug: 384940858
> Change-Id: I322573ad6d2d758cd3d2de872efdbba4fd9330c2
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6111996
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Scott Lee <ddoman@chromium.org>

Bug: 384940858
Change-Id: Ibe20d5e46c519d7fdbd1114565ec3856e5bf928e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6111997
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Scott Lee <ddoman@chromium.org>

git_cl.py

@@ -6790,8 +6790,10 @@ def _RunMetricsXMLFormat(opts, paths, top_dir, upstream_commit):
         # tools/metrics/histogrmas, pretty-print should be run with an
         # additional relative path argument, like: $ python pretty_print.py
         # metadata/UMA/histograms.xml $ python pretty_print.py enums.xml
-        if metrics_xml_format.GetMetricsDir(path) == 'tools/metrics/histograms':
-            cmd.append(path.replace('/', os.path.sep))
+        metricsDir = metrics_xml_format.GetMetricsDir(top_dir, path)
+        histogramsDir = os.path.join(top_dir, 'tools', 'metrics', 'histograms')
+        if metricsDir == histogramsDir:
+            cmd.append(path)
         if opts.dry_run or opts.diff:
             cmd.append('--diff')
 

metrics_xml_format.py

@@ -11,17 +11,16 @@ import subprocess
 import sys
 
 
-def GetMetricsDir(path):
+def GetMetricsDir(top_dir, path):
     metrics_xml_dirs = [
-        'tools/metrics/actions',
-        'tools/metrics/histograms',
-        'tools/metrics/structured',
-        'tools/metrics/ukm',
+        os.path.join(top_dir, 'tools', 'metrics', 'actions'),
+        os.path.join(top_dir, 'tools', 'metrics', 'histograms'),
+        os.path.join(top_dir, 'tools', 'metrics', 'structured'),
+        os.path.join(top_dir, 'tools', 'metrics', 'ukm'),
     ]
-    normalized_abs_dirname = os.path.dirname(os.path.realpath(path)).replace(
-        os.sep, '/')
+    abs_dirname = os.path.dirname(os.path.realpath(path))
     for xml_dir in metrics_xml_dirs:
-        if normalized_abs_dirname.endswith(xml_dir):
+        if abs_dirname.startswith(xml_dir):
             return xml_dir
     return None
 
@@ -46,7 +45,7 @@ def FindMetricsXMLFormatterTool(path, verbose=False):
     if not top_dir:
         log('Not executed in a Chromium checkout; skip formatting', verbose)
         return ''
-    xml_dir = GetMetricsDir(path)
+    xml_dir = GetMetricsDir(top_dir, path)
     if not xml_dir:
         log(f'{path} is not a metric XML; skip formatting', verbose)
         return ''
@@ -58,17 +57,16 @@ def FindMetricsXMLFormatterTool(path, verbose=False):
             'skip formatting', verbose)
         return ''
 
-    if xml_dir == 'tools/metrics/histograms':
+    histograms_base_dir = os.path.join(top_dir, 'tools', 'metrics',
+                                       'histograms')
+    if xml_dir.startswith(histograms_base_dir):
         # Skips the formatting, if the XML file is not one of the known types.
         if not IsSupportedHistogramsXML(path):
             log(f'{path} is not a supported histogram XML; skip formatting',
                 verbose)
             return ''
 
-    # top_dir is already formatted with the OS specific path separator, whereas
-    # xml_dir is not yet.
-    tool_dir = os.path.join(top_dir, xml_dir.replace('/', os.path.sep))
-    return os.path.join(tool_dir, 'pretty_print.py')
+    return os.path.join(xml_dir, 'pretty_print.py')
 
 
 usage_text = """Usage: %s [option] filepath

tests/git_cl_test.py

@@ -5227,6 +5227,70 @@ class CMDFormatTestCase(unittest.TestCase):
         ]
         self._check_yapf_filtering(files, expected)
 
+    @mock.patch('gclient_paths.GetPrimarySolutionPath')
+    def testRunMetricsXMLFormatSkipIfPresubmit(self, find_top_dir):
+        """Verifies that it skips the formatting if opts.presubmit is True."""
+        find_top_dir.return_value = self._top_dir
+        mock_opts = mock.Mock(full=True,
+                              dry_run=True,
+                              diff=False,
+                              presubmit=True)
+        files = [
+            os.path.join(self._top_dir, 'tools', 'metrics', 'ukm', 'ukm.xml'),
+        ]
+        return_value = git_cl._RunMetricsXMLFormat(mock_opts, files,
+                                                   self._top_dir, 'HEAD')
+        git_cl.RunCommand.assert_not_called()
+        self.assertEqual(0, return_value)
+
+    @mock.patch('gclient_paths.GetPrimarySolutionPath')
+    def testRunMetricsFormatWithUkm(self, find_top_dir):
+        """Checks if the command line arguments do not contain the input path.
+        """
+        find_top_dir.return_value = self._top_dir
+        mock_opts = mock.Mock(full=True,
+                              dry_run=False,
+                              diff=False,
+                              presubmit=False)
+        files = [
+            os.path.join(self._top_dir, 'tools', 'metrics', 'ukm', 'ukm.xml'),
+        ]
+        git_cl._RunMetricsXMLFormat(mock_opts, files, self._top_dir, 'HEAD')
+        git_cl.RunCommand.assert_called_with([
+            mock.ANY,
+            os.path.join(self._top_dir, 'tools', 'metrics', 'ukm',
+                         'pretty_print.py'),
+            '--non-interactive',
+        ],
+                                             cwd=self._top_dir)
+
+    @mock.patch('gclient_paths.GetPrimarySolutionPath')
+    def testRunMetricsFormatWithHistograms(self, find_top_dir):
+        """Checks if the command line arguments contain the input file paths."""
+        find_top_dir.return_value = self._top_dir
+        mock_opts = mock.Mock(full=True,
+                              dry_run=False,
+                              diff=False,
+                              presubmit=False)
+        files = [
+            os.path.join(self._top_dir, 'tools', 'metrics', 'histograms',
+                         'enums.xml'),
+            os.path.join(self._top_dir, 'tools', 'metrics', 'histograms',
+                         'test_data', 'enums.xml'),
+        ]
+        git_cl._RunMetricsXMLFormat(mock_opts, files, self._top_dir, 'HEAD')
+
+        pretty_print_path = os.path.join(self._top_dir, 'tools', 'metrics',
+                                         'histograms', 'pretty_print.py')
+        git_cl.RunCommand.assert_has_calls([
+            mock.call(
+                [mock.ANY, pretty_print_path, '--non-interactive', files[0]],
+                cwd=self._top_dir),
+            mock.call(
+                [mock.ANY, pretty_print_path, '--non-interactive', files[1]],
+                cwd=self._top_dir),
+        ])
+
 
 @unittest.skipIf(gclient_utils.IsEnvCog(),
                 'not supported in non-git environment')

tests/metrics_xml_format_test.py

@@ -14,8 +14,7 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 import gclient_paths_test
 import metrics_xml_format
 
-norm = lambda path: path.replace('/', os.sep)
-join = os.path.join
+norm = lambda path: os.path.join(*path.split('/'))
 
 
 class TestBase(gclient_paths_test.TestBase):
@@ -30,83 +29,95 @@ class TestBase(gclient_paths_test.TestBase):
         mock.patch('os.path.realpath', self.realpath).start()
         # gclient_paths.GetPrimarysolutionPath() defaults to src.
         self.make_file_tree({'.gclient': ''})
-        self.cwd = join(self.cwd, 'src')
+        self.cwd = os.path.join(self.cwd, 'src')
 
     def realpath(self, path):
         if os.path.isabs(path):
             return path
 
-        return join(self.getcwd(), path)
+        return os.path.join(self.getcwd(), path)
 
 
 class GetMetricsDirTest(TestBase):
 
     def testWithAbsolutePath(self):
-        get = lambda path: metrics_xml_format.GetMetricsDir(norm(path))
-        self.assertTrue(get('/src/tools/metrics/actions/abc.xml'))
-        self.assertTrue(get('/src/tools/metrics/histograms/abc.xml'))
-        self.assertTrue(get('/src/tools/metrics/structured/abc.xml'))
-        self.assertTrue(get('/src/tools/metrics/ukm/abc.xml'))
+        top = self.getcwd()
+        get = lambda path: metrics_xml_format.GetMetricsDir(
+            top, os.path.join(top, norm(path)))
 
-        self.assertFalse(get('/src/tools/metrics/actions/next/abc.xml'))
-        self.assertFalse(get('/src/tools/metrics/histograms/next/abc.xml'))
-        self.assertFalse(get('/src/tools/metrics/structured/next/abc.xml'))
-        self.assertFalse(get('/src/tools/metrics/ukm/next/abc.xml'))
+        self.assertTrue(get('tools/metrics/actions/abc.xml'))
+        self.assertTrue(get('tools/metrics/histograms/abc.xml'))
+        self.assertTrue(get('tools/metrics/structured/abc.xml'))
+        self.assertTrue(get('tools/metrics/ukm/abc.xml'))
+
+        self.assertFalse(get('tools/test/metrics/actions/abc.xml'))
+        self.assertFalse(get('tools/test/metrics/histograms/abc.xml'))
+        self.assertFalse(get('tools/test/metrics/structured/abc.xml'))
+        self.assertFalse(get('tools/test/metrics/ukm/abc.xml'))
 
     def testWithRelativePaths(self):
-        get = lambda path: metrics_xml_format.GetMetricsDir(norm(path))
-        self.cwd = join(self.cwd, 'tools')
-        self.assertFalse(get('abc.xml'))
-        self.assertTrue(get('metrics/actions/abc.xml'))
+        top = self.getcwd()
+        # chdir() to tools so that relative paths from tools become valid.
+        self.cwd = os.path.join(self.cwd, 'tools')
+        get = lambda path: metrics_xml_format.GetMetricsDir(top, path)
+        self.assertTrue(get(norm('metrics/actions/abc.xml')))
+        self.assertFalse(get(norm('abc.xml')))
 
 
 class FindMetricsXMLFormatTool(TestBase):
 
     def testWithMetricsXML(self):
+        top = self.getcwd()
         findTool = metrics_xml_format.FindMetricsXMLFormatterTool
 
         self.assertEqual(
             findTool(norm('tools/metrics/actions/abc.xml')),
-            join(self.getcwd(), norm('tools/metrics/actions/pretty_print.py')),
+            os.path.join(top, norm('tools/metrics/actions/pretty_print.py')),
         )
 
         # same test, but with an absolute path.
         self.assertEqual(
-            findTool(join(self.getcwd(),
-                          norm('tools/metrics/actions/abc.xml'))),
-            join(self.getcwd(), norm('tools/metrics/actions/pretty_print.py')),
+            findTool(os.path.join(top, norm('tools/metrics/actions/abc.xml'))),
+            os.path.join(top, norm('tools/metrics/actions/pretty_print.py')),
         )
 
     def testWthNonMetricsXML(self):
         findTool = metrics_xml_format.FindMetricsXMLFormatterTool
-        self.assertEqual(findTool('tools/metrics/actions/next/abc.xml'), '')
+        self.assertEqual(findTool(norm('tools/metrics/test/abc.xml')), '')
 
     def testWithNonCheckout(self):
         findTool = metrics_xml_format.FindMetricsXMLFormatterTool
         self.cwd = self.root
-        self.assertEqual(findTool('tools/metrics/actions/abc.xml'), '')
+        self.assertEqual(findTool(norm('tools/metrics/actions/abc.xml')), '')
 
     def testWithDifferentCheckout(self):
         findTool = metrics_xml_format.FindMetricsXMLFormatterTool
-        checkout2 = join(self.root, '..', self._testMethodName + '2', 'src')
+        checkout2 = os.path.join(self.root, '..', self._testMethodName + '2',
+                                 'src')
         self.assertEqual(
             # this is the case the tool was given a file path that is located
             # in a different checkout folder.
-            findTool(join(checkout2, norm('tools/metrics/actions/abc.xml'))),
+            findTool(
+                os.path.join(checkout2, norm('tools/metrics/actions/abc.xml'))),
             '',
         )
 
     def testSupportedHistogramsXML(self):
+        top = self.getcwd()
         findTool = metrics_xml_format.FindMetricsXMLFormatterTool
         self.assertEqual(
             findTool(norm('tools/metrics/histograms/enums.xml')),
-            join(self.getcwd(),
-                 norm('tools/metrics/histograms/pretty_print.py')),
+            os.path.join(top, norm('tools/metrics/histograms/pretty_print.py')),
+        )
+        self.assertEqual(
+            findTool(norm('tools/metrics/histograms/tests/histograms.xml')),
+            os.path.join(top, norm('tools/metrics/histograms/pretty_print.py')),
         )
 
     def testNotSupportedHistogramsXML(self):
-        findTool = metrics_xml_format.FindMetricsXMLFormatterTool
-        self.assertEqual(findTool(norm('tools/metrics/histograms/NO.xml')), '')
+        tool = metrics_xml_format.FindMetricsXMLFormatterTool(
+            norm('tools/metrics/histograms/NO.xml'))
+        self.assertEqual(tool, '')
 
 
 if __name__ == '__main__':