Refactor layout test scripts to get the dashboard work for other types

Refactor layout test scripts to get the dashboard work for other types
- refactor layout-test specific part out from json_results_generator.py
- add test_output_xml_to_json to generate JSON files for other test types

BUG=32152
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=36621

[kinuko, 2010-01-13 18:11:11]

Hi Ojan,
I've uploaded a snapshot.  The code may still need to be changed, but can you
take a look at it when you have time.
Thanks,

[ojan, 2010-01-13 19:44:29]

This looks great. Mostly a few nits.

http://codereview.chromium.org/550027/diff/2001/3001
File webkit/tools/layout_tests/layout_package/json_layout_results_generator.py
(right):

http://codereview.chromium.org/550027/diff/2001/3001#newcode17
webkit/tools/layout_tests/layout_package/json_layout_results_generator.py:17:
SKIP_RESULT = "X"
It's not a big deal, but I think we'll want this for other test types as well.
Specifically, we'll want to know the number of DISABLED tests. We can leave that
as a TODO though.

http://codereview.chromium.org/550027/diff/2001/3001#newcode87
webkit/tools/layout_tests/layout_package/json_layout_results_generator.py:87:
def _GetOldResults(self):
We're going to want the same logic for other test types of retrieving the JSON
file from the archive location. The only thing that would change is the path we
grab it from, right? So, that part should be set in the subclass, but the rest
of this should be in the super-class.

http://codereview.chromium.org/550027/diff/2001/3002
File webkit/tools/layout_tests/layout_package/json_results_generator.py (left):

http://codereview.chromium.org/550027/diff/2001/3002#oldcode45
webkit/tools/layout_tests/layout_package/json_results_generator.py:45:
ALL_FIXABLE_COUNT = "allFixableCount"
This variable is really poorly named. Sorry about that. I'll fix this one day.
This is the total number of tests that we want to pass. So, if we're keeping
fixableCount in this file, we should probably keep allFixableCount as well.

I think it does make sense to keep them both since these concepts apply to all
test types and we can use this to create a dashboard to track progress fixing
them.

http://codereview.chromium.org/550027/diff/2001/3002
File webkit/tools/layout_tests/layout_package/json_results_generator.py (right):

http://codereview.chromium.org/550027/diff/2001/3002#newcode78
webkit/tools/layout_tests/layout_package/json_results_generator.py:78: 
Nit: here and below. we should have a consistent number of line breaks between
function definitions. i think the standard is 1.

http://codereview.chromium.org/550027/diff/2001/3002#newcode246
webkit/tools/layout_tests/layout_package/json_results_generator.py:246: time = 0
Nit: Please add some line-breaks here. Perhaps after each if-statement. Just to
make it a bit more readable.

[kinuko, 2010-01-14 07:48:13]

Thanks for reviewing, I've updated the code.

http://codereview.chromium.org/550027/diff/2001/3001
File webkit/tools/layout_tests/layout_package/json_layout_results_generator.py
(right):

http://codereview.chromium.org/550027/diff/2001/3001#newcode17
webkit/tools/layout_tests/layout_package/json_layout_results_generator.py:17:
SKIP_RESULT = "X"
On 2010/01/13 19:44:29, ojan wrote:
> It's not a big deal, but I think we'll want this for other test types as well.
> Specifically, we'll want to know the number of DISABLED tests. We can leave
that
> as a TODO though.

Done.

http://codereview.chromium.org/550027/diff/2001/3001#newcode87
webkit/tools/layout_tests/layout_package/json_layout_results_generator.py:87:
def _GetOldResults(self):
On 2010/01/13 19:44:29, ojan wrote:
> We're going to want the same logic for other test types of retrieving the JSON
> file from the archive location. The only thing that would change is the path
we
> grab it from, right? So, that part should be set in the subclass, but the rest
> of this should be in the super-class.

Done.

http://codereview.chromium.org/550027/diff/2001/3002
File webkit/tools/layout_tests/layout_package/json_results_generator.py (left):

http://codereview.chromium.org/550027/diff/2001/3002#oldcode45
webkit/tools/layout_tests/layout_package/json_results_generator.py:45:
ALL_FIXABLE_COUNT = "allFixableCount"
On 2010/01/13 19:44:29, ojan wrote:
> This variable is really poorly named. Sorry about that. I'll fix this one day.
> This is the total number of tests that we want to pass. So, if we're keeping
> fixableCount in this file, we should probably keep allFixableCount as well.
>
> I think it does make sense to keep them both since these concepts apply to all
> test types and we can use this to create a dashboard to track progress fixing
> them.

Sounds reasonable, restored them and SKIP_RESULT definitions.
I've also added some code that inserts those 'fixable' numbers, but there might
be a subtlety about whether we want to distinguish "fixable skipped" tests and
"skipped and wontfix" ones for other test types.  Currently I assume that we
treat all DISABLED_ tests as skipped (i.e. fixable).

