Bug 60521

Summary: Modify jsonresults_unittest.py to use a dict format for its test data, and modify jsonresults.py to flatten hierarchical directory structures in input JSON.
Product: WebKit Reporter: Alice Boxhall <aboxhall>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alice Boxhall 2011-05-09 17:47:18 PDT
Modify jsonresults_unittest.py to use a dict format for its test data, and modify jsonresults.py to flatten hierarchical directory structures in input JSON.
Comment 1 Alice Boxhall 2011-05-09 17:50:17 PDT
Created attachment 92893 [details]
Patch
Comment 2 Alice Boxhall 2011-05-09 18:53:41 PDT
Feel free to tell me to pull it out into two separate patches.
Comment 3 Ojan Vafai 2011-05-10 10:29:44 PDT
Comment on attachment 92893 [details]
Patch

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

Mostly style nits. R- for the regexp check on line 252.

> Tools/TestResultServer/model/jsonresults.py:220
> +        incremental_json = cls._flatten_json_tests(incremental_json)

Eventually, when both aggregated and incremental json are hierarchical, we probably don't want to flatten it just to unflatten it later. Can you add a FIXME to that effect?

> Tools/TestResultServer/model/jsonresults.py:246
> +        for (name, test) in json.iteritems():

We typically don't use parens when unpacking in other places in webkit's python code.

> Tools/TestResultServer/model/jsonresults.py:247
> +            if prefix != None:

"if prefix:" is sufficient here, no?

> Tools/TestResultServer/model/jsonresults.py:248
> +                fullname = "/".join([prefix, name])

Using join is less readable to me than just concatenating:
fullname = prefix + "/" + name

> Tools/TestResultServer/model/jsonresults.py:252
> +            if re.match(".+\.html", name):

This check is not sufficiently general, e.g. some test names end in "svg". I think it would be better to avoid checking the test name and instead check if test has a "results" key.

> Tools/TestResultServer/model/jsonresults_unittest.py:88
> +        (builds, tests) = (test_data["builds"], test_data["tests"])

No need to do this in one line.

> Tools/TestResultServer/model/jsonresults_unittest.py:122
> +        if re.match(".+\.html", name):

ditto above. we should probably add a FIXME to have some tests for xhtml and svg tests.

> Tools/TestResultServer/model/jsonresults_unittest.py:123
> +            t = JSON_RESULTS_TESTS_TEMPLATE.replace("[TESTDATA_TEST_NAME]", name)

I know the old code did this, but webkit frowns on non-descriptive variable names. s/t/tests_dict/ ?

> Tools/TestResultServer/model/jsonresults_unittest.py:127
> +        else:

no need for the else since you early return from the if statement

> Tools/TestResultServer/model/jsonresults_unittest.py:130
> +            for (_name, _test) in sorted(test.iteritems()):

We use underscores to denote private methods/variables. This isn't needed for local variables since there is no concept of private local variables.

> Tools/TestResultServer/model/jsonresults_unittest.py:185
> +                            "results": "[200,\"F\"]",
> +                            "times": "[200,0]"}}},

this indentation is off by one space. here and a few places below. later on it's correct.
Comment 4 Alice Boxhall 2011-05-10 10:46:50 PDT
Comment on attachment 92893 [details]
Patch

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

>> Tools/TestResultServer/model/jsonresults.py:220
>> +        incremental_json = cls._flatten_json_tests(incremental_json)
> 
> Eventually, when both aggregated and incremental json are hierarchical, we probably don't want to flatten it just to unflatten it later. Can you add a FIXME to that effect?

Done.

>> Tools/TestResultServer/model/jsonresults.py:246
>> +        for (name, test) in json.iteritems():
> 
> We typically don't use parens when unpacking in other places in webkit's python code.

Ok.

>> Tools/TestResultServer/model/jsonresults.py:247
>> +            if prefix != None:
> 
> "if prefix:" is sufficient here, no?

