Bug 53004 – Introduces DriverInput and DriverOutput classes and single_test_runner module.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hayato Ito]

This is a separated patch from https://bugs.webkit.org/show_bug.cgi?id=51091.

This patch does mainly two things:
 - Introduces Input/Output classes used by base.Driver into port/base.py.
 - Move _run_single_test() and _process_output() functions from dump_render_tree_thread.py to a single_test_runner.py as an individual module. 

test_types/* classes are not touched except for renaming variables in this patch. The following patches will move test_type/* classes into a single_test_runner module introduced in this patch.

Please see  https://bugs.webkit.org/show_bug.cgi?id=51091 for more details.

[Hayato Ito]

Created attachment 79916 [details]
single_test_runner

[Eric Seidel (no email)]

Comment on attachment 79916 [details]
single_test_runner

Why would we want to move DriverOutput out of its own file and into a shared file?

This change is still qutie large.  The rename could be done first and easily rubber-stamped.

I guess you're moving single-test-running logic from dump_render_tree_thread onto its own class?  That sounds like a very good idea.

Are you just moving code, or causing functional changes as well?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 79916 [details])
> Why would we want to move DriverOutput out of its own file and into a shared file?
>

Read the comments in the other bug ... the motivation is that TestInput and TestOutput are really pretty closely tied to the Driver class, and so it makes sense to rename them and move them to be next to the other class. I suggested that we actually move all of the Driver* classes out of base.py and into a driver.py (or some such file), but I could understand it if Ito-san didn't want to do that as part of this patch.
 
> This change is still qutie large.  The rename could be done first and easily rubber-stamped.
>

I actually prefer the fact that he's renaming things and moving them in a single patch, because it allows me to understand what's going on and review all the changes at once.
 
> I guess you're moving single-test-running logic from dump_render_tree_thread onto its own class?  That sounds like a very good idea.
>

Yes. Ironically, there's a large amount of overlap between single_test_runner and what will be the Worker class in the multiprocessing-safe patch (bug 52194), but that's what I get for taking so long to land the other one. More merging ahead :)
  
> Are you just moving code, or causing functional changes as well?

This change looks like it's just moving stuff around, and it looks good to me (though I can't R+ it).

[Hayato Ito]

Hi Eric, Dirk, thank you for the reviews.

(In reply to comment #2)
> (From update of attachment 79916 [details])
> Why would we want to move DriverOutput out of its own file and into a shared file?
>

As Dirk mentioned, I'd like to move Driver* classes out of port/base.py as well as Driver class itself in another patch. So in this patch, please forgive me that DriverInput/Out classes are put on port/base.py. Driver and DirverInput/Output classes are tightly related.
 
> This change is still qutie large.  The rename could be done first and easily rubber-stamped.


I could have separated this patch into two patches for easy reviewing, but I couldn't help but move _process_output() method to a separated module when I tried to modify  _process_output() function in dump_render_thread.py so that that uses DriverInput/Output classes :)

> I guess you're moving single-test-running logic from dump_render_tree_thread onto its own class?  That sounds like a very good idea.
> 
> Are you just moving code, or causing functional changes as well?

single_test_runner.py contains only moved code from dump_render_tree.py as of now. There is no functional change. That's just moving stuff around.

It seems that there is no easy way to know 'diff' in bugzilla if we move the code and modify it a little. Sorry for the inconvenience.

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 79916 [details] [details])
> > Why would we want to move DriverOutput out of its own file and into a shared file?
> >
> 
> Read the comments in the other bug ... the motivation is that TestInput and TestOutput are really pretty closely tied to the Driver class, and so it makes sense to rename them and move them to be next to the other class. I suggested that we actually move all of the Driver* classes out of base.py and into a driver.py (or some such file), but I could understand it if Ito-san didn't want to do that as part of this patch.

Thank you for the comment, Dirk. I think you can tell what I am thinking successfully :) 

> 
> This change looks like it's just moving stuff around, and it looks good to me (though I can't R+ it).

There should be no functional change in this patch. Just a first attempt to clean up and keeping things simple. I'll continue to create patches, which are separated from https://bugs.webkit.org/show_bug.cgi?id=51091

I'll separe this patch into two patches if that is needed.
If you are confortable as is, r+ is welcome :)

[Hayato Ito]

Is there any WebKit reviewer to take a look at this patch and set r+? I'll appreciate if you review this.


(In reply to comment #4)
> Hi Eric, Dirk, thank you for the reviews.
> 
> (In reply to comment #2)
> > (From update of attachment 79916 [details] [details])
> > Why would we want to move DriverOutput out of its own file and into a shared file?
> >
> 
> As Dirk mentioned, I'd like to move Driver* classes out of port/base.py as well as Driver class itself in another patch. So in this patch, please forgive me that DriverInput/Out classes are put on port/base.py. Driver and DirverInput/Output classes are tightly related.
> 
> > This change is still qutie large.  The rename could be done first and easily rubber-stamped.
> 
> 
> I could have separated this patch into two patches for easy reviewing, but I couldn't help but move _process_output() method to a separated module when I tried to modify  _process_output() function in dump_render_thread.py so that that uses DriverInput/Output classes :)
> 
> > I guess you're moving single-test-running logic from dump_render_tree_thread onto its own class?  That sounds like a very good idea.
> > 
> > Are you just moving code, or causing functional changes as well?
> 
> single_test_runner.py contains only moved code from dump_render_tree.py as of now. There is no functional change. That's just moving stuff around.
> 
> It seems that there is no easy way to know 'diff' in bugzilla if we move the code and modify it a little. Sorry for the inconvenience.
> 
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 79916 [details] [details] [details])
> > > Why would we want to move DriverOutput out of its own file and into a shared file?
> > >
> > 
> > Read the comments in the other bug ... the motivation is that TestInput and TestOutput are really pretty closely tied to the Driver class, and so it makes sense to rename them and move them to be next to the other class. I suggested that we actually move all of the Driver* classes out of base.py and into a driver.py (or some such file), but I could understand it if Ito-san didn't want to do that as part of this patch.
> 
> Thank you for the comment, Dirk. I think you can tell what I am thinking successfully :) 
> 
> > 
> > This change looks like it's just moving stuff around, and it looks good to me (though I can't R+ it).
> 
> There should be no functional change in this patch. Just a first attempt to clean up and keeping things simple. I'll continue to create patches, which are separated from https://bugs.webkit.org/show_bug.cgi?id=51091
> 
> I'll separe this patch into two patches if that is needed.
> If you are confortable as is, r+ is welcome :)

[Dirk Pranke]

Tony, Mihai - one of you want to take a look at this? As I noted before, this patch LGTM and I'd like to get it landed ASAP because it collides like heck with some of multiprocessing cleanup ;)

[Tony Chang]

Comment on attachment 79916 [details]
single_test_runner

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

[Hayato Ito]

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.

[Hayato Ito]

Committed r77603: <http://trac.webkit.org/changeset/77603>