[webkit-reviews] review granted: [Bug 52267] Change results.json format to the one used by unexpected_results.json : [Attachment 83778] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 52267: Change results.json format to the one used by
unexpected_results.json
https://bugs.webkit.org/show_bug.cgi?id=52267

Attachment 83778: Patch
https://bugs.webkit.org/attachment.cgi?id=83778&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83778&action=review

Looks fine to me, although Mihai is more familiar with this code so maybe he
wants to take a look too.

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:17
4
> +    def generator_full_results_file(self):

Nit: Maybe generate_full_results_file to match generate_json_output?

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:17
7
> +	   # For now we only include the times as this is only used for
treemaps and
> +	   # expected/actual don't make test for gtests.

Nit: don't make sense for gtests?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:98
> +    test_timings_map = dict((test_tuple.filename, test_tuple.test_run_time)
for test_tuple in test_timings)

Nit: test_tuple -> test_result?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:115
>      for k, v in TestExpectationsFile.EXPECTATIONS.iteritems():
>	   keywords[v] = k.upper()
>  
> +    for k, v in TestExpectationsFile.MODIFIERS.iteritems():
> +	   keywords[v] = k.upper()

Nit: k -> modifier? v-> code_string?  Even key, value would be better.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:151
> +	       time_seconds = test_timings_map[filename]
> +	       tests[test]['time'] = int(1000 * time_seconds)

Can we name the dict key time_ms to make the units clear?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:815
> +    def _dump_summarized_result(self, filename, results):

Nit: docstring (mainly, it's not clear what results is).  Also, should there be
a unittest for this?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:820
> +	   # Compact the results since we'll be uploading this to the
test-results server.
> +	   # This shrinks the file size by ~20%.
> +	   # actual --> a

In a future patch, maybe we can just zip the file.  I bet you get a > 20%
savings.