My python-fu is weak. You are correct.

>> Tools/TestResultServer/model/jsonresults.py:248
>> +                fullname = "/".join([prefix, name])
> 
> Using join is less readable to me than just concatenating:
> fullname = prefix + "/" + name

Done.

>> Tools/TestResultServer/model/jsonresults.py:252
>> +            if re.match(".+\.html", name):
> 
> This check is not sufficiently general, e.g. some test names end in "svg". I think it would be better to avoid checking the test name and instead check if test has a "results" key.

Done.

>> Tools/TestResultServer/model/jsonresults_unittest.py:88
>> +        (builds, tests) = (test_data["builds"], test_data["tests"])
> 
> No need to do this in one line.

Done.

>> Tools/TestResultServer/model/jsonresults_unittest.py:122
>> +        if re.match(".+\.html", name):
> 
> ditto above. we should probably add a FIXME to have some tests for xhtml and svg tests.

Done.

>> Tools/TestResultServer/model/jsonresults_unittest.py:123
>> +            t = JSON_RESULTS_TESTS_TEMPLATE.replace("[TESTDATA_TEST_NAME]", name)
> 
> I know the old code did this, but webkit frowns on non-descriptive variable names. s/t/tests_dict/ ?

Done.

>> Tools/TestResultServer/model/jsonresults_unittest.py:127
>> +        else:
> 
> no need for the else since you early return from the if statement

Done.

>> Tools/TestResultServer/model/jsonresults_unittest.py:130
>> +            for (_name, _test) in sorted(test.iteritems()):
> 
> We use underscores to denote private methods/variables. This isn't needed for local variables since there is no concept of private local variables.

I was trying to come up with names that wouldn't clobber the existing name/test variables, although I agree this was not the way to do it. Changed to child_{name,test}.

>> Tools/TestResultServer/model/jsonresults_unittest.py:185
>> +                            "times": "[200,0]"}}},
> 
> this indentation is off by one space. here and a few places below. later on it's correct.

Damn, I really thought I caught all of those. Fixed.
Comment 5 Alice Boxhall 2011-05-10 10:49:45 PDT
Created attachment 92979 [details]
Patch
Comment 6 Ojan Vafai 2011-05-10 11:04:53 PDT
Comment on attachment 92979 [details]
Patch

Please test this out with real data on test-results-test.appspot.com before committing and pushing to test-results.appspot.com. You can just create dummy data and upload it.
Comment 7 Alice Boxhall 2011-05-11 10:21:54 PDT
Created attachment 93142 [details]
Patch
Comment 8 Ojan Vafai 2011-05-11 10:26:34 PDT
Comment on attachment 93142 [details]
Patch

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

> Tools/TestResultServer/model/jsonresults.py:48
> +JSON_RESULTS_PARTIALLY_SUPPORTED_VERSION = 4

Don't love this name. How about JSON_RESULTS_HIERARCHICAL_VERSION?
Comment 9 Alice Boxhall 2011-05-11 10:31:15 PDT
Created attachment 93145 [details]
Patch
Comment 10 Alice Boxhall 2011-05-11 10:32:15 PDT
Comment on attachment 93142 [details]
Patch

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

>> Tools/TestResultServer/model/jsonresults.py:48
>> +JSON_RESULTS_PARTIALLY_SUPPORTED_VERSION = 4
> 
> Don't love this name. How about JSON_RESULTS_HIERARCHICAL_VERSION?

Done.
Comment 11 WebKit Commit Bot 2011-05-11 12:09:20 PDT
The commit-queue encountered the following flaky tests while processing attachment 93145 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2011-05-11 12:12:37 PDT
Comment on attachment 93145 [details]
Patch

Clearing flags on attachment: 93145

Committed r86252: <http://trac.webkit.org/changeset/86252>
Comment 13 WebKit Commit Bot 2011-05-11 12:12:42 PDT
All reviewed patches have been landed.  Closing bug.