Bug 59736 - Use a hierarchical data structure to store results JSON
Summary: Use a hierarchical data structure to store results JSON
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-28 14:07 PDT by Ojan Vafai
Modified: 2011-04-28 16:14 PDT (History)
4 users (show)

See Also:


Attachments
Patch (36.27 KB, patch)
2011-04-28 14:13 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (34.15 KB, patch)
2011-04-28 15:48 PDT, Ojan Vafai
mihaip: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-04-28 14:07:34 PDT
use a heirarchical data structure to store results json
Comment 1 Ojan Vafai 2011-04-28 14:13:58 PDT
Created attachment 91553 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-28 14:31:53 PDT
Comment on attachment 91553 [details]
Patch

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

Are you going to bump the version number?

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:58
>      "skipped": 450,

Do we need to update the version number for these results?

> Tools/Scripts/webkitpy/layout_tests/layout_package/printing.py:448
> +    def _print_unexpected_results_subtree(self, tree, passes, flaky, regressions, prefix=""):

Seems we should share code with LayoutTestResults/JSONTestResult, no?
Comment 3 Mihai Parparita 2011-04-28 14:40:51 PDT
Comment on attachment 91553 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +        use a heirarchical data structure to store results json

Should probably fix the typo everywhere :)

> LayoutTests/fast/harness/resources/results-test.js:116
> +    var subtree = {}

Seems cleaner to say var subtree = results.tests['foo'] = {};

(below too)

> LayoutTests/fast/harness/results.html:311
> +        globalState().testsWithStderr.push(test);

Seems like the initial part of tableRow is about initializing the global state from the tests, and doesn't actually have anything to do with table rows. You may want to move that to a separate function (can be in a different patch).

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:400
> +    def _gather_current_baselines(self, tree, test_config, new_tests_subtree, prefix=""):

Use single quotes, to be consistent with the rest of the code.
Comment 4 Ojan Vafai 2011-04-28 15:17:56 PDT
(In reply to comment #2)
> (From update of attachment 91553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91553&action=review
> 
> Are you going to bump the version number?

Sure. In practice, this is just here so we have the option of writing code that understands both version as we transition. I'm changing all the places where we read in the JSON, so it doesn't really matter.

> > Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:58
> >      "skipped": 450,
> 
> Do we need to update the version number for these results?

May as well.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/printing.py:448
> > +    def _print_unexpected_results_subtree(self, tree, passes, flaky, regressions, prefix=""):
> 
> Seems we should share code with LayoutTestResults/JSONTestResult, no?

Yeah. I couldn't think of a great place to put it and it's only 5 lines of code. But it is in 4 places! I'm moving it to being a free function in resultsjsonparser.
Comment 5 Ojan Vafai 2011-04-28 15:42:38 PDT
(In reply to comment #3)
> (From update of attachment 91553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91553&action=review
> 
> > LayoutTests/ChangeLog:5
> > +        use a heirarchical data structure to store results json
> 
> Should probably fix the typo everywhere :)

Sigh. English is hard. :(

> > LayoutTests/fast/harness/resources/results-test.js:116
> > +    var subtree = {}
> 
> Seems cleaner to say var subtree = results.tests['foo'] = {};
> 
> (below too)

Will do.

> > LayoutTests/fast/harness/results.html:311
> > +        globalState().testsWithStderr.push(test);
> 
> Seems like the initial part of tableRow is about initializing the global state from the tests, and doesn't actually have anything to do with table rows. You may want to move that to a separate function (can be in a different patch).

May as well do it here.
Comment 6 Ojan Vafai 2011-04-28 15:48:35 PDT
Created attachment 91571 [details]
Patch
Comment 7 Ojan Vafai 2011-04-28 16:12:38 PDT
Committed r85254: <http://trac.webkit.org/changeset/85254>
Comment 8 Eric Seidel (no email) 2011-04-28 16:14:22 PDT
Committed r85255: <http://trac.webkit.org/changeset/85255>