Bug 59736 – Use a hierarchical data structure to store results JSON

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

use a heirarchical data structure to store results json

[Ojan Vafai]

Created attachment 91553 [details]
Patch

[Eric Seidel (no email)]

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?

[Mihai Parparita]

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.

[Ojan Vafai]

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

[Ojan Vafai]

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

[Ojan Vafai]

Created attachment 91571 [details]
Patch

[Ojan Vafai]

Committed r85254: <http://trac.webkit.org/changeset/85254>

[Eric Seidel (no email)]

Committed r85255: <http://trac.webkit.org/changeset/85255>