try-flag: Intersect unexpected passes with the flag expectation file.

Previously, try-flag reported all unexpected passes, even if the failure
expectation came from the main TestExpectations file.  This produced a lot of
irrelevant noise.

Now, try-flag only reports unexpected passes of tests that have flag-specific
failure expectations.

The --flag argument is no longer optional.

Bug: 755401
Change-Id: Ifa507e19159b8cdadf63752f5c8aa2635de014d5
Reviewed-on: https://chromium-review.googlesource.com/794877
Commit-Queue: Steve Kobes <skobes@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519950}

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/try_flag.py

@@ -47,29 +47,41 @@ class TryFlag(object):
         self._path_finder = PathFinder(self._filesystem)
         self._git = self._host.git()
 
-    def _set_flag(self, flag):
+    def _force_flag_for_test_runner(self):
+        flag = self._args.flag
         path = self._path_finder.path_from_layout_tests(FLAG_FILE)
         self._filesystem.write_text_file(path, flag + '\n')
         self._git.add_list([path])
         self._git.commit_locally_with_message(
             'Flag try job: force %s for run-webkit-tests.' % flag)
 
-    def _clear_expectations(self, flag):
-        path = self._path_finder.path_from_layout_tests(
-            'FlagExpectations', flag.lstrip('-'))
+    def _flag_expectations_path(self):
+        return self._path_finder.path_from_layout_tests(
+            'FlagExpectations', self._args.flag.lstrip('-'))
+
+    def _clear_expectations(self):
+        path = self._flag_expectations_path()
         self._filesystem.write_text_file(path, '')
         self._git.add_list([path])
         self._git.commit_locally_with_message(
-            'Flag try job: clear expectations for %s.' % flag)
+            'Flag try job: clear expectations for %s.' % self._args.flag)
+
+    def _tests_in_flag_expectations(self):
+        result = set()
+        path = self._flag_expectations_path()
+        for line in self._filesystem.read_text_file(path).split('\n'):
+            expectation_line = TestExpectationLine.tokenize_line(path, line, 0)
+            test_name = expectation_line.name
+            if test_name:
+                result.add(test_name)
+        return result
 
     def trigger(self):
-        flag = self._args.flag
-        if flag:
-            self._set_flag(flag)
-            if self._args.regenerate:
-                self._clear_expectations(flag)
-            self._git_cl.run(['upload', '--bypass-hooks', '-f',
-                              '-m', 'Flag try job for %s.' % flag])
+        self._force_flag_for_test_runner()
+        if self._args.regenerate:
+            self._clear_expectations()
+        self._git_cl.run(['upload', '--bypass-hooks', '-f',
+                          '-m', 'Flag try job for %s.' % self._args.flag])
         for builder in sorted(BUILDER_MASTERS.keys()):
             master = BUILDER_MASTERS[builder]
             self._git_cl.trigger_try_jobs([builder], master)
@@ -106,7 +118,7 @@ class TryFlag(object):
         # TODO: Get jobs from the _tryflag branch. Current branch for now.
         jobs = self._git_cl.latest_try_jobs(BUILDER_CONFIGS.keys())
         buildbot = self._host.buildbot
-        for build in sorted(jobs.keys()):
+        for build in sorted(jobs):
             self._host.print_('-- %s: %s/results.html' % (
                 BUILDER_CONFIGS[build.builder_name].version,
                 buildbot.results_url(build.builder_name, build.build_number)))
@@ -117,11 +129,14 @@ class TryFlag(object):
         # TODO: Write to flag expectations file. For now, stdout. :)
         unexpected_failures = []
         unexpected_passes = []
+        tests_in_flag_expectations = self._tests_in_flag_expectations()
         for line in self._expectations_model.all_lines():
-            if TestExpectations.EXPECTATIONS['pass'] in line.parsed_expectations:
-                unexpected_passes.append(line)
-            else:
+            is_pass = (TestExpectations.EXPECTATIONS['pass'] in
+                       line.parsed_expectations)
+            if not is_pass:
                 unexpected_failures.append(line)
