Create test expectations line automatically from failing test results

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 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 = {}
+    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)
+
+
+class W3CExpectationsLineAdder(object):
+
+    def __init__(self, host):
+        self._host = host
+
+    def get_issue_number(self):
+        return self._host._scm.get_issue_number()

[qyearsley, 2016/07/20 16:55:28]

This line is accessing a private attribute of another object directly (_scm
attribute of Host object). In this case it's not really dangerous, but as a
general principle accessing private attributes should always be avoided.

Host has a method scm() which could be used here:

  return self._host.scm().get_issue_number()

[dcampb, 2016/07/20 21:11:59]

done

+
+    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):
+        """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()
+        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, final, temp, path=None):

[qyearsley, 2016/07/20 16:55:27]

A docstring could also be useful here. saying something like:

"""Recursively merges two dictionaries, modifying the first."""

Other notes:

The name `temp` doesn't provide much information. Maybe call the arguments
`target` and `source` since the first one is being modified?


Alternatively, this this function could also be changed to return a new dict and
not modify either of its arguments; in that case the arguments could be called
`first` and `second`.

[dcampb, 2016/07/20 21:11:59]

I chose the first option, I don't really see a benefit in returning a new
dictionary. It's a preference of mine to just update the first parameter. 

[qyearsley, 2016/07/20 21:21:45]

Sounds good -- and with the docstring and argument names changed, it should be
pretty clear to future readers that that's the behavior.

+        path = path or []
+        for key in temp:
+            if key in final:
+                if (isinstance(final[key], dict)) and isinstance(temp[key], dict):
+                    self.merge_dicts(final[key], temp[key], path + [str(key)])
+                elif final[key] == temp[key]:
+                    pass
+                else:
+                    raise Exception('conflict at %s' % '.'.join(path))

[qyearsley, 2016/07/20 16:55:28]

Usually it's better to raise a more specific exception, so that callers can
catch a more specific exception.

One possible exception to use here would be ValueError, which usually indicates
"something about the value of the passed-in argument was incorrect".

[dcampb, 2016/07/20 21:11:59]

Changed it to a value error, with the conflict line still there for debugging. 

+            else:
+                final[key] = temp[key]
+        return final
+
+    def merge_same_valued_keys(self, dictionary):
+        """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.
+            Example: {
+                'one': {'foo': 'bar'},
+                'two': {'foo': 'bar'},
+                'three': {'foo': bar'}
+                     }     is converted to

[qyearsley, 2016/07/20 16:55:28]

Indentation is off here -- maybe due to tab characters?

[dcampb, 2016/07/20 21:11:59]

fixed.

+                {('one', 'two', 'three'): {'foo': 'bar'}}
+        """
+        matching_value_keys = set()
+        keys = dictionary.keys()
+        is_last_item = False
+        for index, item in enumerate(keys):
+            if is_last_item:
+                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
+
+    def get_expectations(self, results):
+        '''Returns a list of test expectations for a given test dict.

[qyearsley, 2016/07/20 16:55:27]

Use double-quotes (""") for docstrings for consistency

[dcampb, 2016/07/20 21:11:59]

fixed.

[dcampb, 2016/07/20 21:11:59]

fixed.

[dcampb, 2016/07/20 21:11:59]

fixed.

+
+        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']
+        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())
+        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))
+        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.
+
+        Args:
+            host: A Host object.
+            path: The path to the file LayoutTest/TestExpectations.
+            line_list: A list of w3c test expectations lines.
+
+        Returns:
+            Writes to a file on the filesystem called LayoutTests/TestExpectations.
+        """
+        file_contents = host.filesystem.read_text_file(path)
+        w3c_comment_line_index = file_contents.find('# Tests added from W3C auto import bot')

[qyearsley, 2016/07/20 16:55:28]

The string '# Tests added from W3C auto import bot' could be a global constant.
It could also be re-used below on line 236.

[dcampb, 2016/07/20 21:11:59]

made it into a variable in the function. Don't use it outside this function so
no need for a global variable. 

[qyearsley, 2016/07/20 21:21:45]

Sounds fine to me.

Note, sometimes people extract literal strings and numbers up to the top of the
file even if they're only used in one place -- the main advantage of this is
that "magical values" are then grouped at the top of the file, and when one
wants to tweak a "magical value", then any change is at the top of the file.

But I think that this is a matter of personal preference, and having as few
global values as possible seems like a good idea to me.

+        all_lines = ''
+        for line in line_list:
+            all_lines += str(line) + '\n'
+        all_lines = all_lines[:-1]
+        if w3c_comment_line_index == -1:
+            file_contents += '\n# Tests added from W3C auto import bot\n'
+            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)