[webkit-reviews] review denied: [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. : [Attachment 92893] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 10 10:29:44 PDT 2011
Ojan Vafai <ojan at chromium.org> has denied Alice Boxhall
<aboxhall at chromium.org>'s request for review:
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.
https://bugs.webkit.org/show_bug.cgi?id=60521
Attachment 92893: Patch
https://bugs.webkit.org/attachment.cgi?id=92893&action=review
------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.
More information about the webkit-reviews
mailing list