Don't match flag-specific expectations and baselines for --additional…

…-driver-flag

This simplifies the logic, and discourages the use of
--additional-driver-flag when flag-specific expectations and
baselines are needed, especially on bots.

--flag-specific has many advantages and is preferred.

Bug: 1238155
Change-Id: I83a817faf4c47194db1e9a637cff6f85a011b317
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3812107
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Weizhong Xia <weizhong@google.com>
Cr-Commit-Position: refs/heads/main@{#1033569}

docs/testing/web_tests.md

@@ -220,14 +220,21 @@ on this.
 
 There are two ways to run web tests with additional command-line arguments:
 
-### --flag-specific or --additional-driver-flag:
+### --flag-specific
 
 ```bash
-# Actually we prefer --flag-specific in some cases. See below for details.
-third_party/blink/tools/run_web_tests.py --additional-driver-flag=--blocking-repaint
+third_party/blink/tools/run_web_tests.py --flag-specific=blocking-repaint
 ```
+It requires that `web_tests/FlagSpecificConfig` contains an entry like:
 
-This tells the test harness to pass `--blocking-repaint` to the
+```json
+{
+  "name": "blocking-repaint",
+  "args": ["--blocking-repaint", "--another-flag"]
+}
+```
+
+This tells the test harness to pass `--blocking-repaint --another-flag` to the
 content_shell binary.
 
 It will also look for flag-specific expectations in
@@ -239,39 +246,17 @@ is always merged into the used expectations.
 It will also look for baselines in `web_tests/flag-specific/blocking-repaint`.
 The baselines in this directory override the fallback baselines.
 
-By default, name of the expectation file name under
-`web_tests/FlagExpectations` and name of the baseline directory under
-`web_tests/flag-specific` uses the first flag of --additional-driver-flag
-with leading '-'s stripped.
-
-You can also customize the name in `web_tests/FlagSpecificConfig` when
-the name is too long or when we need to match multiple additional args:
-
-```json
-{
-  "name": "short-name",
-  "args": ["--blocking-repaint", "--another-flag"]
-}
-```
-
-`web_tests/FlagSpecificConfig` is preferred when you need multiple flags,
-or the flag is long.
-
-With the config, you can use `--flag-specific=short-name` as a shortcut
-of `--additional-driver-flag=--blocking-repaint --additional-driver-flag=--another-flag`.
-
-`--additional-driver-flags` still works with `web_tests/FlagSpecificConfig`.
-For example, when at least `--additional-driver-flag=--blocking-repaint` and
-`--additional-driver-flag=--another-flag` are specified, `short-name` will
-be used as name of the flag specific expectation file and the baseline directory.
-
 *** note
 [BUILD.gn](../../BUILD.gn) assumes flag-specific builders always runs on linux bots, so
 flag-specific test expectations and baselines are only downloaded to linux bots.
 If you need run flag-specific builders on other platforms, please update
 BUILD.gn to download flag-specific related data to that platform.
 ***
 
+You can also use `--additional-driver-flag` to specify additional command-line
+arguments to content_shell, but the test harness won't use any flag-specific
+test expectations or baselines.
+
 ### Virtual test suites
 
 A *virtual test suite* can be defined in

third_party/blink/tools/blinkpy/web_tests/port/base.py

@@ -297,29 +297,17 @@ def get_platform_tags(self):
 
     @memoized
     def flag_specific_config_name(self):
-        """Returns the name of the flag-specific configuration which best matches
-           self._specified_additional_driver_flags(), or the first specified flag
-           with leading '-'s stripped if no match in the configuration is found.
+        """Returns the name of the flag-specific configuration if it's specified in
+           --flag-specific option, or None. The name must be defined in
+           FlagSpecificConfig or an AssertionError will be raised.
         """
-        specified_flags = self._specified_additional_driver_flags()
-        if not specified_flags:
-            return None
-
-        best_match = None
-        configs = self._flag_specific_configs()
-        for name in configs:
-            # To match the specified flags must start with all config args.
-            args = configs[name]
-            if specified_flags[:len(args)] != args:
-                continue
-            # The first config matching the highest number of specified flags wins.
-            if not best_match or len(configs[best_match]) < len(args):
-                best_match = name
-
-        if best_match:
-            return best_match
-        # If no match, fallback to the old mode: using the name of the first specified flag.
-        return specified_flags[0].lstrip('-')
+        config_name = self.get_option('flag_specific')
+        if config_name:
+            configs = self._flag_specific_configs()
+            assert config_name in configs, '{} is not defined in FlagSpecificConfig'.format(
+                config_name)
+            return config_name
+        return None
 
     @memoized
     def _flag_specific_configs(self):
@@ -367,12 +355,9 @@ def _specified_additional_driver_flags(self):
         if self._filesystem.exists(flag_file):
             flags = self._filesystem.read_text_file(flag_file).split()
 
-        flag_specific_option = self.get_option('flag_specific')
+        flag_specific_option = self.flag_specific_config_name()
         if flag_specific_option:
-            configs = self._flag_specific_configs()
-            assert flag_specific_option in configs, '{} is not defined in FlagSpecificConfig'.format(
-                flag_specific_option)
-            flags += configs[flag_specific_option]
+            flags += self._flag_specific_configs()[flag_specific_option]
 
         flags += self.get_option('additional_driver_flag', [])
         return flags
@@ -492,7 +477,7 @@ def baseline_version_dir(self):
         return baseline_search_paths[0]
 
     def baseline_flag_specific_dir(self):
-        """If --additional-driver-flag is specified, returns the absolute path to the flag-specific
+        """If --flag-specific is specified, returns the absolute path to the flag-specific
            platform-independent results. Otherwise returns None."""
         config_name = self.flag_specific_config_name()
         if not config_name:

third_party/blink/tools/blinkpy/web_tests/port/base_unittest.py

@@ -232,10 +232,14 @@ def test_expected_baselines_flag_specific(self):
             MOCK_WEB_TESTS + 'VirtualTestSuites',
             '[{ "prefix": "bar", "platforms": ["Linux", "Mac", "Win"],'
             ' "bases": ["fast"], "args": ["--bar"]}]')
