Bug 105391 – PerfTest.parse_output does too much

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 180096 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 180098 [details]
Added the missing change log

[Tony Chang]

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]

(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]

Created attachment 180195 [details]
Fixed a bug

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

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