[webkit-reviews] review granted: [Bug 53004] Introduces DriverInput and DriverOutput classes and single_test_runner module. : [Attachment 79916] single_test_runner

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 53004: Introduces DriverInput and DriverOutput classes and
single_test_runner module.
https://bugs.webkit.org/show_bug.cgi?id=53004

Attachment 79916: single_test_runner
https://bugs.webkit.org/attachment.cgi?id=79916&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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 ''?