Better support of real tests under virtual test suite

Now group previous multiple virtual test suites with the same
prefix and args together.

For example, previous configuration
  {
    "prefix": "composite-after-paint",
    "base": "compositing",
    "args": ["--enable-blink-features=CompositeAfterPaint"]
  }
  {
    "prefix": "composite-after-paint",
    "base": "paint",
    "args": ["--enable-blink-features=CompositeAfterPaint"]
  }
now becomes
  {
    "prefix": "composite-after-paint",
    "bases": ["compositing",
              "paint"],
    "args": ["--enable-blink-features=CompositeAfterPaint"]
  }

This shortens VirtualTestSuites by nearly half, avoids accidental
inconsistent args (We should create different virtual test suite
for different args), and allow "pure physical" virtual test suite
which is an alternative of physical test suite.

A "pure physical" virtual test suite has empty "bases", and the
"virtual/<prefix>" test path contains real tests only.

Normal vitual test suites can still contain real tests.

Bug: 1014162
Change-Id: I8d24be0e62b44ec8987aef65cb6f9ff0423b2d2b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1873504
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710210}

docs/testing/web_tests.md

@@ -222,35 +222,43 @@ There are two ways to run web tests with additional command-line arguments:
 
 * Using a *virtual test suite* defined in
   [web_tests/VirtualTestSuites](../../third_party/blink/web_tests/VirtualTestSuites).
-  A virtual test suite runs a subset of web tests under a specific path with
-  additional flags. For example, you could test a (hypothetical) new mode for
+  A virtual test suite runs a subset of web tests with additional flags, with
+  `virtual/<prefix>/...` in their paths. The tests can be virtual tests that
+  map to real base tests (directories or files) whose paths match any of the
+  specified bases, or any real tests under `web_tests/virtual/<prefix>/`
+  directory. For example, you could test a (hypothetical) new mode for
   repainting using the following virtual test suite:
 
   ```json
   {
     "prefix": "blocking_repaint",
-    "base": "fast/repaint",
-    "args": ["--blocking-repaint"],
+    "bases": ["compositing", "fast/repaint"],
+    "args": ["--blocking-repaint"]
   }
   ```
 
   This will create new "virtual" tests of the form
+  `virtual/blocking_repaint/compositing/...` and
   `virtual/blocking_repaint/fast/repaint/...` which correspond to the files
-  under `web_tests/fast/repaint` and pass `--blocking-repaint` to
-  content_shell when they are run.
-
-  These virtual tests exist in addition to the original `fast/repaint/...`
-  tests. They can have their own expectations in TestExpectations, and their own
-  baselines.  The test harness will use the non-virtual baselines as a fallback.
-  However, the non-virtual expectations are not inherited: if
-  `fast/repaint/foo.html` is marked `[ Fail ]`, the test harness still expects
+  under `web_tests/compositing` and `web_tests/fast/repaint`, respectively,
+  and pass `--blocking-repaint` to `content_shell` when they are run.
+
+  These virtual tests exist in addition to the original `compositing/...` and
+  `fast/repaint/...` tests. They can have their own expectations in
+  `web_tests/TestExpectations`, and their own baselines. The test harness will
+  use the non-virtual baselines as a fallback. However, the non-virtual
+  expectations are not inherited: if `fast/repaint/foo.html` is marked
+  `[ Fail ]`, the test harness still expects
   `virtual/blocking_repaint/fast/repaint/foo.html` to pass. If you expect the
   virtual test to also fail, it needs its own suppression.
 
-  The "prefix" value does not have to be unique. This is useful if you want to
-  run multiple directories with the same flags (but see the notes below about
-  performance). Using the same prefix for different sets of flags is not
-  recommended.
+  This will also let any real tests under `web_tests/virtual/blocking_repaint`
+  directory run with the `--blocking-repaint` flag.
+
+  The "prefix" value should be unique. Multiple directories with the same flags
+  should be listed in the same "bases" list. The "bases" list can be empty,
+  in case that we just want to run the real tests under `virtual/<prefix>`
+  with the flags without creating any virtual tests.
 
 For flags whose implementation is still in progress, virtual test suites and
 flag-specific expectations represent two alternative strategies for testing.
@@ -273,7 +281,10 @@ Consider the following when choosing between them:
   architectural changes that potentially impact all of the tests.
 
 * Note that using wildcards in virtual test path names (e.g.
-  `virtual/blocking_repaint/fast/repaint/*`) is not supported.
+  `virtual/blocking_repaint/fast/repaint/*`) is not supported, but you can
+  still use `virtual/blocking_repaint` to run all real and virtual tests
+  in the suite or `virtual/blocking_repaint/fast/repaint/dir` to run real
+  or virtual tests in the suite under a specific directory.
 
 ## Tracking Test Failures
 

third_party/blink/tools/blinkpy/common/checkout/baseline_optimizer_unittest.py