+        port.host.filesystem.write_text_file(
+            MOCK_WEB_TESTS + 'FlagSpecificConfig',
+            '[{"name": "special-flag", "args": ["--special"]}]')
 
         # pylint: disable=protected-access
         port._options.additional_platform_directory = []
-        port._options.additional_driver_flag = ['--special-flag']
+        port._options.additional_driver_flag = ['--flag-not-affecting']
+        port._options.flag_specific = 'special-flag'
         self.assertEqual(port.baseline_search_path(), [
             MOCK_WEB_TESTS + 'flag-specific/special-flag',
             MOCK_WEB_TESTS + 'platform/foo'
@@ -527,8 +531,9 @@ def test_additional_expectations_additional_flag(self):
             ],
             'additional_driver_flag': ['--special-flag']
         })
+        # --additional-driver-flag doesn't affect baseline search path.
         self.assertEqual(list(port.expectations_dict().values()),
-                         ['content3', 'content1\n', 'content2\n'])
+                         ['content1\n', 'content2\n'])
 
     def test_flag_specific_expectations(self):
         port = self.make_port(port_name='foo')
@@ -573,15 +578,15 @@ def test_flag_specific_config_name_from_options(self):
                 'additional_driver_flag': ['--bb']
             }))
         self.assertEqual(port_b._specified_additional_driver_flags(), ['--bb'])
-        self.assertEqual(port_b.flag_specific_config_name(), 'bb')
+        self.assertIsNone(port_b.flag_specific_config_name())
 
         port_c = self.make_port(
             options=optparse.Values({
                 'additional_driver_flag': ['--cc', '--dd']
             }))
         self.assertEqual(port_c._specified_additional_driver_flags(),
                          ['--cc', '--dd'])
-        self.assertEqual(port_c.flag_specific_config_name(), 'cc')
+        self.assertIsNone(port_c.flag_specific_config_name())
 
     def test_flag_specific_config_name_from_options_and_file(self):
         flag_file = MOCK_WEB_TESTS + 'additional-driver-flag.setting'
