Bug 105391

Summary:

PerfTest.parse_output does too much

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Tools / Tests

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dpranke, eric, morrita, zoltan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Cleanup	none
Added the missing change log	none
Fixed a bug	tony: review+

Ryosuke Niwa

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

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 attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2012-12-18 23:09:08 PST

Created attachment 180096 [details] Cleanup

Ryosuke Niwa

Comment 2 2012-12-18 23:09:48 PST

Created attachment 180098 [details] Added the missing change log

Tony Chang

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

Ryosuke Niwa

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

Ryosuke Niwa

Comment 5 2012-12-19 10:50:37 PST

Created attachment 180195 [details] Fixed a bug

Ryosuke Niwa

Comment 6 2012-12-19 11:19:41 PST

Thanks for the review!

Ryosuke Niwa

Comment 7 2012-12-19 13:32:07 PST

Committed r138192: <http://trac.webkit.org/changeset/138192>

Note You need to log in before you can comment on or make changes to this bug.