Bug 60521

Created attachment 92893 [details] Patch

Feel free to tell me to pull it out into two separate patches.

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

Created attachment 92979 [details] Patch

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.

Created attachment 93142 [details] Patch

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?

Created attachment 93145 [details] Patch

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.

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 on attachment 93145 [details] Patch Clearing flags on attachment: 93145 Committed r86252: <http://trac.webkit.org/changeset/86252>

All reviewed patches have been landed. Closing bug.