Added support for versioning of layout test results of run-webkit-tests runs

Tools/Scripts/webkitpy/layout_tests/generate_results_dashboard.py

+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+import json
+import logging
+import string
+
+from webkitpy.layout_tests.port import configuration_options, platform_options

[Dirk Pranke, 2014/07/15 20:50:01]

Do you need this import? I don't think so; you probably don't need the logging
and string imports, either.

[patro, 2014/07/16 14:37:37]

Done.

+
+_log = logging.getLogger(__name__)
+
+
+class GenerateDashBoard(object):
+
+    "A class for generating the Dashboard from the list of archived results"

[Dirk Pranke, 2014/07/15 20:50:01]

This docstring doesn't tell us much; I'd probably delete it.

[patro, 2014/07/16 14:37:36]

Done.

+
+    def __init__(self, port):
+        self._port = port
+        self._filesystem = port.host.filesystem
+        self._results_directory = self._port.results_directory()
+        self._release_directory = self._filesystem.join(self._filesystem.dirname(self._results_directory), '')

[Dirk Pranke, 2014/07/15 20:50:00]

You don't need the .join() here; dirname() gives you what you need. 

'release_directory' is misleading if you are writing into a custom results
directory or using a debug build. 

You need to decide if you need to support versioned results with custom
directories, and, if so, how that should work. One way that might be good enough
would be to move dashboard.html and archived_results.json into the
layout-test-results directory, and then link back to prior layout-test-results
directories if you find them next to the current directory. 

This would remove the need to overwrite archived_results.json on each run, and
would keep you from writing to files outside of layout-test-results (though you
would still be renaming directories).

It would also be good to have a link from 'results.html' to the dashboard
directly, and if you keep results.html and dashboard.html next to each other,
you can probably reuse some of the javascript between the two files.

[patro, 2014/07/16 14:37:36]

Renaming it to self._results_directory_path

Yes if we move the dashboard and archived results json inside the results
directory we need not write outside the layout-results-directory. This can allow
to have unique dashboard for each test. I will modify this in next patch.

I already added a link from results.html to dashboard.

+        self._input_json_data = ""

[Dirk Pranke, 2014/07/15 20:50:01]

Is this supposed to be a string or a dict? Why not just use
self._result_directory_list instead (here and below)?

[patro, 2014/07/16 14:37:37]

It is a dict.
Renaming it ro self._current_result_json_dict

[Dirk Pranke, 2014/07/16 19:32:44]

If it's a dict, why are you assigning a string to it?

+        self._old_failing_results_list = []
+        self._old_full_results_list = []
+        self._final_result = []
+
+    def _add_individual_result_links(self, file_list):

[Dirk Pranke, 2014/07/15 20:50:00]

this is actually a list of directories, not a list of files, right? You should
probably fix the parameter name to be clearer, if so (e.g.,
'results_directories').

[patro, 2014/07/16 14:37:37]

Done.

+        file_list = [(file + '/results.html') for file in file_list]
+        self._input_json_data['result_links'] = file_list
+
+    def _copy_dashboard_html(self):
+        dashboard_file = self._filesystem.join(self._release_directory, 'dashboard.html')
+        dashboard_html_file_path = self._filesystem.join(self._port.layout_tests_dir(), 'fast/harness/dashboard.html')
+        if ~(self._filesystem.exists(dashboard_file)):

[Dirk Pranke, 2014/07/15 20:50:01]

this should be 'if not self._filesystem.exists(dashboard_file):' . Don't use the
outside parens, and definitely don't use '~' :).

[patro, 2014/07/16 14:37:36]

Done.

+            if self._filesystem.exists(dashboard_html_file_path):
+                self._filesystem.copyfile(dashboard_html_file_path, dashboard_file)
+
+    def _initialize(self):
+        file_list = self._filesystem.listdir(self._release_directory)
+        json_file_list = []

[Dirk Pranke, 2014/07/15 20:50:01]

json_file_list isn't a list of json files, it's a list of results directories.

[patro, 2014/07/16 14:37:36]

Done.

+        for dir in file_list:
+            if self._filesystem.isdir(self._filesystem.join(self._release_directory, dir)):
+                json_file_list.append(self._filesystem.join(self._release_directory, dir))
+        json_file_list.sort(reverse=True)
+        #Read the current Layout Test Results

[Dirk Pranke, 2014/07/15 20:50:00]

if you are going to add comments, add a blank line before them, and put a space
between the '#' and your text (it's good style to break up a long-ish method
into blocks of lines like paragraphs). In this case, I don't think the comments
are adding much beyond the fairly-clear code and method names, so I'd omit them
completely.

[patro, 2014/07/16 14:37:37]

Done.

