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

Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py

Index: Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py
===================================================================
--- Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py	(revision 157479)
+++ Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py	(working copy)
@@ -41,6 +41,7 @@
 import optparse
 import os
 import sys
+import types
 
 # Since we execute this script directly as part of the unit tests, we need to ensure
 # that Tools/Scripts is in sys.path for the next imports to work correctly.
@@ -48,6 +49,7 @@
 if script_dir not in sys.path:
     sys.path.append(script_dir)
 
+from webkitpy.common import read_checksum_from_png
 from webkitpy.common.system.systemhost import SystemHost
 from webkitpy.layout_tests.port.driver import DriverInput, DriverOutput
 from webkitpy.layout_tests.port.factory import PortFactory
@@ -64,6 +66,8 @@
 
     def __init__(self, host, port_name, **kwargs):
         self.__delegate = PortFactory(host).get(port_name.replace('mock-', ''), **kwargs)
+        self.__delegate_driver_class = self.__delegate._driver_class
+        self.__delegate._driver_class = types.MethodType(self._driver_class, self.__delegate)

[Dirk Pranke, 2013/09/13 18:05:45]

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?

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

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.

[Dirk Pranke, 2013/09/13 19:08:29]

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.

 
     def __getattr__(self, name):
         return getattr(self.__delegate, name)
@@ -74,18 +78,18 @@
     def check_sys_deps(self, needs_http):
         return True
 
-    def _driver_class(self):
+    def _driver_class(self, delegate):
         return self._mocked_driver_maker
 
-    @staticmethod
-    def _mocked_driver_maker(port, worker_number, pixel_tests, no_timeout=False):
-        path_to_this_file = port.host.filesystem.abspath(__file__.replace('.pyc', '.py'))
-        driver = port.__delegate._driver_class()(port, worker_number, pixel_tests, no_timeout)
-        driver.cmd_line = port._overriding_cmd_line(driver.cmd_line,
-                                                    port.__delegate._path_to_driver(),
+    #@staticmethod

[Dirk Pranke, 2013/09/13 19:08:29]

Delete the commented-out line :).

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

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.

+    def _mocked_driver_maker(self, port, worker_number, pixel_tests, no_timeout=False):
+        path_to_this_file = self.host.filesystem.abspath(__file__.replace('.pyc', '.py'))
+        driver = self.__delegate_driver_class()(self, worker_number, pixel_tests, no_timeout)
+        driver.cmd_line = self._overriding_cmd_line(driver.cmd_line,
+                                                    self.__delegate._path_to_driver(),
                                                     sys.executable,
                                                     path_to_this_file,
-                                                    port.__delegate.name())
+                                                    self.__delegate.name())
         return driver
 
     @staticmethod
@@ -152,6 +156,11 @@
     platform_index = argv.index('--platform')
     platform = argv[platform_index + 1]
 
+    actual_directory = None
+    if '--actual-directory' in argv:
+        actual_directory_index = argv.index('--actual-directory')
+        actual_directory = argv[actual_directory_index + 1]
+
     pixel_tests = False
     pixel_path = None
     test_shell = '--test-shell' in argv
@@ -162,7 +171,11 @@
                 pixel_path = arg[len('--pixel-tests='):]
     else:
         pixel_tests = '--pixel-tests' in argv
-    options = optparse.Values({'test_shell': test_shell, 'platform': platform, 'pixel_tests': pixel_tests, 'pixel_path': pixel_path})
+    options = optparse.Values({'test_shell': test_shell,
+                               'platform': platform,
+                               'pixel_tests': pixel_tests,
+                               'pixel_path': pixel_path,
+                               'actual_directory': actual_directory})
     return (options, argv)
 
 
@@ -195,12 +208,10 @@
 
     def input_from_line(self, line):
         vals = line.strip().split("'")
-        if len(vals) == 1:
-            uri = vals[0]
-            checksum = None
-        else:
-            uri = vals[0]
-            checksum = vals[1]
+        uri = vals[0]
+        checksum = None
+        if len(vals) >= 3:
+            checksum = vals[2]
         if uri.startswith('http://') or uri.startswith('https://'):
             test_name = self._driver.uri_to_test(uri)
         else:
@@ -227,6 +238,21 @@
             actual_checksum = port.expected_checksum(test_input.test_name)
             actual_image = port.expected_image(test_input.test_name)
 
+        if self._options.actual_directory:

[Dirk Pranke, 2013/09/13 18:05:45]

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]

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).
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:29]

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.

+            actual_path = port._filesystem.join(self._options.actual_directory, test_input.test_name)
+            root, _ = port._filesystem.splitext(actual_path)
+            text_path = root + '-actual.txt'
+            if port._filesystem.exists(text_path):
+                actual_text = port._filesystem.read_binary_file(text_path)
+            audio_path = root + '-actual.wav'
+            if port._filesystem.exists(audio_path):
+                actual_audio = port._filesystem.read_binary_file(audio_path)
+            image_path = root + '-actual.png'
+            if port._filesystem.exists(image_path):
+                actual_image = port._filesystem.read_binary_file(image_path)
+                with port._filesystem.open_binary_file_for_reading(image_path) as filehandle:
+                    actual_checksum = read_checksum_from_png.read_checksum(filehandle)
+
         return DriverOutput(actual_text, actual_image, actual_checksum, actual_audio)
 
     def write_test_output(self, test_input, output, is_reftest):
@@ -243,7 +269,7 @@
 
         self._stdout.write('#EOF\n')
 
-        if self._options.pixel_tests and output.image_hash:
+        if self._options.pixel_tests and output.image_hash or is_reftest:
             self._stdout.write('\n')
             self._stdout.write('ActualHash: %s\n' % output.image_hash)
             self._stdout.write('ExpectedHash: %s\n' % test_input.image_hash)