Make mock_drt functional and add optional secondary 'actual' directory.

Make mock_drt functional and add optional secondary 'actual' directory.

Currently, mock_drt fails because its 'mock' of _driver_class is inadequate.
Adding an 'actual' directory allows for layout-test-results.zip from trybots
to be used to create local reports.

[bungeman-skia, 2013-09-13 16:42:39]

This has been very useful with testing changes to Skia in Chromium. For an
(admittedly transient) example see
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/9740
. From this we can download the zip and extract it, but the zip contains only
the actual results so it is difficult compare the changes. If ew then run the
following (with this patch applied)

~/chromium/src$ webkit/tools/layout_tests/run_webkit_tests.sh
--no-retry-failures --fully-parallel --no-new-test-results --platform mock-linux
--additional-env-var=PATH=$PATH --ignore-tests=virtual
--additional-drt-flag=--actual-directory
--additional-drt-flag=$HOME/Downloads/layout-test-results/
--additional-drt-flag=--pixel-tests

we end up with an actual results page.

[Dirk Pranke, 2013-09-13 18:05:44]

First, big thumbs-up to making mock_drt work again. I'd been meaning to do that
for a while but hadn't gotten around to it.

Second:
On 2013/09/13 16:42:39, bungeman1 wrote:
> This has been very useful with testing changes to Skia in Chromium. For an
> (admittedly transient) example see
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/9740
> . From this we can download the zip and extract it, but the zip contains only
> the actual results so it is difficult compare the changes. If ew then run the
> following (with this patch applied)

(1) Do you know that you can now view the full results directly off the web (w/o
downloading the zip ?) 

https://storage.googleapis.com/chromium-layout-test-archives/linux_layout/974...

This is new as of last-week-ish, so I wouldn't be surprised if you didn't know
that

(2) It sounds like we should just include the -expected files in the zip. I
don't think we ever had a great reason not to, except for being kinda paranoid
about the size of the zip file, but we fail a lot fewer tests than we used to
...

That said, the --actual-directory flags seems like it could be useful
regardless.

A few comments on the changes itself ...

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
File Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py (right):

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:70:
self.__delegate._driver_class = types.MethodType(self._driver_class,
self.__delegate)
I think I sort-of understand what this change is doing, but I don't understand
how it's actually different from the previous code or why it's needed?

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:241: if
self._options.actual_directory:
What happens if the -actual.* files don't exist ? Is using the values above the
right thing to do?

Would it be better for this block to precede at least the block on lines 237-239
so that we don't read things twice?

[bungeman-skia, 2013-09-13 18:46:16]

I was not aware of the web results. Very, very nice (and mostly covers all of
the use cases I had for this). I think I hadn't seen them simply because the
'archive_webkit_tests_results' 'layout_test_results' link has been broken for so
long I just don't look at it (I've been looking at stdio to get the gs path).

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
File Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py (right):

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:70:
self.__delegate._driver_class = types.MethodType(self._driver_class,
self.__delegate)
On 2013/09/13 18:05:45, Dirk Pranke wrote:
> I think I sort-of understand what this change is doing, but I don't understand
> how it's actually different from the previous code or why it's needed?

This is basically why this wasn't working. What was happening is that external
code was calling driver_cmd_line on an instance of this object, which would go
through __getattr__ and be handled by base.py. However, once this call is made,
'self' there is the delegate. Then base.py calls self._driver_class but this
calls the delegate's method since 'self' is actually the delegate at that point.
This is the issue with just using __getattr__; you get to override if called
directly, but once you've gone through a __getattr__ 'self' is the delegate and
you're no longer 'overriding'.

This change fixes that by replacing the delegate instance's _driver_class
method, saving the original method so that we can call it later. It would
actually be nice to make this more generically 'parasitic' where this object
simply steals all of the delegate's attrs.

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:241: if
self._options.actual_directory:
On 2013/09/13 18:05:45, Dirk Pranke wrote:
> What happens if the -actual.* files don't exist ? Is using the values above
the
> right thing to do?

The idea was that if the -actual.* files don't exist then there is 'no change'
so the test should pass (which is the intent). The idea is that with
layout-test-results.zip any test without 'actual' results is considered to be
passing (on the server the test did pass).

> 
> Would it be better for this block to precede at least the block on lines
237-239
> so that we don't read things twice?

So the idea was that if there is an actual result available, use it, otherwise
use the expected result (or, in the case of reftests, a made up result). The
same comment could be made for actual_text with regard to is_reftest (though the
text is expected to be smaller). The current pattern of this method (which I
extended) is to go from most generic to most specific, overwriting as needed.
This would need to be inverted, from most specific to most generic, checking to
ensure we never overwrite, and possible exiting as soon as everything is filled
out (though this is unlikely to happen due to actual_audio). I'm not sure how to
do so cleanly.

