Removes build and builder object refrences along with minor bug fixes

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 buildbot
+from webkitpy.common.net.buildbot import BuildBot
 from webkitpy.common.net import rietveld
 
 
 _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)
-    line_expectations_dict = {}
+    test_expectations_dict = {}
     if not try_jobs:
         print 'No Try Job information was collected.'
         return 1
     for try_job in try_jobs:
         builder_name = try_job[0]
         build_number = try_job[1]
-        builder = buildbot.Builder(builder_name, host.buildbot)
-        build = buildbot.Build(builder, build_number)
-        platform_results_dict = expectations_line_adder.get_failing_results_dict(builder, build)
-        line_expectations_dict = expectations_line_adder.merge_dicts(line_expectations_dict, platform_results_dict)
-    for platform_results_dicts in line_expectations_dict.values():
+        buildbot = BuildBot()
+        platform_results_dict = expectations_line_adder.get_failing_results_dict(buildbot, builder_name, build_number)

[qyearsley, 2016/07/26 21:40:35]

Somewhat unrelated to this CL: An alternative way of writing the above several
lines would be:

for job in try_jobs:
    platform_results =
expectations_line_adder.get_failing_results_dict(BuildBot(), job.builder_name,
job.build_number)
    ...

Several points here:
 - Adding the type to the variable name (e.g. _dict, _list) isn't always
clearer, and it makes the variable name longer.
 - Fewer variables and lines is often simpler.
 - For "named tuple" classes in Python (like TryJob), you can access attributes
by name.

[dcampb, 2016/07/26 22:08:35]

suggestions noted and changed made. 

+        test_expectations_dict = expectations_line_adder.merge_dicts(test_expectations_dict, platform_results_dict)
+    for test_name, platform_results_dicts in test_expectations_dict.iteritems():
         platform_results_dicts = expectations_line_adder.merge_same_valued_keys(platform_results_dicts)
-    line_list = expectations_line_adder.create_line_list(line_expectations_dict)
+        test_expectations_dict[test_name] = platform_results_dicts
+    line_list = expectations_line_adder.create_line_list(test_expectations_dict)
     expectations_line_adder.write_to_test_expectations(host, expectations_file, line_list)
 
 
 class W3CExpectationsLineAdder(object):
 
     def __init__(self, host):
         self._host = host
         self.filesystem = host.filesystem
 
     def get_issue_number(self):
         return self._host.scm().get_issue_number()
 
     def get_try_bots(self):
         return self._host.builders.all_try_builder_names()
 
     def _generate_results_dict(self, platform, result_list):
         test_dict = {}
         if '-' in platform:
             platform = platform[platform.find('-') + 1:].capitalize()
         for result in result_list:
             test_dict[result.test_name()] = {
                 platform: {
                     'expected': result.expected_results(),
                     'actual': result.actual_results(),
                     'bug': 'crbug.com/626703'
                 }}
         return test_dict
 
-    def get_failing_results_dict(self, builder, build):
+    def get_failing_results_dict(self, buildbot, builder_name, build_number):
         """Returns a nested dict of failing test results.
 
         Retrieves a full list of layout test results from a builder result URL. Collects
         the builder name, platform and a list of tests that did not run as expected.
 
         Args:
             builder: A Builder object.
             build: A Build object.
 
         Returns:
             A dictionary with the structure: {
                 'key': {
                     'expected': 'TIMEOUT',
                     'actual': 'CRASH',
                     'bug': 'crbug.com/11111'
                 }
             }
         """
-        layout_test_results = builder.fetch_layout_test_results(build.results_url())
-        builder_name = layout_test_results.builder_name()
+        results_url = buildbot.results_url(builder_name, build_number)
+        layout_test_results = buildbot.fetch_layout_test_results(results_url)

[qyearsley, 2016/07/26 21:40:35]

Side note: In a subsequent CL (after this CL is landed), I think it might be
nice to add a method to BuildBot that is just "fetch_results", which does a
combination of getting the URL and then calling fetch_layout_test_results. When
I do this, I should update this code as well.

[dcampb, 2016/07/26 22:08:35]

sounds good. 

         platform = self._host.builders.port_name_for_builder_name(builder_name)
         result_list = layout_test_results.didnt_run_as_expected_results()
         failing_results_dict = self._generate_results_dict(platform, result_list)
         return failing_results_dict
 
     def merge_dicts(self, target, source, path=None):
         """Recursively merge nested dictionaries, returning the target dictionary
 
         Merges the keys and values from the source dict into the target dict.
 
@@ skipping 23 matching lines @@
         """Merges keys in dictionary with same value.
 
         Traverses through a dict and compares the values of keys to one another.
         If the values match, the keys are combined to a tuple and the previous keys
         are removed from the dict.
 
         Args:
             dictionary: A dictionary with a dictionary as the value.
 
         Returns:
