Removes build and builder object refrences along with minor bug fixes

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 f74cfec2b08689fa8df3ffc05eaaa330ce405045..4a17f035ef5a5a048ed1507aa063e93f10d7906e 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,7 +10,7 @@ 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
 
 
@@ -23,20 +23,20 @@ def main(host, port):
     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)
 
 
@@ -65,7 +65,7 @@ class W3CExpectationsLineAdder(object):
                 }}
         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
@@ -84,8 +84,8 @@ class W3CExpectationsLineAdder(object):
                 }
             }
         """
-        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)
@@ -129,29 +129,40 @@ class W3CExpectationsLineAdder(object):
             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.
@@ -172,16 +183,18 @@ class W3CExpectationsLineAdder(object):
             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):