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

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

 # Copyright (c) 2012 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 23 matching lines @@
 return the output a real DRT would return for a given test, assuming that
 test actually passes (except for reftests, which currently cause the
 MockDRT to crash).
 """
 
 import base64
 import logging
 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.
 script_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))))
 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
 
 _log = logging.getLogger(__name__)
 
 
 class MockDRTPort(object):
     port_name = 'mock'
 
     @classmethod
     def determine_full_port_name(cls, host, options, port_name):
         return port_name
 
     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)
 
     def check_build(self, needs_http):
         return True
 
     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
     def _overriding_cmd_line(original_cmd_line, driver_path, python_exe, this_file, port_name):
         def new_cmd_line(pixel_tests, per_test_args):
             cmd_line = original_cmd_line(pixel_tests, per_test_args)
             index = cmd_line.index(driver_path)
             cmd_line[index:index + 1] = [python_exe, this_file, '--platform', port_name]
             return cmd_line
 
@@ skipping 46 matching lines @@
     # module.  First, Chromium and non-Chromium DRTs have a different argument
     # syntax.  Chromium uses --pixel-tests=<path>, and non-Chromium uses
     # --pixel-tests as a boolean flag. Second, we don't want to have to list
     # every command line flag DRT accepts, but optparse complains about
     # unrecognized flags. At some point it might be good to share a common
     # DRT options class between this file and webkit.py and chromium.py
     # just to get better type checking.
     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
     if test_shell:
         for arg in argv:
             if arg.startswith('--pixel-tests'):
                 pixel_tests = True
                 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)
 
 
 class MockDRT(object):
     def __init__(self, options, args, host, stdin, stdout, stderr):
         self._options = options
         self._args = args
         self._host = host
         self._stdout = stdout
         self._stdin = stdin
@@ skipping 12 matching lines @@
                 return 0
             driver_input = self.input_from_line(line)
             dirname, basename = self._port.split_test(driver_input.test_name)
             is_reftest = (self._port.reference_files(driver_input.test_name) or
                           self._port.is_reference_html_file(self._port._filesystem, dirname, basename))
             output = self.output_for_test(driver_input, is_reftest)
             self.write_test_output(driver_input, output, is_reftest)
 
     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:
             test_name = self._port.relative_test_filename(uri)
 
         return DriverInput(test_name, 0, checksum, self._options.pixel_tests)
 
     def output_for_test(self, test_input, is_reftest):
         port = self._port
         actual_text = port.expected_text(test_input.test_name)
         actual_audio = port.expected_audio(test_input.test_name)
         actual_image = None
         actual_checksum = None
         if is_reftest:
             # Make up some output for reftests.
             actual_text = 'reference text\n'
             actual_checksum = 'mock-checksum'
             actual_image = 'blank'
             if test_input.test_name.endswith('-mismatch.html'):
                 actual_text = 'not reference text\n'
                 actual_checksum = 'not-mock-checksum'
                 actual_image = 'not blank'
         elif self._options.pixel_tests and test_input.image_hash:
             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):
         if output.audio:
             self._stdout.write('Content-Type: audio/wav\n')
             self._stdout.write('Content-Transfer-Encoding: base64\n')
             self._stdout.write(base64.b64encode(output.audio))
         else:
             self._stdout.write('Content-Type: text/plain\n')
             # FIXME: Note that we don't ensure there is a trailing newline!
             # This mirrors actual (Mac) DRT behavior but is a bug.
             if output.text:
                 self._stdout.write(output.text)
 
         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)
             if output.image_hash != test_input.image_hash:
                 self._stdout.write('Content-Type: image/png\n')
                 self._stdout.write('Content-Length: %s\n' % len(output.image))
                 self._stdout.write(output.image)
         self._stdout.write('#EOF\n')
         self._stdout.flush()
         self._stderr.write('#EOF\n')
@@ skipping 34 matching lines @@
         if output.text and not output.text.endswith('\n'):
             self._stdout.write('\n')
         self._stdout.write('#EOF\n')
         self._stdout.flush()
 
 
 if __name__ == '__main__':
     # Note that the Mock in MockDRT refers to the fact that it is emulating a
     # real DRT, and as such, it needs access to a real SystemHost, not a MockSystemHost.
     sys.exit(main(sys.argv[1:], SystemHost(), sys.stdin, sys.stdout, sys.stderr))