From 88bc812650299ff2ae07af2ff8d8731be8a448ba Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Thu, 19 Dec 2024 14:14:40 -0800 Subject: [PATCH] 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 > > Auto-Submit: Scott Lee > > Commit-Queue: Scott Lee > > 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 > Commit-Queue: Scott Lee Bug: 384940858 Change-Id: Ibe20d5e46c519d7fdbd1114565ec3856e5bf928e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6111997 Reviewed-by: Josip Sokcevic Commit-Queue: Scott Lee --- git_cl.py | 6 ++- metrics_xml_format.py | 26 ++++++------ tests/git_cl_test.py | 64 +++++++++++++++++++++++++++++ tests/metrics_xml_format_test.py | 69 ++++++++++++++++++-------------- 4 files changed, 120 insertions(+), 45 deletions(-) diff --git a/git_cl.py b/git_cl.py index 8cda7dc119..0838d596b1 100755 --- a/git_cl.py +++ b/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') diff --git a/metrics_xml_format.py b/metrics_xml_format.py index a415291b59..e249364e57 100755 --- a/metrics_xml_format.py +++ b/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 diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index d7f7cb4803..635b2ce932 100755 --- a/tests/git_cl_test.py +++ b/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') diff --git a/tests/metrics_xml_format_test.py b/tests/metrics_xml_format_test.py index 4d83b6bab8..7beda69428 100755 --- a/tests/metrics_xml_format_test.py +++ b/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__':