@@ -97,7 +97,7 @@ def _assert_optimization(self, results_by_directory, directory_to_new_results, b
         baseline_name = 'mock-test-expected.' + suffix
         self.fs.write_text_file(
             self.fs.join(web_tests_dir, 'VirtualTestSuites'),
-            '[{"prefix": "gpu", "base": "fast/canvas", "args": ["--foo"]}]')
+            '[{"prefix": "gpu", "bases": ["fast/canvas"], "args": ["--foo"]}]')
 
         for dirname, contents in results_by_directory.items():
             self.fs.write_binary_file(self.fs.join(web_tests_dir, dirname, baseline_name), contents)

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

@@ -24,7 +24,7 @@ def setUp(self):
         # Mock a virtual test suite at virtual/gpu/external/wpt/foo.
         self.host.filesystem = MockFileSystem({
             MOCK_WEB_TESTS + 'VirtualTestSuites':
-            '[{"prefix": "gpu", "base": "external/wpt/foo", "args": ["--foo"]}]'
+            '[{"prefix": "gpu", "bases": ["external/wpt/foo"], "args": ["--foo"]}]'
         })
         self.git = self.host.git()
         self.local_wpt = MockLocalWPT()

third_party/blink/tools/blinkpy/web_tests/controllers/single_test_runner.py

@@ -571,9 +571,8 @@ def _run_reftest(self):
         expected_output = None
         reference_test_names = []
         reftest_failures = []
-        if self._port.lookup_virtual_test_base(self._test_name):
-            args = self._port.lookup_virtual_reference_args(self._test_name)
-        else:
+        args = self._port.lookup_virtual_reference_args(self._test_name)
+        if not args:
             args = self._port.lookup_physical_reference_args(self._test_name)
 
         # sort self._reference_files to put mismatch tests first

third_party/blink/tools/blinkpy/web_tests/controllers/web_test_runner.py

@@ -335,7 +335,10 @@ def _kill_driver(self, driver, label):
             driver.stop()
 
     def _clean_up_after_test(self, test_input, result):
-        test_name = test_input.test_name
+        test_description = test_input.test_name
+        test_args = self._port.args_for_test(test_input.test_name)
+        if test_args:
+            test_description += ' with args ' + ' '.join(test_args)
 
         if result.failures:
             # Check and kill the driver if we need to.
@@ -350,13 +353,13 @@ def _clean_up_after_test(self, test_input, result):
                 self._batch_count = 0
 
             # Print the error message(s).
-            _log.debug('%s %s failed:', self._name, test_name)
+            _log.debug('%s %s failed:', self._name, test_description)
             for f in result.failures:
                 _log.debug('%s  %s', self._name, f.message())
         elif result.type == test_expectations.SKIP:
-            _log.debug('%s %s skipped', self._name, test_name)
+            _log.debug('%s %s skipped', self._name, test_description)
         else:
-            _log.debug('%s %s passed', self._name, test_name)
+            _log.debug('%s %s passed', self._name, test_description)
 
 
 class TestShard(object):

third_party/blink/tools/blinkpy/web_tests/lint_test_expectations.py

@@ -83,38 +83,52 @@ def check_virtual_test_suites(host, options):
     fs = host.filesystem
     web_tests_dir = port.web_tests_dir()
     virtual_suites = port.virtual_test_suites()
-    # Make sure ancestors come first (e.g. "virtual/foo/bar", "virtual/foo/bar/baz").
-    virtual_suites.sort(key=lambda s: s.name)
+    virtual_suites.sort(key=lambda s: s.full_prefix)
 
-    seen = set()
     failures = []
     for suite in virtual_suites:
-        suite_comps = suite.name.split(port.TEST_PATH_SEPARATOR)
-        # E.g. virtual/foo/fast/css/a.html will execute twice if
-        # both virtual/foo/fast and virtual/foo/fast/css are both defined.
-        for i in range(3, len(suite_comps)):
-            ancestor = port.TEST_PATH_SEPARATOR.join(suite_comps[:i])
-            if ancestor in seen:
-                failure = ('{} is a subset of {}; you will see tests under the '
-                           'former running multiple times (potentially with '
-                           'different args).'.format(suite.name, ancestor))
-                _log.error(failure)
-                failures.append(failure)
-        seen.add(suite.name)
+        suite_comps = suite.full_prefix.split(port.TEST_PATH_SEPARATOR)
+        prefix = suite_comps[1]
+        normalized_bases = [port.normalize_test_name(b) for b in suite.bases]
+        normalized_bases.sort();
+        for i in range(1, len(normalized_bases)):
+            for j in range(0, i):
+                if normalized_bases[i].startswith(normalized_bases[j]):
+                    failure = 'Base "{}" starts with "{}" in the same virtual suite "{}", so is redundant.'.format(
+                        normalized_bases[i], normalized_bases[j], prefix)
+                    _log.error(failure)
+                    failures.append(failure)
 
         # A virtual test suite needs either
         # - a top-level README.md (e.g. virtual/foo/README.md)
