Bug 52267 - Change results.json format to the one used by unexpected_results.json
Summary: Change results.json format to the one used by unexpected_results.json
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 55116 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-01-11 17:55 PST by Mihai Parparita
Modified: 2011-03-01 22:50 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.15 KB, patch)
2011-02-25 00:16 PST, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2011-01-11 17:55:02 PST
Right now results.json uses a JSON-like format that's meant for the flakiness dashboard (it has a custom compression scheme, wraps the JSON in an ADD_RESULT method, etc.).

Since we don't actually need it for the flakiness dashboard anymore (it uses incremental_results.json), we should repurpose results.json to have a format closed to unexpected_results.json (which is a straightforward mapping from test name to expected and actual result). That would then make it easier to programatically parse the results of a NRWT run (the way webkitpy.common.net.layouttestresults.LayoutTestResults parses results.html for old-RWT - we should make that class handle either result type).
Comment 1 Eric Seidel (no email) 2011-01-11 17:56:23 PST
sgtm.
Comment 2 Dirk Pranke 2011-01-11 18:22:43 PST
This sounds reasonable. Who do you imagine parsing the results files? Do the webkit.org bots parse the results files directly?
Comment 3 Mihai Parparita 2011-01-11 18:26:22 PST
Besides the various rebaselining tools (which should let you pick amongst failing tests), there are bunch of webkit-patch commands that use webkitpy.common.net.layouttestresults.LayoutTestResults (failure-reason, find-flaky-tests, etc.). Right now they don't work with bots that use NRWT, but if we output an easily parseable results.json then it should be reasonable to load it into the Python LayoutTestResults representation too.
Comment 4 Ojan Vafai 2011-01-11 18:28:32 PST
Eventually, I'd also like to see results.html pull in results.json and use that to generate that page from JS. That would let us make that page a richer experience.
Comment 5 Dirk Pranke 2011-01-11 18:29:50 PST
I have on my TODO list somewhere the task to unify results.html with the html generated by ORWT, so it seems reasonable to do that and pull in results.json at the same time.
Comment 6 Eric Seidel (no email) 2011-01-11 18:40:14 PST
(In reply to comment #4)
> Eventually, I'd also like to see results.html pull in results.json and use that to generate that page from JS. That would let us make that page a richer experience.

I am *strong* in favor of this.  If for no other reason than it separates model and view.  It would also make it trivial to unify the results.html experience between ORWT and NRWT.
Comment 7 Mihai Parparita 2011-01-11 18:43:34 PST
(In reply to comment #6)
> I am *strong* in favor of this.  If for no other reason than it separates model and view.  It would also make it trivial to unify the results.html experience between ORWT and NRWT.

+1. Is there a bug filed?

Dirk, mind if I work on this bug in the meantime?
Comment 8 Dirk Pranke 2011-01-11 18:59:29 PST
Not at all. Feel free to do the other thing as well ;)
Comment 9 Dirk Pranke 2011-01-11 19:04:32 PST
I think bug 37736 is Maciej's original request to unify the html output. There isn't a bug to have that file pull results.json, obviously.
Comment 10 Ojan Vafai 2011-02-24 23:45:10 PST
*** Bug 55116 has been marked as a duplicate of this bug. ***
Comment 11 Ojan Vafai 2011-02-25 00:16:42 PST
Created attachment 83778 [details]
Patch
Comment 12 Tony Chang 2011-02-25 10:07:04 PST
Comment on attachment 83778 [details]
Patch

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:174
> +    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:177
> +        # 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.
Comment 13 Ojan Vafai 2011-02-27 19:51:36 PST
Comment on attachment 83778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83778&action=review

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:174
>> +    def generator_full_results_file(self):
> 
> Nit: Maybe generate_full_results_file to match generate_json_output?

whoops. just a typo.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:115
>> +        keywords[v] = k.upper()
> 
> Nit: k -> modifier? v-> code_string?  Even key, value would be better.

Sorry. Was just mimicking the code above. Will fix both.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:151
>> +            tests[test]['time'] = int(1000 * time_seconds)
> 
> Can we name the dict key time_ms to make the units clear?

yes.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:820
>> +        # actual --> a
> 
> In a future patch, maybe we can just zip the file.  I bet you get a > 20% savings.

The downside is that we'd have to unzip it on the receiving end since we're ultimately serving up a JS file to the browser.
Comment 14 Ojan Vafai 2011-02-27 19:55:35 PST
Committed r79837: <http://trac.webkit.org/changeset/79837>
Comment 15 WebKit Review Bot 2011-02-27 20:25:12 PST
http://trac.webkit.org/changeset/79837 might have broken SnowLeopard Intel Release (Tests)
Comment 16 Dirk Pranke 2011-03-01 22:50:24 PST
(In reply to comment #13)
> >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:820
> >> +        # actual --> a
> > 
> > In a future patch, maybe we can just zip the file.  I bet you get a > 20% savings.
> 
> The downside is that we'd have to unzip it on the receiving end since we're ultimately serving up a JS file to the browser.

Can we gzip the file and then just respond to the raw script URL with the gzipped content?