@@ -590,7 +595,8 @@ def test_flag_specific_config_name_from_options_and_file(self):
         port_a.host.filesystem.write_text_file(flag_file, '--aa')
         # pylint: disable=protected-access
         self.assertEqual(port_a._specified_additional_driver_flags(), ['--aa'])
-        self.assertEqual(port_a.flag_specific_config_name(), 'aa')
+        # Additional driver flags don't affect flag_specific_config_name.
+        self.assertIsNone(port_a.flag_specific_config_name())
 
         port_b = self.make_port(
             options=optparse.Values({
@@ -599,7 +605,7 @@ def test_flag_specific_config_name_from_options_and_file(self):
         port_b.host.filesystem.write_text_file(flag_file, '--aa')
         self.assertEqual(port_b._specified_additional_driver_flags(),
                          ['--aa', '--bb'])
-        self.assertEqual(port_b.flag_specific_config_name(), 'aa')
+        self.assertIsNone(port_a.flag_specific_config_name())
 
         port_c = self.make_port(
             options=optparse.Values({
@@ -609,7 +615,7 @@ def test_flag_specific_config_name_from_options_and_file(self):
         # We don't remove duplicated flags at this time.
         self.assertEqual(port_c._specified_additional_driver_flags(),
                          ['--bb', '--dd', '--bb', '--cc'])
-        self.assertEqual(port_c.flag_specific_config_name(), 'bb')
+        self.assertIsNone(port_a.flag_specific_config_name())
 
     def _write_flag_specific_config(self, port):
         port.host.filesystem.write_text_file(
@@ -620,81 +626,6 @@ def _write_flag_specific_config(self, port):
             '  {"name": "c", "args": ["--bb", "--cc"]}'
             ']')
 
-    def test_flag_specific_config_name_from_options_and_config(self):
-        port_a1 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--aa']
-            }))
-        self._write_flag_specific_config(port_a1)
-        # pylint: disable=protected-access
-        self.assertEqual(port_a1.flag_specific_config_name(), 'a')
-
-        port_a2 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--aa', '--dd']
-            }))
-        self._write_flag_specific_config(port_a2)
-        self.assertEqual(port_a2.flag_specific_config_name(), 'a')
-
-        port_a3 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--aa', '--bb']
-            }))
-        self._write_flag_specific_config(port_a3)
-        self.assertEqual(port_a3.flag_specific_config_name(), 'a')
-
-        port_b1 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--bb', '--aa']
-            }))
-        self._write_flag_specific_config(port_b1)
-        self.assertEqual(port_b1.flag_specific_config_name(), 'b')
-
-        port_b2 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--bb', '--aa', '--cc']
-            }))
-        self._write_flag_specific_config(port_b2)
-        self.assertEqual(port_b2.flag_specific_config_name(), 'b')
-
-        port_b3 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--bb', '--aa', '--dd']
-            }))
-        self._write_flag_specific_config(port_b3)
-        self.assertEqual(port_b3.flag_specific_config_name(), 'b')
-
-        port_c1 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--bb', '--cc']
-            }))
-        self._write_flag_specific_config(port_c1)
-        self.assertEqual(port_c1.flag_specific_config_name(), 'c')
-
-        port_c2 = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--bb', '--cc', '--aa']
-            }))
-        self._write_flag_specific_config(port_c2)
-        self.assertEqual(port_c2.flag_specific_config_name(), 'c')
-
-    def test_flag_specific_fallback(self):
-        port_b = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--bb']
-            }))
-        self._write_flag_specific_config(port_b)
-        # No match. Fallback to first specified flag.
-        self.assertEqual(port_b.flag_specific_config_name(), 'bb')
-
-        port_d = self.make_port(
-            options=optparse.Values({
-                'additional_driver_flag': ['--dd', '--ee']
-            }))
-        self._write_flag_specific_config(port_d)
-        # pylint: disable=protected-access
-        self.assertEqual(port_d.flag_specific_config_name(), 'dd')
-
     def test_flag_specific_option(self):
         port_a = self.make_port(
             options=optparse.Values({
@@ -703,6 +634,7 @@ def test_flag_specific_option(self):
         self._write_flag_specific_config(port_a)
         # pylint: disable=protected-access
         self.assertEqual(port_a.flag_specific_config_name(), 'a')
+        self.assertEqual(port_a._specified_additional_driver_flags(), ['--aa'])
 
         port_b = self.make_port(
             options=optparse.Values({
@@ -711,13 +643,17 @@ def test_flag_specific_option(self):
             }))
         self._write_flag_specific_config(port_b)
         self.assertEqual(port_b.flag_specific_config_name(), 'a')
+        self.assertEqual(port_b._specified_additional_driver_flags(),
+                         ['--aa', '--bb'])
 
         port_d = self.make_port(
             options=optparse.Values({
                 'flag_specific': 'd'
             }))
         self._write_flag_specific_config(port_d)
         self.assertRaises(AssertionError, port_d.flag_specific_config_name)
+        self.assertRaises(AssertionError,
+                          port_d._specified_additional_driver_flags)
 
     def test_duplicate_flag_specific_name(self):
         port = self.make_port()

third_party/blink/tools/blinkpy/web_tests/port/port_testcase.py

@@ -361,9 +361,15 @@ def test_default_expectations_ordering(self):
     def test_used_expectations_files(self):
         options = optparse.Values({
             'additional_expectations': ['/tmp/foo'],
-            'additional_driver_flag': ['flag-specific']
+            'additional_driver_flag': ['--flag-not-affecting'],
+            'flag_specific':
+            'a',
         })
         port = self.make_port(options=options)
+        port.host.filesystem.write_text_file(
+            port.host.filesystem.join(port.web_tests_dir(),
+                                      'FlagSpecificConfig'),
+            '[{"name": "a", "args": ["--aa"]}]')
         self.assertEqual(list(port.used_expectations_files()), [
             port.path_to_generic_test_expectations_file(),
             port.path_to_webdriver_expectations_file(),
@@ -372,7 +378,7 @@ def test_used_expectations_files(self):
                                       'StaleTestExpectations'),
             port.host.filesystem.join(port.web_tests_dir(), 'SlowTests'),
             port.host.filesystem.join(port.web_tests_dir(), 'FlagExpectations',
-                                      'flag-specific'),
+                                      'a'),
             '/tmp/foo',
         ])
 

third_party/blink/tools/blinkpy/web_tests/port/test.py

@@ -716,13 +716,6 @@ def look_for_new_samples(self, crashed_processes, start_time):
                 sample_files[cp[0]] = sample_file
         return sample_files
 
-    def _flag_specific_expectations_path(self):
-        flags = [f[2:] for f in self._specified_additional_driver_flags()]
-        if not flags:
-            return None
-        return self._filesystem.join(self.web_tests_dir(), 'FlagExpectations',
-                                     flags[0])
-
     def look_for_new_crash_logs(self, crashed_processes, start_time):
         del start_time
         crash_logs = {}

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

@@ -187,6 +187,18 @@ def parse_args(args):
     option_group_definitions.append((
         'Results Options',
         [
+            optparse.make_option(
+                '--flag-specific',
+                dest='flag_specific',
+                action='store',
+                default=None,
+                help=
+                ('Name of a flag-specific configuration defined in FlagSpecificConfig. '
+                 'It is like a shortcut of --additional-driver-flag, '
+                 '--additional-expectations and --additional-platform-directory. '
+                 'When setting up flag-specific testing on bots, we should use '
+                 'this option instead of the discrete options. '
+                 'See crbug.com/1238155 for details.')),
             optparse.make_option(
                 '--additional-driver-flag',
                 '--additional-drt-flag',
@@ -196,14 +208,6 @@ def parse_args(args):
                 help=
                 ('Additional command line flag to pass to the driver. Specify multiple '
                  'times to add multiple flags.')),
-            optparse.make_option(
-                '--flag-specific',
-                dest='flag_specific',
-                action='store',
-                default=None,
-                help=
-                ('Name of a flag-specific configuration defined in FlagSpecificConfig, '
-                 ' as a shortcut of --additional-driver-flag options.')),
             optparse.make_option(
                 '--additional-expectations',
                 action='append',