Create test expectations line automatically from failing test results

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

+"""TODO(dcampb): DO NOT SUBMIT without one-line documentation for print-layout-test-paths."""

[qyearsley, 2016/07/11 23:17:33]

This line can be removed, and at the top of the file you should add:

# 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.

[dcampb, 2016/07/12 03:55:55]

Corrections made. 

+from webkitpy.common.net.layouttestresults import LayoutTestResults
+from webkitpy.layout_tests.models.test_expectations import *

[qyearsley, 2016/07/11 23:17:33]

It's best to avoid "wildcard imports" like these, because they may "pollute the
global namespace"; that is, it defines some names at the global level, which may
potentially conflict with other names. The other disadvantage of wildcard
imports It's not clear exactly what's being used from the source module.

See http://legacy.python.org/dev/peps/pep-0008/#imports

[dcampb, 2016/07/12 03:55:56]

Ahh, never knew that. Changes made.

+from webkitpy.common.net.web import Web
+from webkitpy.common.net import buildbot
+from webkitpy.common.net import rietveld
+from webkitpy.common.checkout.scm.git import Git

[qyearsley, 2016/07/11 23:17:32]

Imports should be sorted, for consistency.

[dcampb, 2016/07/12 03:55:55]

sorted by directory, file name or module name?

[qyearsley, 2016/07/12 16:43:35]

Sorted lexicographically (which ends up to be by directory, generally) --
usually in vim I use `:'<,'>!sort`.

+import logging

[qyearsley, 2016/07/12 16:43:35]

One more thing about import section format: Standard library headers are usually
listed above imports from the project itself.

+
+_log = logging.getLogger(__name__)
+
+
+TRY_BOTS = ['linux_chromium_rel_ng', 'mac_chromium_rel_ng', 'win_chromium_rel_ng']
+
+
+def Main():

[qyearsley, 2016/07/11 23:17:32]

For consistency with most of the other scripts here, I think that the main
function is usually lower-cased and put at the bottom of the file (although I
personally think the top of the file is good).

[dcampb, 2016/07/12 03:55:55]

done

+    json_obj = {}

[qyearsley, 2016/07/11 23:17:33]

This name can be made more descriptive; how would you describe what it contains?
A good name should be 1-3 words, descriptive, unambiguous, and not too verbose.
Words like "obj", "data", and "info" should generally be avoided.

[dcampb, 2016/07/12 03:55:55]

Is that the same for local variable names in sub functions? 

[dcampb, 2016/07/12 03:55:55]

Is that the same for local variable names in sub functions? 

[qyearsley, 2016/07/12 16:43:35]

Yep -- there may be some cases where `obj, `data` and `info` are OK, but should
be avoided if possible, since they often don't make it clearer what the variable
does.

+    counter = 0
+    webo = Web()

[raikiri, 2016/07/11 18:46:55]

maybe rename webo to something a little more clear?

[qyearsley, 2016/07/11 23:17:33]

I'd suggest just calling it web.

[dcampb, 2016/07/12 03:55:55]

done, web_object?

[dcampb, 2016/07/12 19:57:03]

just made it web, per qyearsley@ comment on line 29.

+    try_jobs_info = get_tryjobs_information(webo)
+    while counter < len(TRY_BOTS):
+        failing_paths = get_tryjob_results(try_jobs_info, webo, counter)
+        json_obj = merge_dicts(json_obj, failing_paths)
+        counter += 1
+    return 0
+
+
+def get_tryjobs_information(web):

[qyearsley, 2016/07/11 23:17:33]

tryjobs -> try_jobs

I prefer to treat this as two words, since that's how it is in the official
documentation (https://www.chromium.org/developers/testing/try-server-usage).

[dcampb, 2016/07/12 03:55:55]

done

+    git_obj = Git('.')

[qyearsley, 2016/07/11 23:17:33]

You could rename git_obj -> git; the _obj suffix doesn't add much information,
and since it's only used in this short function, it's clear that it's an
instance of Git.

[dcampb, 2016/07/12 03:55:55]

makes sense, did the same for the web object.

+    issue_number = git_obj.get_issue_number()
+    info = rietveld.latest_try_jobs(issue_number, TRY_BOTS, web)
+    return info
+
+
+def _generate_results_dict(layout_test_list, test_results_object):
+    test_dict = {}
+    builder_name = test_results_object.builder_name()
+    if test_results_object.builder_name() in TRY_BOTS:
+        platform = builder_name[:builder_name.find("_")]

[qyearsley, 2016/07/11 23:17:33]

We can't assume that builder names are always for the form <platform>_something;
I suspect that what we actually want to use is "port name", or possibly another
name which we can get from the port name; see
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

[dcampb, 2016/07/15 00:24:13]

taken care of. 

+    for obj in layout_test_list:

[qyearsley, 2016/07/11 23:17:33]

The name `obj` can be improved. In this case, it's a LayoutTestResult object, so
you can call it `result`. If it were in scope outside of this short function,
I'd suggest calling it layout_test_result.

[dcampb, 2016/07/12 03:55:55]

done

+        test_dict[obj.test_name()] = {platform: {'expected': obj.expected_results(), 'actual': obj.actual_results(), 'bug': 'crbug.com/626703'}}
+    return test_dict
+
+
+def _strip_failing_results(data):
+    test_object = LayoutTestResults.results_from_string(data)
+    failing_paths = test_object.didnt_run_as_expected_results()
+    return _generate_results_dict(failing_paths, test_object)
+
+
+def get_tryjob_results(try_jobs_data, web, index):

[qyearsley, 2016/07/11 23:17:32]

tryjob -> try_job

[dcampb, 2016/07/12 03:55:55]

done

+    builder_name = try_jobs_data[index][0]
+    build_number = try_jobs_data[index][1]
+    builder = buildbot.Builder(builder_name, buildbot.BuildBot())
+    url_components = (builder.results_url(), str(build_number), 'layout-test-results', 'failing_results.json')
+    results_url = '/'.join(url_components)

[qyearsley, 2016/07/11 23:17:32]

The results URL can be constructed by methods on the Builder class in
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...,
I believe.

[dcampb, 2016/07/12 03:55:56]

I believe I tried using it and it was giving me an error. I will try it again
just to confirm. 

+    json_data = web.get_binary(results_url)
+    failing_results_paths = _strip_failing_results(json_data)
+    return failing_results_paths
+
+
+def _remove_duplicates(failing_paths):
+    seen = set()
+    new_list = []
+    for dictionary in failing_paths:
+        path = tuple(dictionary.items())
+    if path not in seen:
+        seen.add(path)
+        new_list.append(dict(path))
+    return new_list
+
+
+def merge_dicts(final, temp, path=None):
+    if path is None:
+        path = []

[raikiri, 2016/07/11 18:46:55]

can also be written:

path = path or []

+    for key in temp:
+        if key in final:
+            if (isinstance(final[key], dict)) and isinstance(temp[key], dict):
+                merge_dicts(final[key], temp[key], path + [str(key)])

[raikiri, 2016/07/11 18:46:55]

iiuc this line seems to do nothing as you don't keep the value that's returned

[dcampb, 2016/07/12 03:55:55]

this line recursively calls the function with a sub dictionary all the way down
until final[key] is no longer a dictionary. The str(key) is there for debugging
purposes, if there is a merging conflict. 

+            elif final[key] == temp[key]:
+                pass
+            else:
+                raise Exception('Conflict at %s' % '.'.join(path))
+        else:
+            final[key] = temp[key]
+    return final
+
+
+if __name__ == '__main__':
+    Main()