(Also if there's a possibility that we may eventually have more detailed test
results/expectations for other test types I think we should use ResultSummary
for them too.  In that case I can revive my previous patch (Patchset 1).)

http://codereview.chromium.org/550027/diff/2001/3002
File webkit/tools/layout_tests/layout_package/json_results_generator.py (right):

http://codereview.chromium.org/550027/diff/2001/3002#newcode78
webkit/tools/layout_tests/layout_package/json_results_generator.py:78: 
On 2010/01/13 19:44:29, ojan wrote:
> Nit: here and below. we should have a consistent number of line breaks between
> function definitions. i think the standard is 1.

Done.

http://codereview.chromium.org/550027/diff/2001/3002#newcode246
webkit/tools/layout_tests/layout_package/json_results_generator.py:246: time = 0
On 2010/01/13 19:44:29, ojan wrote:
> Nit: Please add some line-breaks here. Perhaps after each if-statement. Just
to
> make it a bit more readable.

Done.

[ojan, 2010-01-15 02:54:47]

LGTM. One nit. Also, JSONResultsGenerator changed a bunch today. Please be very
careful with merging the changes in.

http://codereview.chromium.org/550027/diff/4007/5005
File webkit/tools/layout_tests/run_webkit_tests.py (right):

http://codereview.chromium.org/550027/diff/4007/5005#newcode1591
webkit/tools/layout_tests/run_webkit_tests.py:1591: "results."))
I don't think we need this as an option to run_webkit_tests. This script already
has more options than it needs. Lets just pass BUILDER_BASE_URL in as an
argument to JSONLayoutResultsGenerator.

[kinuko, 2010-01-15 13:23:25]

I've merged the changes.  I also added a new script  that generates a
results.json from a given test output (so that we can call it in the buildbot
slaves).  Could you review it one more time for the script?  Thanks!

http://codereview.chromium.org/550027/diff/4007/5005
File webkit/tools/layout_tests/run_webkit_tests.py (right):

http://codereview.chromium.org/550027/diff/4007/5005#newcode1591
webkit/tools/layout_tests/run_webkit_tests.py:1591: "results."))
On 2010/01/15 02:54:47, ojan wrote:
> I don't think we need this as an option to run_webkit_tests. This script
already
> has more options than it needs. Lets just pass BUILDER_BASE_URL in as an
> argument to JSONLayoutResultsGenerator.

Done.

[ojan, 2010-01-15 19:38:48]

LGTM

http://codereview.chromium.org/550027/diff/4008/4012
File webkit/tools/layout_tests/test_output_xml_to_json.py (right):

http://codereview.chromium.org/550027/diff/4008/4012#newcode69
webkit/tools/layout_tests/test_output_xml_to_json.py:69: continue
Nit: Another I line break after the continue might make this a bit more
readable. It's easier at a quick glance to notice the continue statement that
way.

http://codereview.chromium.org/550027/diff/4008/4012#newcode75
webkit/tools/layout_tests/test_output_xml_to_json.py:75: time_in_sec =
float(testcase.getAttribute('time'))
nit: this should be moved down between lines 77 and 78 since it's not used until
line 78. or just drop the local variable entirely.

http://codereview.chromium.org/550027/diff/4008/4012#newcode99
webkit/tools/layout_tests/test_output_xml_to_json.py:99: if
options.results_directory.startswith("/"):
Do you need the if-check? GetAbsolutePath looks to me like it's a noop if it's
already absolute, but I might be reading it wrong.

[kinuko, 2010-01-19 11:22:09]

Thanks for your review!  I'm going to submit it if I don't hear red flags from
you.

http://codereview.chromium.org/550027/diff/4008/4012
File webkit/tools/layout_tests/test_output_xml_to_json.py (right):

http://codereview.chromium.org/550027/diff/4008/4012#newcode69
webkit/tools/layout_tests/test_output_xml_to_json.py:69: continue
On 2010/01/15 19:38:48, ojan wrote:
> Nit: Another I line break after the continue might make this a bit more
> readable. It's easier at a quick glance to notice the continue statement that
> way.

Done.

http://codereview.chromium.org/550027/diff/4008/4012#newcode75
webkit/tools/layout_tests/test_output_xml_to_json.py:75: time_in_sec =
float(testcase.getAttribute('time'))
On 2010/01/15 19:38:48, ojan wrote:
> nit: this should be moved down between lines 77 and 78 since it's not used
until
> line 78. or just drop the local variable entirely.

Done.

http://codereview.chromium.org/550027/diff/4008/4012#newcode99
webkit/tools/layout_tests/test_output_xml_to_json.py:99: if
options.results_directory.startswith("/"):
On 2010/01/15 19:38:48, ojan wrote:
> Do you need the if-check? GetAbsolutePath looks to me like it's a noop if it's
> already absolute, but I might be reading it wrong.

Well, I guess we don't need it.  The code was originally from
run_webkit_tests.py.  Removed the part.