Refactor wpt metadata filename creation to handle variants containing '/'

This splits up generation of metadata filenames and generation of
"inline test names" that go inside the metadata files. This improves
readability and code isolation so that fixing a bug on the filename
side doesn't break the inline testname side.

Bug: 937369
Change-Id: I80665a551db216de391abe2fdf736202e2b466c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2095726
Commit-Queue: Luke Z <lpz@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748894}

third_party/blink/tools/blinkpy/w3c/wpt_metadata_builder.py

@@ -209,6 +209,46 @@ class WPTMetadataBuilder(object):
             # baseline is all PASS which should just be deleted.
             _log.error("Test %s has a non-FAIL baseline" % test_name)
 
+    def _metadata_filename_from_test_file(self, wpt_test_file):
+        """Returns the filename of the metadata (.ini) file for the test.
+
+        Args:
+            wpt_test_file: The file on disk that the specified test lives in.
+                For multi-global tests this is usually a ".js" file.
+
+        Returns:
+            The fully-qualified string path of the metadata file for this test.
+        """
+        assert "?" not in wpt_test_file
+        test_file_parts = wpt_test_file.split("/")
+        return os.path.join(self.metadata_output_dir, *test_file_parts) + ".ini"
+
+    def _metadata_inline_test_name_from_test_name(self, wpt_test_name):
+        """Returns the test name to use *inside* of a metadata file.
+
+        The inline name inside the metadata file is the logical name of the
+        test without any subdirectories.
+        For multi-global tests this means that it must have the specific scope
+        of the test (eg: worker, window, etc). This name must also include any
+        variants that are set.
+
+        Args:
+            wpt_test_name: The fully-qualified test name which contains all
+                subdirectories as well as scope (for multi-globals), and
+                variants.
+
+        Returns:
+            The string test name inside of the metadata file.
+        """
+        # To generate the inline test name we basically want to strip away the
+        # subdirectories from the test name, being careful not to accidentally
+        # clobber the variant.
+        variant_split = wpt_test_name.split("?")
+        test_path = variant_split[0]
+        test_name_part = test_path.split("/")[-1]
+        variant = "?" + variant_split[1] if len(variant_split) == 2 else ""
+        return test_name_part + variant
+
     def get_metadata_filename_and_contents(self, chromium_test_name, test_status_bitmap=0):
         """Determines the metadata filename and contents for the specified test.
 
@@ -255,27 +295,16 @@ class WPTMetadataBuilder(object):
             metadata_file_contents = self._get_dir_disabled_string()
         else:
             # For individual tests, we create one file per test, with the name
-            # of the test in the file as well. This name can contain variants.
+            # of the test in the file as well.
             test_file_path = self.wpt_manifest.file_path_for_test_url(wpt_test_name)
             if not test_file_path:
                 _log.info("Could not find file for test %s, skipping" % wpt_test_name)
                 return None, None
-            test_file_parts = test_file_path.split("/")
 
-            # Append `.ini` to the test filename to indicate it's the metadata
-            # file. The `test_filename` can differ from the `wpt_test_name` for
-            # multi-global tests.
-            test_file_parts[-1] += ".ini"
-            metadata_filename = os.path.join(self.metadata_output_dir,
-                                             *test_file_parts)
+            metadata_filename = self._metadata_filename_from_test_file(test_file_path)
             _log.debug("Creating a test ini file %s with status_bitmap %s", metadata_filename, test_status_bitmap)
-
-            # The contents of the metadata file is two lines:
-            # 1. the last part of the WPT test path (ie the filename) inside
-            #    square brackets - this could differ from the metadata filename.
-            # 2. an indented line with the test status and reason
-            wpt_test_file_name_part = wpt_test_name_parts[-1]
-            metadata_file_contents = self._get_test_failed_string(wpt_test_file_name_part, test_status_bitmap)
+            inline_test_name = self._metadata_inline_test_name_from_test_name(wpt_test_name)
+            metadata_file_contents = self._get_test_failed_string(inline_test_name, test_status_bitmap)
 
         return metadata_filename, metadata_file_contents
 
@@ -285,8 +314,12 @@ class WPTMetadataBuilder(object):
     def _get_test_disabled_string(self, test_name):
         return "[%s]\n  disabled: wpt_metadata_builder.py\n" % test_name
 
-    def _get_test_failed_string(self, test_name, test_status_bitmap):
-        result = "[%s]\n" % test_name
+    def _get_test_failed_string(self, inline_test_name, test_status_bitmap):
+        # The contents of the metadata file is two lines:
+        # 1. the inline name of the WPT test pathinside square brackets. This
+        #    name contains the test scope (for multi-globals) and variants.
+        # 2. an indented line with the test status and reason
+        result = "[%s]\n" % inline_test_name
 
         # A skipped test is a little special in that it doesn't happen along with
         # any other status. So we compare directly against SKIP_TEST and also

third_party/blink/tools/blinkpy/w3c/wpt_metadata_builder_unittest.py

@@ -64,13 +64,15 @@ class WPTMetadataBuilderTest(unittest.TestCase):
                 'items': {
                     'reftest': {
                         'reftest.html': [
-                            ['reftest.html', [['reftest-ref.html', '==']], {}]
+                            ['reftest.html', [['reftest-ref.html', '==']], {}],
                         ]
                     },
                     'testharness': {
                         'test.html': [['test.html', {}]],
-                        'variant.html': [['variant.html?foo=bar', {}],
-                                         ['variant.html?foo=baz', {}]],
+                        'variant.html': [
+                            ['variant.html?foo=bar/abc', {}],
+                            ['variant.html?foo=baz', {}],
+                        ],
                         'dir/zzzz.html': [['dir/zzzz.html', {}]],
                         'dir/multiglob.https.any.js': [
                             ['dir/multiglob.https.any.window.html', {}],
@@ -231,14 +233,14 @@ class WPTMetadataBuilderTest(unittest.TestCase):
 
     def test_metadata_for_skipped_test_with_variants(self):
         """A skipped WPT tests with variants should get a test-specific metadata file."""
-        test_name = "external/wpt/variant.html?foo=bar"
+        test_name = "external/wpt/variant.html?foo=bar/abc"
         expectations = _make_expectation(self.port, test_name, "SKIP")
         metadata_builder = WPTMetadataBuilder(expectations, self.port)
         filename, contents = metadata_builder.get_metadata_filename_and_contents(test_name, SKIP_TEST)
         # The metadata file name should not include variants
         self.assertEqual("variant.html.ini", filename)
         # ..but the contents of the file should include variants in the test name
-        self.assertEqual("[variant.html?foo=bar]\n  disabled: wpt_metadata_builder.py\n", contents)
+        self.assertEqual("[variant.html?foo=bar/abc]\n  disabled: wpt_metadata_builder.py\n", contents)
 
     def test_metadata_for_skipped_directory(self):
         """A skipped WPT directory should get a dir-wide metadata file."""
@@ -324,3 +326,25 @@ class WPTMetadataBuilderTest(unittest.TestCase):
         self.assertEqual(1, len(test_and_status_dict))
         self.assertTrue(test_name in test_and_status_dict)
         self.assertEqual(TEST_PASS | TEST_PRECONDITION_FAILED, test_and_status_dict[test_name])
+
+    def test_metadata_filename_from_test_file(self):
+        """Check that we get the correct metadata filename in various cases."""
+        expectations = TestExpectations(self.port)
+        mb = WPTMetadataBuilder(expectations, self.port)
+        self.assertEqual("test.html.ini", mb._metadata_filename_from_test_file("test.html"))
+        test_file = os.path.join("dir", "multiglob.https.any.js")
+        self.assertEqual(test_file + ".ini", mb._metadata_filename_from_test_file(test_file))
+        with self.assertRaises(AssertionError):
+            mb._metadata_filename_from_test_file("test.html?variant=abc")
+
+    def test_inline_test_name_from_test_name(self):
+        """Check that we get the correct inline test name in various cases."""
+        expectations = TestExpectations(self.port)
+        mb = WPTMetadataBuilder(expectations, self.port)
+        self.assertEqual("test.html", mb._metadata_inline_test_name_from_test_name("test.html"))
+        self.assertEqual("test.html", mb._metadata_inline_test_name_from_test_name("dir/test.html"))
+        self.assertEqual("test.html?variant=abc", mb._metadata_inline_test_name_from_test_name("dir/test.html?variant=abc"))
+        self.assertEqual("test.html?variant=abc/def", mb._metadata_inline_test_name_from_test_name("dir/test.html?variant=abc/def"))
+        self.assertEqual("test.worker.html", mb._metadata_inline_test_name_from_test_name("test.worker.html"))
+        self.assertEqual("test.worker.html?variant=abc",
+                         mb._metadata_inline_test_name_from_test_name("dir/test.worker.html?variant=abc"))