[Webkit-unassigned] [Bug 60521] 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.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60521


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92893|review?                     |review-
               Flag|                            |




--- Comment #3 from Ojan Vafai <ojan at chromium.org>  2011-05-10 10:29:44 PST ---
(From update of attachment 92893)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.