-            A dictionary with updated keys to reflect matching values of keys.
+            A new dictionary with updated keys to reflect matching values of keys.
             Example: {
                 'one': {'foo': 'bar'},
                 'two': {'foo': 'bar'},
                 'three': {'foo': bar'}

[qyearsley, 2016/07/26 21:40:35]

Unrelated to this CL, but there's a quote missing before the last "bar".

[dcampb, 2016/07/26 22:08:35]

ahh, nice eye. done

             }
-            is converted to {('one', 'two', 'three'): {'foo': 'bar'}}
+            is converted to a new dictionary with that contains
+            {('one', 'two', 'three'): {'foo': 'bar'}}
         """
+        merged_dict = {}
         matching_value_keys = set()
-        keys = dictionary.keys()
-        is_last_item = False
-        for index, item in enumerate(keys):
-            if is_last_item:
+        keys = sorted(dictionary.keys())

[qyearsley, 2016/07/26 21:40:35]

Same as `keys = sorted(dictionary)` (both are OK).

[dcampb, 2016/07/26 22:08:35]

noted

+        while keys:
+            current_key = keys[0]
+            found_match = False
+            if current_key == keys[-1]:
+                merged_dict[current_key] = dictionary[current_key]
+                keys.remove(current_key)
                 break
-            for i in range(index + 1, len(keys)):
+            for i in range(1, len(keys)):
                 next_item = keys[i]

[qyearsley, 2016/07/26 21:40:36]

If `i` isn't used below, then maybe you could also do `for next_item in
keys[1:]: ...`

[dcampb, 2016/07/26 22:08:35]

nicee, noted.

-                if dictionary[item] == dictionary[next_item]:
-                    matching_value_keys.update([item, next_item])
-                    dictionary[tuple(matching_value_keys)] = dictionary[item]
-                    is_last_item = next_item == keys[-1]
-                    del dictionary[item]
-                    del dictionary[next_item]
-        return dictionary
+                if dictionary[current_key] == dictionary[next_item]:
+                    found_match = True
+                    matching_value_keys.update([current_key, next_item])
+                if next_item == keys[-1]:
+                    if found_match:
+                        merged_dict[tuple(matching_value_keys)] = dictionary[current_key]
+                        for key in matching_value_keys:
+                            keys.remove(key)

[qyearsley, 2016/07/26 21:40:35]

For many short for loops, you can sometimes write it on one line using list
comprehensions, e.g.:

  keys = [k for k in keys if k not in matching_value_keys]

[dcampb, 2016/07/26 22:08:35]

nice tip, thanks.
done

+                    else:
+                        merged_dict[current_key] = dictionary[current_key]
+                        keys.remove(current_key)
+            matching_value_keys = set()
+        return merged_dict
 
     def get_expectations(self, results):
         """Returns a list of test expectations for a given test dict.
 
         Returns a list of one or more test expectations based on the expected
         and actual results of a given test name.
 
         Args:
             results: A dictionary that maps one test to its results. Example: {
                 'test_name': {
                     'expected': 'PASS',
                     'actual': 'FAIL',
                     'bug': 'crbug.com/11111'
                 }
             }
 
         Returns:
             A list of one or more test expectations with the first letter capitalized. Example:
             ['Failure', 'Timeout']
         """
-        expectations = []
-        failure_expectations = ['TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE']
+        expectations = set()
+        failure_expectations = ['TIMEOUT', 'CRASH', 'SLOW', 'TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE']
         pass_crash_timeout = ['TIMEOUT', 'CRASH', 'PASS']
-        if results['expected'] in pass_crash_timeout and results['actual'] in failure_expectations:
-            expectations.append('Failure')
-        if results['expected'] in failure_expectations and results['actual'] in pass_crash_timeout:
-            expectations.append(results['actual'].capitalize())
-        if results['expected'] in pass_crash_timeout and results['actual'] in pass_crash_timeout:
-            expectations.append(results['actual'].capitalize())
-            expectations.append(results['expected'].capitalize())
+        for expected in results['expected'].split():
+            for actual in results['actual'].split():

[qyearsley, 2016/07/26 21:40:35]

For this change, I think it will be good to have unit test methods that cover
the cases when we have multiple expectations in "expected" and/or multiple
results in "actual".

[dcampb, 2016/07/26 22:08:35]

working on this now.

+                if expected in pass_crash_timeout and actual in failure_expectations:
+                    expectations.update(['Failure'])
+                if expected in failure_expectations and actual in pass_crash_timeout:
+                    expectations.update([actual.capitalize()])
+                if expected in pass_crash_timeout and actual in pass_crash_timeout:
+                    expectations.update([actual.capitalize()])
+                    expectations.update([expected.capitalize()])

[qyearsley, 2016/07/26 21:40:35]

For sets, if you're adding only one item, you can use the `add` method. So,
instead of `expectations.update([expected.capitalize()])`, you can write
`expectations.add(expected.capitalize())`.

[dcampb, 2016/07/26 22:08:35]

noted and done. 

         return expectations
 
     def create_line_list(self, merged_results):
         """Creates list of test expectations lines.
 
         Traverses through a merged_results and parses the value to create a test
         expectations line per key.
 
         Args:
             merged_results: A merged_results with the format {
@@ skipping 58 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)