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 = {}
     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():
-        platform_results_dicts = expectations_line_adder.merge_same_valued_keys(platform_results_dicts)
-    line_list = expectations_line_adder.create_line_list(line_expectations_dict)
-    expectations_line_adder.write_to_test_expectations(host, expectations_file, line_list)
+    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_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
 
     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)
         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'}
+                'three': {'foo': 'bar'}
             }
-            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())
+        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)):
-                next_item = keys[i]
-                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
+            for next_item in keys[1:]:
+                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]
+                        keys = [k for k in keys if k not in matching_value_keys]
+                    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.

[qyearsley, 2016/07/26 23:59:43]

In particular, it's a list of a expectations which should be added as a new line
in TestExpectations, right?

[dcampb, 2016/07/27 16:26:36]

This function returns a list of actual test expectations for a given test. for
example, 'Timeout', 'Failure', 'Pass'. 
This list is used in the function that creates the test-expectations line list.
Example, ['crbug.com/626777 [  Win7 ] fast/new/foo/bar.html [ Failure ]'] 

[qyearsley, 2016/07/27 17:25:39]

Right, so the output of this function determines the expectation type(s) in the
new test expectations line. This could optionally be added to the docstring
(although this is unnecessary since how the function is used should usually be
understood by looking at the code).

[dcampb, 2016/07/27 18:17:50]

agreed. 

 
         Returns a list of one or more test expectations based on the expected
         and actual results of a given test name.

[qyearsley, 2016/07/27 17:25:39]

This function has a precondition or assumption that only result dicts for
results where the test didn't run as expected are passed in -- it's helpful to
list preconditions and assumptions with the function documentation.

 
         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']

[qyearsley, 2016/07/26 23:59:43]

This comment can be updated now that this function returns a set.

[dcampb, 2016/07/27 18:17:49]

done

         """
-        expectations = []
-        failure_expectations = ['TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE']
+        expectations = set()
+        failure_expectations = ['SLOW', 'TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE']

[qyearsley, 2016/07/26 23:59:43]

I'm not sure if SLOW counts as a failure, and also I don't think SLOW is ever in
results["actual"] -- if SLOW is in results["expected"] for a test, I think that
also means it's expected to pass. So, I think maybe SLOW can be removed from
here.

Also, in theory there's one more "baseline mismatch" failure type: AUDIO. I'm
pretty sure there are no tests with audio baselines in web-platform-tests, but
you could add it here for completeness I think.

[dcampb, 2016/07/27 16:26:36]

I do remember seeing an example of an expected that was SLOW TIMEOUT CRASH FAIL,
however in TestExpectations the only expectations are 'Failure, Pass, Timeout'.
Which is why I added SLOW to the list. 

I will look around for all the possible failure types and expected types and see
which ones results in which expectation.

[qyearsley, 2016/07/27 17:25:39]

Alright; here shouldn't be any more "baseline mismatch" types besides those
four. Note, the "official" documentation for the results JSON format is
https://www.chromium.org/developers/the-json-test-results-format.

         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 23:59:43]

This morning I was saying that if there are multiple words in results["actual"]
then it means the test was retried. If there are multiple words in
results["expected"], though, I believe that this means that any of those
expected results are OK. For example, if expected is "PASS CRASH" and actual is
"CRASH" OR "PASS", then that's OK, and no new expectation line needs to be
added.

Would it also make sense to rewrite this function to take a LayoutTestResult
object (from
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...)

[dcampb, 2016/07/27 16:26:36]

Would that result in a mismatch result? or wouldn't that test pass?

 If so, it would not be ran through this script. This script only deals with
results with mismatch results. 

[dcampb, 2016/07/27 16:44:24]

Correction: This script only works with tests that did not run as expected.
However, I don't think that made a difference, as if it was expected to 'Crash'
or 'Pass' and Passes, then it would not have been reported as a failure. 

[qyearsley, 2016/07/27 17:25:39]

What I meant was, since the dict passed in contains basically the same
information as a LayoutTestResult object, I was wondering whether it would make
sense later to refactor the rest of the code to use LayoutTestResult objects --
but now I see that this isn't a simple refactoring if it were done, and
shouldn't be done in this CL.

[dcampb, 2016/07/27 18:17:49]

I see what your saying. I agree, it could be refactored in a later cl. 

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

[qyearsley, 2016/07/26 23:59:43]

What if expected is "PASS" and actual is "PASS" -- then we don't want to add any
new expectation line, right?

[dcampb, 2016/07/27 16:26:36]

That wouldn't happen as this script only parses test that have mismatch results.

[dcampb, 2016/07/27 16:44:24]

Correction: This script parses tests that did not run as expected. therefore,
the expected and pass would never be the same.

Note: There is a function in LayoutTestResults called
unexpected_mismatch_results().. not exactly sure how different that is from
didnt_run_as_expected().

[qyearsley, 2016/07/27 17:25:39]

Yeah, those names aren't super clear. "Mismatch" was supposed to mean "mismatch
with existing baselines", i.e. TEXT, IMAGE, etc.

[dcampb, 2016/07/27 18:17:50]

understood. I think it would be wise to get this cl up first, then take into
account tests that need -expected.txt files added later. 

         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 {
                 'test_name': {
                     'platform': {
                         'expected: 'PASS',
                         'actual': 'FAIL',
                         'bug': 'crbug.com/11111'
                     }
                 }
             }
                 It is possible for the dicitonary to have many test_name
                 keys.
 
         Returns:
             A list of test expectations lines with the format
             ['BUG_URL [PLATFORM(S)] TEST_MAME [EXPECTATION(S)]']
         """
         line_list = []
         for test_name, platform_results in merged_results.iteritems():
             for platform in platform_results:
-                platform_list = []
-                bug = []
-                expectations = []
-                if isinstance(platform, tuple):
-                    platform_list = list(platform)
-                else:
-                    platform_list.append(platform)
-                bug.append(platform_results[platform]['bug'])
-                expectations = self.get_expectations(platform_results[platform])
-                line = '%s [ %s ] %s [ %s ]' % (bug[0], ' '.join(platform_list), test_name, ' '.join(expectations))
-                line_list.append(str(line))
+                if test_name.startswith('imported'):
+                    print platform_results
+                    platform_list = []
+                    bug = []
+                    expectations = []
+                    if isinstance(platform, tuple):
+                        platform_list = list(platform)
+                    else:
+                        platform_list.append(platform)
+                    bug.append(platform_results[platform]['bug'])
+                    expectations = self.get_expectations(platform_results[platform])
+                    line = '%s [ %s ] %s [ %s ]' % (bug[0], ' '.join(platform_list), test_name, ' '.join(expectations))
+                    line_list.append(str(line))
         return line_list
 
     def write_to_test_expectations(self, host, path, line_list):
         """Writes to TestExpectations.
 
         Writes to the test expectations lines in line_list
         to LayoutTest/TestExpectations. Checks the file for the string
         '# Tests added from W3C auto import bot' and writes expectation
         lines directly under it. If not found, it writes to the end of
         the file. If the test name is already in LayoutTests/TestExpectations,
@@ skipping 19 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)