Also, this is being done on the sharded part, and in general there aren't that
may 'actuals'.

[Dirk Pranke, 2013-09-13 19:08:28]

lgtm w/ the commented out line deleted.

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
File Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py (right):

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:70:
self.__delegate._driver_class = types.MethodType(self._driver_class,
self.__delegate)
Oh, that makes sense. I think at one point (before it broke) mock_drt was
overridding create_driver so it didn't have this issue, but that didn't work w/
android anyway.

I think you want to avoid being as parasitic as possible, and mock out as few
things as possible.

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:84: #@staticmethod
Delete the commented-out line :).

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:241: if
self._options.actual_directory:
On 2013/09/13 18:46:16, bungeman1 wrote:
> On 2013/09/13 18:05:45, Dirk Pranke wrote:
> > What happens if the -actual.* files don't exist ? Is using the values above
> the
> > right thing to do?
> 
> The idea was that if the -actual.* files don't exist then there is 'no change'
> so the test should pass (which is the intent). The idea is that with
> layout-test-results.zip any test without 'actual' results is considered to be
> passing (on the server the test did pass).
> 
> > 
> > Would it be better for this block to precede at least the block on lines
> 237-239
> > so that we don't read things twice?
> 
> So the idea was that if there is an actual result available, use it, otherwise
> use the expected result (or, in the case of reftests, a made up result). The
> same comment could be made for actual_text with regard to is_reftest (though
the
> text is expected to be smaller). The current pattern of this method (which I
> extended) is to go from most generic to most specific, overwriting as needed.
> This would need to be inverted, from most specific to most generic, checking
to
> ensure we never overwrite, and possible exiting as soon as everything is
filled
> out (though this is unlikely to happen due to actual_audio). I'm not sure how
to
> do so cleanly.
> 
> Also, this is being done on the sharded part, and in general there aren't that
> may 'actuals'.
> 

Makes sense. The only concern I have about the unnecessary reads is that I often
used mock drt as a proxy for "the fastest possible implementation of a driver"
(which still does at least some i/o) to try and get upper bounds on how fast we
could execute tests w/ run-webkit-tests, so I don't want to do any unnecessary
work, but this seems like it'll be fine.

[bungeman-skia, 2013-09-13 20:22:13]

So I ran this change through the bots, and it appears that the test framework
has been trying to call the mock, but it was always doing things 'for real'. Now
that we're actually calling into the mock driver the output is different. I'm
not sure what the correct output should even be. Should we just update the
expectations? Has the contract changed since this last worked and we should
change the output of the mock while fixing this?

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
File Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py (right):

https://codereview.chromium.org/23889025/diff/1/Tools/Scripts/webkitpy/layout...
Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:84: #@staticmethod
On 2013/09/13 19:08:29, Dirk Pranke wrote:
> Delete the commented-out line :).

Done. Yep, figured out self could come back around in order to know the original
'_driver_class' (here __delegate_driver_class). Note that I think this could be
made static again if we just moved the delegate's _driver_class method to some
other name (like __original_driver_class) so that we could call that instead.
This seems a bit more clear to me, however.

[Dirk Pranke, 2013-09-14 00:06:17]

On 2013/09/13 20:22:13, bungeman1 wrote:
> So I ran this change through the bots, and it appears that the test framework
> has been trying to call the mock, but it was always doing things 'for real'. 

I took a look, and unfortunately things aren't quite that easy. It looks like
you're revealing several different bugs and/or places where the code hasn't been
updated in a long time.

I think maybe the best thing to do is to just pass this CL to me and I'll finish
updating it. That okay to you?

[bungeman-skia, 2013-09-14 01:00:21]

On 2013/09/14 00:06:17, Dirk Pranke wrote:
> On 2013/09/13 20:22:13, bungeman1 wrote:
> > So I ran this change through the bots, and it appears that the test
framework
> > has been trying to call the mock, but it was always doing things 'for real'.

> 
> I took a look, and unfortunately things aren't quite that easy. It looks like
> you're revealing several different bugs and/or places where the code hasn't
been
> updated in a long time.
> 
> I think maybe the best thing to do is to just pass this CL to me and I'll
finish
> updating it. That okay to you?

I have no problem with that, you can have this issue and all the associated fun!

[bungeman-chromium, 2013-09-20 22:22:01]

Closing in favor of https://codereview.chromium.org/23503087/ .