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

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py
index 2a5ef4d82e380518af5a96f459fba4c41b05ec95..3440778d5004e280bd5c6f8a26ea085953076482 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py
@@ -10,9 +10,10 @@ by retrieving the try job results for the current CL.
 
 import logging
 
-from webkitpy.common.net.buildbot import BuildBot
 from webkitpy.common.net import rietveld
-
+from webkitpy.common.net.buildbot import BuildBot
+from webkitpy.common.webkit_finder import WebKitFinder
+from webkitpy.w3c.test_parser import TestParser
 
 _log = logging.getLogger(__name__)
 
@@ -32,6 +33,7 @@ def main(host, port):
         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)
 
@@ -268,3 +270,45 @@ class W3CExpectationsLineAdder(object):
             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()