[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


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #79916|review?                     |review+
               Flag|                            |




--- Comment #7 from Tony Chang <tony at chromium.org>  2011-02-02 15:56:02 PST ---
(From update of attachment 79916)
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.

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

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

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

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

> 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 ''?

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