[Webkit-unassigned] [Bug 53004] Introduces DriverInput and DriverOutput classes and single_test_runner module.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53004





--- Comment #8 from Hayato Ito <hayato at chromium.org>  2011-02-03 21:27:02 PST ---
Thank you for the review.
I'll land the patch after merging it with ToT, reflecting your comments, because r+ is given.

(In reply to comment #7)
> (From update of attachment 79916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79916&action=review
> 
> I agree with eric that the renaming (from *_test_output to *_driver_output) could have happened first, which would have made it easier to review the refactoring better.

Sorry for the inconvenience. Please forgive me about this patch. I'll try to make patch more smaller and separate into logical groups from next.

> 
> Otherwise, just some small style nits.
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:1
> > +# Copyright (C) 2010 Google Inc. All rights reserved.
> 
> 2011

Done.

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:35
> > +from webkitpy.layout_tests.port.base import DriverInput
> > +from webkitpy.layout_tests.port.base import DriverOutput
> 
> This could simply be:
>   from webkitpy.layout_tests.port.base import DriverInput, DriverOutput
> although I prefer:
>   from webkitpy.layout_tests.port import base
> and using base.DriverInput and base.DriverOutput in the code.  Either way is better than 2 lines.

Done.

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:38
> > +import test_failures
> > +from test_results import TestResult
> 
> Please include the full path to these files.  E.g.:
> from webkitpy.layout_tests.layout_package import test_failures.

Done.

> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:51
> > +class ExpectedDriverOutput:
> > +    """Groups information about an expected driver output."""
> 
> Should this class be near DriverInput/DriverOutput?  Alternately, do we need this class at all? Seems like it's the same as DriverOutput (or we could inherit from DriverOutput).

Although I don't have a strong opinion whether DriverOutput and ExpectedDriverOutput should be merged into one class or not, I'd like to be conserative to introduce tight relationship between these two classes.
ExpectedDriverOutput is only used in single_test_runner.py and Driver class doesn't have to know it.

I might change my mind, but for now I'd like to separate these classes.


> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:834
> > +    def __init__(self, text, image, image_hash,
> > +                 crash=None, test_time=None, timeout=None, error=None):
> 
> Should crash and timeout default to False?  Should error default to ''?

I just moved the original TestOutput code to DiverOutput without thinking these default values.

So I've taken a look at usage of TestOutput class and found that every client specifies all parameters so these default values don't matter.

Anyway, I am happy with your suggestions. I've updated default values of these parameters as you suggested and also updated client code (layout_tests/port/dryrun.py) so they give parameter values in the same manner.

These changes should not afect the behavior.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.