From 597ba08be5eb953914ba48d2dc85f1f41dbbec31 Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Thu, 19 Dec 2024 12:27:31 -0800 Subject: [PATCH] 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 --- metrics_xml_format.py | 26 ++++++------ tests/metrics_xml_format_test.py | 69 ++++++++++++++++++-------------- 2 files changed, 52 insertions(+), 43 deletions(-) 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/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__':