-        # - a README.txt for each covered dir/file (e.g.
+        # - a README.txt for each covered directory (e.g.
         #   virtual/foo/http/tests/README.txt, virtual/foo/fast/README.txt, ...)
-        comps = [web_tests_dir] + suite_comps + ['README.txt']
-        path_to_readme_txt = fs.join(*comps)
-        comps = [web_tests_dir] + suite_comps[:2] + ['README.md']
+        comps = [web_tests_dir] + suite_comps + ['README.md']
         path_to_readme_md = fs.join(*comps)
-        if not fs.exists(path_to_readme_txt) and not fs.exists(path_to_readme_md):
-            failure = '{} and {} are both missing (each virtual suite must have one).'.format(
-                path_to_readme_txt, path_to_readme_md)
-            _log.error(failure)
-            failures.append(failure)
+        for base in suite.bases:
+            if not base:
+                failure = 'Base value in virtual suite "{}" should not be an empty string'.format(prefix)
+                _log.error(failure)
+                failures.append(failure)
+                continue
+            base_comps = base.split(port.TEST_PATH_SEPARATOR)
+            absolute_base = port.abspath_for_test(base)
+            if fs.isfile(absolute_base):
+                del base_comps[-1]
+            elif not fs.isdir(absolute_base):
+                failure = 'Base "{}" in virtual suite "{}" must refer to a real file or directory'.format(
+                    base, prefix)
+                _log.error(failure)
+                failures.append(failure)
+                continue
+            comps = [web_tests_dir] + suite_comps + base_comps + ['README.txt']
+            path_to_readme_txt = fs.join(*comps)
+            if not fs.exists(path_to_readme_md) and not fs.exists(path_to_readme_txt):
+                failure = '"{}" and "{}" are both missing (each virtual suite must have one).'.format(
+                    path_to_readme_txt, path_to_readme_md)
+                _log.error(failure)
+                failures.append(failure)
+
     if failures:
         _log.error('')
     return failures

third_party/blink/tools/blinkpy/web_tests/lint_test_expectations_unittest.py

@@ -177,9 +177,10 @@ def setUp(self):
 
     def test_check_virtual_test_suites_readme(self):
         self.port.virtual_test_suites = lambda: [
-            VirtualTestSuite(prefix='foo', base='test', args='--foo'),
-            VirtualTestSuite(prefix='bar', base='test', args='--bar'),
+            VirtualTestSuite(prefix='foo', bases=['test'], args=['--foo']),
+            VirtualTestSuite(prefix='bar', bases=['test'], args=['--bar']),
         ]
+        self.host.filesystem.maybe_make_directory(WEB_TEST_DIR + '/test')
 
         res = lint_test_expectations.check_virtual_test_suites(self.host, self.options)
         self.assertEqual(len(res), 2)
@@ -189,13 +190,34 @@ def test_check_virtual_test_suites_readme(self):
         res = lint_test_expectations.check_virtual_test_suites(self.host, self.options)
         self.assertFalse(res)
 
-    def test_check_virtual_test_suites_inclusion(self):
+    def test_check_virtual_test_suites_redundant(self):
         self.port.virtual_test_suites = lambda: [
-            VirtualTestSuite(prefix='foo', base='test/sub', args='--foo'),
-            VirtualTestSuite(prefix='foo', base='test', args='--foo'),
+            VirtualTestSuite(prefix='foo', bases=['test/sub', 'test'], args=['--foo']),
         ]
 
         self.host.filesystem.exists = lambda _: True
+        self.host.filesystem.isdir = lambda _: True
+        res = lint_test_expectations.check_virtual_test_suites(self.host, self.options)
+        self.assertEqual(len(res), 1)
+
+    def test_check_virtual_test_suites_non_redundant(self):
+        self.port.virtual_test_suites = lambda: [
+            VirtualTestSuite(prefix='foo', bases=['test_a', 'test'], args=['--foo']),
+        ]
+
+        self.host.filesystem.exists = lambda _: True
+        self.host.filesystem.isdir = lambda _: True
+        res = lint_test_expectations.check_virtual_test_suites(self.host, self.options)
+        self.assertEqual(len(res), 0)
+
+    def test_check_virtual_test_suites_non_existent_base(self):
+        self.port.virtual_test_suites = lambda: [
+            VirtualTestSuite(prefix='foo', bases=['base1', 'base2', 'base3.html'], args=['-foo']),
+        ]
+
+        self.host.filesystem.maybe_make_directory(WEB_TEST_DIR + '/base1')
+        self.host.filesystem.files[WEB_TEST_DIR + '/base3.html'] = ''
+        self.host.filesystem.files[WEB_TEST_DIR + '/virtual/foo/README.md'] = ''
         res = lint_test_expectations.check_virtual_test_suites(self.host, self.options)
         self.assertEqual(len(res), 1)