Bug 105391 - PerfTest.parse_output does too much
Summary: PerfTest.parse_output does too much
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-18 22:56 PST by Ryosuke Niwa
Modified: 2012-12-19 13:32 PST (History)
5 users (show)

See Also:

Attachments
Cleanup (9.33 KB, patch)
2012-12-18 23:09 PST, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Added the missing change log (11.50 KB, patch)
2012-12-18 23:09 PST, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Fixed a bug (13.24 KB, patch)
2012-12-19 10:50 PST, Ryosuke Niwa 		tony: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-12-18 22:56:37 PST 
Right now, PerfTest.parse_output parses output.text, filter lines to be ignored, and outputs results. This method should focus on parsing the output instead.
Comment 1 Ryosuke Niwa 2012-12-18 23:09:08 PST 
Created attachment 180096 [details]
Cleanup
Comment 2 Ryosuke Niwa 2012-12-18 23:09:48 PST 
Created attachment 180098 [details]
Added the missing change log
Comment 3 Tony Chang 2012-12-19 10:06:30 PST 
Comment on attachment 180098 [details]
Added the missing change log

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

r- for what appears to be a typo.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:175
> +            if description_match:
> +                description = description.group('description')

Is this correct?  Shouldn't it be description_match on the right side?  Can we add a test case with a description?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:209
> +        return results, description

Nit: Please make a class and return the class.  It makes the calling code much easier to understand rather than a tuple with unnamed values and makes it harder to accidentally return the wrong type.
Comment 4 Ryosuke Niwa 2012-12-19 10:17:06 PST 
(In reply to comment #3)
> (From update of attachment 180098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180098&action=review
> 
> r- for what appears to be a typo.
> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:175
> > +            if description_match:
> > +                description = description.group('description')
> 
> Is this correct?  Shouldn't it be description_match on the right side?  Can we add a test case with a description?

That's odd. Yeah, let's add a test case.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:209
> > +        return results, description
> 
> Nit: Please make a class and return the class.  It makes the calling code much easier to understand rather than a tuple with unnamed values and makes it harder to accidentally return the wrong type.

I don't think I want to contribute to the proliferations of useless classes. I'll just store it on PerfTest object itself since we'll have exactly one test description per PerfTest.
Comment 5 Ryosuke Niwa 2012-12-19 10:50:37 PST 
Created attachment 180195 [details]
Fixed a bug
Comment 6 Ryosuke Niwa 2012-12-19 11:19:41 PST 
Thanks for the review!
Comment 7 Ryosuke Niwa 2012-12-19 13:32:07 PST 
Committed r138192: <http://trac.webkit.org/changeset/138192>