+            elif line.name in tests_in_flag_expectations:
+                unexpected_passes.append(line)
 
         self._print_all(unexpected_passes, 'unexpected passes')
         self._print_all(unexpected_failures, 'unexpected failures')
@@ -150,7 +165,7 @@ def parse_args(argv):
         formatter_class=argparse.RawDescriptionHelpFormatter)
     parser.add_argument('action', help='"trigger" or "update"')
     parser.add_argument('--bug', help='crbug number for expectation lines')
-    parser.add_argument('--flag',
+    parser.add_argument('--flag', required=True,
                         help='flag to force-enable in run-webkit-tests')
     parser.add_argument('--regenerate', action='store_true',
                         help='clear the flag expectations before triggering')

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/try_flag_unittest.py

@@ -19,6 +19,11 @@ class TryFlagTest(unittest.TestCase):
         self.linux_build = Build('linux_chromium_rel_ng', 100)
         self.mac_build = Build('mac_chromium_rel_ng', 101)
         self.win_build = Build('win7_chromium_rel_ng', 102)
+        self.mock_try_results = {
+            self.linux_build: TryJobStatus('COMPLETED', 'SUCCESS'),
+            self.win_build: TryJobStatus('COMPLETED', 'SUCCESS'),
+            self.mac_build: TryJobStatus('COMPLETED', 'SUCCESS')
+        }
         super(TryFlagTest, self).__init__(*args, **kwargs)
 
     def _run_trigger_test(self, regenerate):
@@ -115,13 +120,18 @@ class TryFlagTest(unittest.TestCase):
 
     def test_update(self):
         host = MockHost()
-        try_jobs = {
-            self.linux_build: TryJobStatus('COMPLETED', 'SUCCESS'),
-            self.win_build: TryJobStatus('COMPLETED', 'SUCCESS'),
-            self.mac_build: TryJobStatus('COMPLETED', 'SUCCESS')
-        }
+        filesystem = host.filesystem
+        finder = PathFinder(filesystem)
+
+        flag_expectations_file = finder.path_from_layout_tests(
+            'FlagExpectations', 'foo')
+        filesystem.write_text_file(
+            flag_expectations_file,
+            'something/pass-unexpectedly-mac.html [ Fail ]')
+
         self._setup_mock_results(host.buildbot)
-        TryFlag(['update'], host, MockGitCL(host, try_jobs)).run()
+        cmd = ['update', '--flag=--foo']
+        TryFlag(cmd, host, MockGitCL(host, self.mock_try_results)).run()
 
         def results_url(build):
             return '%s/%s/%s/layout-test-results/results.html' % (
@@ -146,8 +156,24 @@ class TryFlagTest(unittest.TestCase):
             ''
         ]))
 
+    def test_update_irrelevant_unexpected_pass(self):
+        host = MockHost()
+        filesystem = host.filesystem
+        finder = PathFinder(filesystem)
+        flag_expectations_file = finder.path_from_layout_tests(
+            'FlagExpectations', 'foo')
+        self._setup_mock_results(host.buildbot)
+        cmd = ['update', '--flag=--foo']
+
+        # Unexpected passes that don't have flag-specific failure expectations
+        # should not be reported.
+        filesystem.write_text_file(flag_expectations_file, '')
+        TryFlag(cmd, host, MockGitCL(host, self.mock_try_results)).run()
+        self.assertTrue('### 0 unexpected passes' in host.stdout.getvalue())
+
     def test_invalid_action(self):
         host = MockHost()
-        TryFlag(['invalid'], host, MockGitCL(host)).run()
+        cmd = ['invalid', '--flag=--foo']
+        TryFlag(cmd, host, MockGitCL(host)).run()
         self.assertEqual(host.stderr.getvalue(),
                          'specify "trigger" or "update"\n')