Invoke webkit-patch in update-w3c-test-expectations

third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """A script to modify TestExpectations lines based layout test failures in try jobs.
 
 This script outputs a list of test expectation lines to add to a 'TestExpectations' file
 by retrieving the try job results for the current CL.
 """
 
 import logging
 
+from webkitpy.common.net import rietveld
 from webkitpy.common.net.buildbot import BuildBot
-from webkitpy.common.net import rietveld
-
+from webkitpy.common.webkit_finder import WebKitFinder
+from webkitpy.w3c.test_parser import TestParser
 
 _log = logging.getLogger(__name__)
 
 
 def main(host, port):
     expectations_file = port.path_to_generic_test_expectations_file()
     expectations_line_adder = W3CExpectationsLineAdder(host)
     issue_number = expectations_line_adder.get_issue_number()
     try_bots = expectations_line_adder.get_try_bots()
     try_jobs = rietveld.latest_try_jobs(issue_number, try_bots, host.web)
     test_expectations = {}
     if not try_jobs:
         print 'No Try Job information was collected.'
         return 1
     for job in try_jobs:
         platform_results = expectations_line_adder.get_failing_results_dict(BuildBot(), job.builder_name, job.build_number)
         test_expectations = expectations_line_adder.merge_dicts(test_expectations, platform_results)
     for test_name, platform_result in test_expectations.iteritems():
         test_expectations[test_name] = expectations_line_adder.merge_same_valued_keys(platform_result)
+    test_expectations = expectations_line_adder.get_expected_txt_files(test_expectations)
     test_expectation_lines = expectations_line_adder.create_line_list(test_expectations)
     expectations_line_adder.write_to_test_expectations(host, expectations_file, test_expectation_lines)
 
 
 class W3CExpectationsLineAdder(object):
 
     def __init__(self, host):
         self._host = host
         self.filesystem = host.filesystem
 
@@ skipping 216 matching lines @@
             all_lines += str(line) + '\n'
         all_lines = all_lines[:-1]
         if w3c_comment_line_index == -1:
             file_contents += '\n%s\n' % comment_line
             file_contents += all_lines
         else:
             end_of_comment_line = (file_contents[w3c_comment_line_index:].find('\n')) + w3c_comment_line_index
             new_data = file_contents[: end_of_comment_line + 1] + all_lines + file_contents[end_of_comment_line:]
             file_contents = new_data
         host.filesystem.write_text_file(path, file_contents)
+
+    def get_expected_txt_files(self, tests_results):
+        """Gets -expected.txt files.
+
+        Invokes webkit-patch rebaseline-from-try-jobs in order
+        to download new -expected.txt files for testharness.js
+        tests that did not Crash or Timeout. Then the platform-
+        specific test is removed from the overall failure test dictionary.
+
+        Args:
+            tests_results: A dictionary that maps test name to platforms to
+            test results.
+
+        Returns:
+            An updated tests_results dictionary without the platform-specific test-
+            harness.js tests that required new baselines to be downloaded from
+            webkit-patch rebaseline-from-try-jobs.
+        """
+        finder = WebKitFinder(self._host.filesystem)
+        layout_tests_relative_path = self._host.filesystem.relpath(finder.layout_tests_dir(), finder.chromium_base())
+        tests_to_rebaseline = []
+        tests = self._host.executive.run_command(['git', 'diff', 'master', '--name-only'])
+        for test_dir in tests.splitlines():
+            js_test = self.is_js_test(finder, test_dir)

[qyearsley, 2016/08/01 18:33:19]

`test_dir` is generally not a directory, right? It's a file which has been
modified by importing new tests, right?

[dcampb, 2016/08/02 17:56:57]

correct. The name 'test_dir' is a bit confusing. 
Changed to 'tests' instead. 

+            if js_test:

[qyearsley, 2016/08/01 18:33:19]

To simplify the above two lines and avoid adding another variable, you can call
self.is_js_test directly in the if expression.

[dcampb, 2016/08/02 17:56:57]

done

+                test_path = self._host.filesystem.relpath(test_dir, layout_tests_relative_path)
+                for platform in tests_results[test_path]:
+                    if tests_results[test_path][platform]['actual'] not in ['CRASH', 'TIMEOUT']:
+                        del tests_results[test_path][platform]
+                        tests_to_rebaseline.append(test_path)

[qyearsley, 2016/08/01 18:33:19]

It seems like you may be able to extract a helper function here, which takes a
list of changed files and a dict of test names to platforms to test results, and
returns a list of tests to rebaseline.

This helper function may be easier to test since it wouldn't depend on executive
-- it would require setting up a MockFileSystem with some JS files, and using an
example results dictionary and example list of changed files.

+        if tests_to_rebaseline:
+            webkit_patch = self._host.filesystem.join(finder.chromium_base(), finder.webkit_base(),
+                                                      finder.path_to_script('webkit-patch'))
+            self._host.executive.run_command(['python', webkit_patch,
+                                              'rebaseline-from-try-jobs', '-v',
+                                              ' '.join(tests_to_rebaseline)])
+        return tests_results

[qyearsley, 2016/08/01 18:33:19]

If we were to write a unit test for this function, I think it would:

 - Set up canned results for running the various commands using
`MockExecutive2`.
 - Assert that certain entries were removed from the dictionary (or not removed,
if there was nothing to remove).

If this would be helpful, you could consider adding a unit test for this whole
function. It might be easier to extract help functions and test those, which may
also be helpful.

[dcampb, 2016/08/02 17:56:57]

I have extracted helper functions and am currently writing unit test for them. 

+
+    def is_js_test(self, webkit_finder, test_path):

[qyearsley, 2016/08/01 18:33:18]

1. Is test_path relative to the LayoutTests directory, or relative to the
chromium/src repo root?

Normally for layout tests, "test path" and "test name" both refer to path
relative to LayoutTests.

[dcampb, 2016/08/02 17:56:57]

It is relative to layoutTests. 

+        absolute_path = self._host.filesystem.join(webkit_finder.chromium_base(), test_path)
+        test_parser = TestParser(None, absolute_path)
+        return test_parser.is_jstest()