+        with open(self._filesystem.join(json_file_list[0], 'failing_results.json'), "r") as file:
+            input_json_string = file.readline()
+        input_json_string = input_json_string[12:-2]   # Remove preceeding string ADD_RESULTS( and ); at the end

[Dirk Pranke, 2014/07/15 20:50:01]

I would extract lines 71-73 into a routine and reuse it on lines 80-82 as well.

+        self._input_json_data = json.loads(input_json_string)
+        #To add hyperlink to individual results.html
+        self._add_individual_result_links(json_file_list)
+        json_file_list = json_file_list[1:]
+        #Load the remaining stale layout test results Json's to create the dashboard
+        for json_file in json_file_list:
+            with open(self._filesystem.join(json_file, 'failing_results.json'), "r") as file:
+                json_string = file.readline()
+            json_string = json_string[12:-2]   # Remove preceeding string ADD_RESULTS( and ); at the end
+            self._old_failing_results_list.append(json.loads(json_string))
+
+            with open(self._filesystem.join(json_file, 'full_results.json'), "r") as full_file:
+                json_string_full_result = full_file.readline()
+            self._old_full_results_list.append(json.loads(json_string_full_result))
+        self._copy_dashboard_html()
+
+    #To safely get the value if key doesn't exit then it is a syntax error
+    def _get_value(self, json_object, key):
+        try:
+            value = json_object[key]
+            return value
+        except KeyError, e:
+            print("Syntax error: Could not find the key ", key)
+            exit()

[Dirk Pranke, 2014/07/15 20:50:01]

printing an error and calling exit is actually less useful than just letting the
exception be raised. I would delete this whole routine (see following comment).

[patro, 2014/07/16 14:37:36]

Done.

+
+    #To process the final dict
+    def _process_json_result(self, json_object):

[Dirk Pranke, 2014/07/15 20:50:01]

what are you 'processing' here? can you use a more specific verb? It looks like
you're actually trying to figure out if a given test passed or not? is the
json_object actually a result object? can you give that a more specific name as
well?

[patro, 2014/07/16 14:37:36]

Done.

+        actual = self._get_value(json_object, "actual")

[Dirk Pranke, 2014/07/15 20:50:01]

just use actual = json_object['actual'] here and as follows.

[patro, 2014/07/16 14:37:36]

Done.

+        expected = self._get_value(json_object, "expected")
+        if actual == 'SKIP':
+            return actual
+        if actual == expected:
+            hasStderr = 'false'
+            try:
+                hasStderr = json_object["has_stderr"]
+            except KeyError, e:
+                pass

[Dirk Pranke, 2014/07/15 20:50:00]

You can replace lines 107-110  with hasStderr = json_object.get('has_stderr',
'false')

You can actually replace all of lines 106-113 with:

  return 'HASSTDERR' if json_object.get('has_stderr') == 'true' else 'PASS'

[patro, 2014/07/16 14:37:37]

Done.

+            if hasStderr == 'true':
+                return 'HASSTDERR'
+            return 'PASS'
+        else:
+            return actual
+
+    def _recurse_json_object(self, json_object, key_list):
+        for key in key_list:
+            try:
+                json_object = json_object[key]
+            except KeyError:
+                return 'NOTFOUND'
+        return self._process_json_result(json_object)
+
+    def _process_previous_json_results(self, key_list):
+        row = []
+        length = len(self._old_failing_results_list)
+        for index in range(0, length - 1):
+            result = self._recurse_json_object(self._old_failing_results_list[index]["tests"], key_list)
+            if result == 'NOTFOUND':
+                result = self._recurse_json_object(self._old_full_results_list[index]["tests"], key_list)
+            row.append(result)
+        return row
+
+    def _add_archived_results(self, json_object, result):
+        json_object['archived_results'] = result
+
+    def _process_json_object(self, json_object, keyList):
+        flag = 0
+        for key, subdict in json_object.iteritems():
+            if type(subdict) == dict:
+                self._process_json_object(subdict, keyList + [key])
+            else:
+                flag = 1
+                break
+        if flag == 1:
+            row = [self._process_json_result(json_object)]
+            row += self._process_previous_json_results(keyList)
+            self._add_archived_results(json_object, row)

[Dirk Pranke, 2014/07/15 20:50:01]

just replace line 144 with lines 147-149 and replace the 'break' with a
'return', and you don't need 'flag' (and things are clearer).

[patro, 2014/07/16 14:37:37]

Done.

+
+    def _process_json_data(self):
+        for key in self._input_json_data["tests"]:
+            self._process_json_object(self._input_json_data["tests"][key], [key])
+
+    def generate(self):
+        self._initialize()
+        self._process_json_data()
+        final_json = json.dumps(self._input_json_data)
+        final_json = 'ADD_RESULTS(' + final_json + ');'
+        with open(self._filesystem.join(self._release_directory, 'archived_results.json'), "w") as file:
+            file.write(final_json)