Changeset 90826 in webkit


Ignore:
Timestamp:
Jul 12, 2011 10:52:54 AM (13 years ago)
Author:
eric@webkit.org
Message:

NRWT should open test results page with Safari trunk, not the system provided one on Mac
​https://bugs.webkit.org/show_bug.cgi?id=64346

Reviewed by Adam Barth.

To fix this I implemented Port.show_results_html_file in Mac, Gtk and Qt ports with
implementations (mostly) matching those found in old-run-webkit-tests.
There are still some minor differences for Qt which Qt hackers may wish to tweak.

I had to add a WebKitPort._port_flag_for_scripts method (similar to flag() in common.config.ports.py)
for the Qt/Gtk ports which always require a flag passed to scripts.

While trying to test this, I found that FactoryTest.test_chromium_gpu_linux
was using a real filesystem (due to assert_platform) which due to
global variables in config.py was causing set-webkit-configuration to have
an affect on unit test results! So I fixed this by making FactoryTest.assert_port
pass a mock file system whenever calling factory.get.

Unfortunately TestPort was depending on always being passed a None filesystem
and asserting filesystem._tests (only true for unit_test_filesystem()).
So I just removed the FactoryTest.test_test and FactoryTest.test_dryrun tests
deciding that they were pretty much useless anyway. If others feel strongly
I'm happy to fix this in a different way.

  • Scripts/webkitpy/common/system/executive.py:
    • default arguments in python are screwy. They use a single shared instance, so it's better to use argument=None and then argument = argument or Default() if you have any chance of mutating (or returning) the default argument.
  • Scripts/webkitpy/layout_tests/port/config.py:
    • This code is wrong. We don't need to use a global variable here (as far as I can tell). I'm not fixing it in this patch, but I've marked it with a FIXME and we can convert to storing the results of the read on the Config object (which should only be created once during normal operation). Unit tests shouldn't be hitting the disk anyway. It's possible Config should move off of Port and onto Tool/Host directly.
  • Scripts/webkitpy/layout_tests/port/factory.py:
  • Scripts/webkitpy/layout_tests/port/factory_unittest.py:
  • Scripts/webkitpy/layout_tests/port/gtk.py:
  • Scripts/webkitpy/layout_tests/port/gtk_unittest.py: Added.
  • Scripts/webkitpy/layout_tests/port/mac.py:
  • Scripts/webkitpy/layout_tests/port/mac_unittest.py:
  • Scripts/webkitpy/layout_tests/port/qt.py:
  • Scripts/webkitpy/layout_tests/port/qt_unittest.py:
  • Scripts/webkitpy/layout_tests/port/webkit.py:
Location:
trunk/Tools
Files:
1 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r90824 r90826  
     12011-07-12  Eric Seidel  <eric@webkit.org>
     2
     3        NRWT should open test results page with Safari trunk, not the system provided one on Mac
     4        https://bugs.webkit.org/show_bug.cgi?id=64346
     5
     6        Reviewed by Adam Barth.
     7
     8        To fix this I implemented Port.show_results_html_file in Mac, Gtk and Qt ports with
     9        implementations (mostly) matching those found in old-run-webkit-tests.
     10        There are still some minor differences for Qt which Qt hackers may wish to tweak.
     11
     12        I had to add a WebKitPort._port_flag_for_scripts method (similar to flag() in common.config.ports.py)
     13        for the Qt/Gtk ports which always require a flag passed to scripts.
     14
     15        While trying to test this, I found that FactoryTest.test_chromium_gpu_linux
     16        was using a real filesystem (due to assert_platform) which due to
     17        global variables in config.py was causing set-webkit-configuration to have
     18        an affect on unit test results!  So I fixed this by making FactoryTest.assert_port
     19        pass a mock file system whenever calling factory.get.
     20
     21        Unfortunately TestPort was depending on always being passed a None filesystem
     22        and asserting filesystem._tests (only true for unit_test_filesystem()).
     23        So I just removed the FactoryTest.test_test and FactoryTest.test_dryrun tests
     24        deciding that they were pretty much useless anyway.  If others feel strongly
     25        I'm happy to fix this in a different way.
     26
     27        * Scripts/webkitpy/common/system/executive.py:
     28         - default arguments in python are screwy.  They use a single shared
     29           instance, so it's better to use argument=None and then argument = argument or Default()
     30           if you have any chance of mutating (or returning) the default argument.
     31        * Scripts/webkitpy/layout_tests/port/config.py:
     32         - This code is wrong.  We don't need to use a global variable here (as far as I can tell).
     33           I'm not fixing it in this patch, but I've marked it with a FIXME and we can convert to
     34           storing the results of the read on the Config object (which should only be created once during normal operation).
     35           Unit tests shouldn't be hitting the disk anyway.  It's possible Config should move off of Port and onto Tool/Host directly.
     36        * Scripts/webkitpy/layout_tests/port/factory.py:
     37        * Scripts/webkitpy/layout_tests/port/factory_unittest.py:
     38        * Scripts/webkitpy/layout_tests/port/gtk.py:
     39        * Scripts/webkitpy/layout_tests/port/gtk_unittest.py: Added.
     40        * Scripts/webkitpy/layout_tests/port/mac.py:
     41        * Scripts/webkitpy/layout_tests/port/mac_unittest.py:
     42        * Scripts/webkitpy/layout_tests/port/qt.py:
     43        * Scripts/webkitpy/layout_tests/port/qt_unittest.py:
     44        * Scripts/webkitpy/layout_tests/port/webkit.py:
     45
    1462011-07-12  Adam Roben  <aroben@apple.com>
    247

  • trunk/Tools/Scripts/webkitpy/common/system/executive.py

    r90534 r90826  
    184184
    185185    @staticmethod
    186     def interpreter_for_script(script_path, fs=FileSystem()):
     186    def interpreter_for_script(script_path, fs=None):
     187        fs = fs or FileSystem()
    187188        lines = fs.read_text_file(script_path).splitlines()
    188189        if not len(lines):
    … …  
    200201
    201202    @staticmethod
    202     def shell_command_for_script(script_path, fs=FileSystem()):
     203    def shell_command_for_script(script_path, fs=None):
     204        fs = fs or FileSystem()
    203205        # Win32 does not support shebang. We need to detect the interpreter ourself.
    204206        if sys.platform == 'win32':

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/config.py

    r90051 r90826  
    132132        # FIXME: See the comment at the top of the file regarding unit tests
    133133        # and our use of global mutable static variables.
     134        # FIXME: We should just @memoize this method and then this will only
     135        # be read once per object lifetime (which should be sufficiently fast).
    134136        global _have_determined_configuration, _configuration
    135137        if not _have_determined_configuration:
    … …  
    147149    def _read_configuration(self):
    148150        try:
    149             configuration_path = self._filesystem.join(self.build_directory(None),
    150                                                        "Configuration")
     151            configuration_path = self._filesystem.join(self.build_directory(None), "Configuration")
    151152            if not self._filesystem.exists(configuration_path):
    152153                return None

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/factory.py

    r89031 r90826  
    7676
    7777    if port_to_use is None:
    78         raise NotImplementedError('unknown port; sys.platform = "%s"' %
    79                                   sys.platform)
     78        raise NotImplementedError('unknown port; sys.platform = "%s"' % sys.platform)
    8079
    8180    if port_to_use.startswith('test'):

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/factory_unittest.py

    r89031 r90826  
    3030import unittest
    3131
    32 from webkitpy.tool import mocktool
     32from webkitpy.common.system.filesystem_mock import MockFileSystem
     33from webkitpy.tool.mocktool import MockOptions, MockUser, MockExecutive
    3334
    3435import chromium_gpu
    … …  
    5758    def setUp(self):
    5859        self.real_sys_platform = sys.platform
    59         self.webkit_options = mocktool.MockOptions(pixel_tests=False)
    60         self.chromium_options = mocktool.MockOptions(pixel_tests=False,
    61                                                     chromium=True)
     60        self.webkit_options = MockOptions(pixel_tests=False)
     61        self.chromium_options = MockOptions(pixel_tests=False, chromium=True)
    6262
    6363    def tearDown(self):
    … …  
    7272          port_obj: optional port object
    7373        """
    74         port_obj = port_obj or factory.get(port_name=port_name)
     74
     75        port_obj = port_obj or factory.get(port_name=port_name, filesystem=MockFileSystem(), user=MockUser(), executive=MockExecutive())
    7576        self.assertTrue(isinstance(port_obj, expected_port))
    7677
    … …  
    8687        orig_platform = sys.platform
    8788        sys.platform = platform
    88         self.assertTrue(isinstance(factory.get(options=options),
    89                                    expected_port))
     89        self.assertTrue(isinstance(factory.get(options=options, filesystem=MockFileSystem(), user=MockUser(), executive=MockExecutive()), expected_port))
    9090        sys.platform = orig_platform
    91 
    92     def test_test(self):
    93         self.assert_port("test", test.TestPort)
    94 
    95     def test_dryrun(self):
    96         self.assert_port("dryrun-test", dryrun.DryRunPort)
    97         self.assert_port("dryrun-mac", dryrun.DryRunPort)
    9891
    9992    def test_mac(self):
    … …  
    112105        # The actual Chrome class names aren't available so we test that the
    113106        # objects we get are at least subclasses of the Chromium versions.
    114         self.assert_port("google-chrome-linux32",
    115                          chromium_linux.ChromiumLinuxPort)
    116         self.assert_port("google-chrome-linux64",
    117                          chromium_linux.ChromiumLinuxPort)
    118         self.assert_port("google-chrome-win",
    119                          chromium_win.ChromiumWinPort)
    120         self.assert_port("google-chrome-mac",
    121                          chromium_mac.ChromiumMacPort)
     107        self.assert_port("google-chrome-linux32", chromium_linux.ChromiumLinuxPort)
     108        self.assert_port("google-chrome-linux64", chromium_linux.ChromiumLinuxPort)
     109        self.assert_port("google-chrome-win", chromium_win.ChromiumWinPort)
     110        self.assert_port("google-chrome-mac", chromium_mac.ChromiumMacPort)
    122111
    123112    def test_gtk(self):
    … …  
    158147        # Test what happens when you specify an unknown port.
    159148        orig_platform = sys.platform
    160         self.assertRaises(NotImplementedError, factory.get,
    161                           port_name='unknown')
     149        self.assertRaises(NotImplementedError, factory.get, port_name='unknown')
    162150
    163151    def test_unknown_default(self):

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/gtk.py

    r90810 r90826  
    6060    port_name = "gtk"
    6161
     62    def _port_flag_for_scripts(self):
     63        return "--gtk"
     64
    6265    def create_driver(self, worker_number):
    6366        return GtkDriver(self, worker_number)
    … …  
    106109    def _runtime_feature_list(self):
    107110        return None
     111
     112    # FIXME: We should find a way to share this implmentation with Gtk,
     113    # or teach run-launcher how to call run-safari and move this down to WebKitPort.
     114    def show_results_html_file(self, results_filename):
     115        run_launcher_args = ["file://%s" % results_filename]
     116        if self.get_option('webkit_test_runner'):
     117            run_launcher_args.append('--webkit-test-runner')
     118        # FIXME: old-run-webkit-tests also added ["-graphicssystem", "raster", "-style", "windows"]
     119        # FIXME: old-run-webkit-tests converted results_filename path for cygwin.
     120        self._run_script("run-launcher", run_launcher_args)

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/mac.py

    r90810 r90826  
    116116    def _path_to_webcore_library(self):
    117117        return self._build_path('WebCore.framework/Versions/A/WebCore')
     118
     119    def show_results_html_file(self, results_filename):
     120        self._run_script('run-safari', ['-NSOpen', results_filename])

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/mac_unittest.py

    r90543 r90826  
    3434from webkitpy.layout_tests.port import port_testcase
    3535from webkitpy.common.system.filesystem_mock import MockFileSystem
     36from webkitpy.common.system.outputcapture import OutputCapture
    3637from webkitpy.tool.mocktool import MockOptions, MockUser, MockExecutive
    3738
    … …  
    3940class MacTest(port_testcase.PortTestCase):
    4041    def port_maker(self, platform):
     42        # FIXME: This platform check should no longer be necessary and should be removed as soon as possible.
    4143        if platform != 'darwin':
    4244            return None
    … …  
    142144        self._assert_search_path(['mac-wk2', 'mac-snowleopard', 'mac'], 'snowleopard', use_webkit2=True)
    143145
     146    def test_show_results_html_file(self):
     147        port = MacPort(filesystem=MockFileSystem(), user=MockUser(), executive=MockExecutive())
     148        # Delay setting a should_log executive to avoid logging from MacPort.__init__.
     149        port._executive = MockExecutive(should_log=True)
     150        expected_stderr = "MOCK run_command: ['Tools/Scripts/run-safari', '--release', '-NSOpen', 'test.html']\n"
     151        OutputCapture().assert_outputs(self, port.show_results_html_file, ["test.html"], expected_stderr=expected_stderr)
     152
    144153
    145154if __name__ == '__main__':

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/qt.py

    r90810 r90826  
    4343    port_name = "qt"
    4444
     45    def _port_flag_for_scripts(self):
     46        return "--qt"
     47
    4548    def _operating_system_for_platform(self, platform):
    4649        if platform.startswith('linux'):
    … …  
    8487        env['QTWEBKIT_PLUGIN_PATH'] = self._build_path('lib/plugins')
    8588        return env
     89
     90    # FIXME: We should find a way to share this implmentation with Gtk,
     91    # or teach run-launcher how to call run-safari and move this down to WebKitPort.
     92    def show_results_html_file(self, results_filename):
     93        run_launcher_args = ["file://%s" % results_filename]
     94        if self.get_option('webkit_test_runner'):
     95            run_launcher_args.append('--webkit-test-runner')
     96        self._run_script("run-launcher", run_launcher_args)

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py

    r90548 r90826  
    3030
    3131from webkitpy.common.system.filesystem_mock import MockFileSystem
     32from webkitpy.common.system.outputcapture import OutputCapture
    3233from webkitpy.layout_tests.port.qt import QtPort
     34from webkitpy.layout_tests.port import port_testcase
    3335from webkitpy.tool.mocktool import MockOptions, MockUser, MockExecutive
    3436
    3537
    36 class QtPortTest(unittest.TestCase):
     38class QtPortTest(port_testcase.PortTestCase):
     39    def port_maker(self, platform):
     40        return QtPort
     41
    3742    def _assert_search_path(self, search_paths, sys_platform, use_webkit2=False):
    3843        # FIXME: Port constructors should not "parse" the port name, but
    … …  
    5762        self._assert_search_path(['qt-wk2', 'qt-win', 'qt'], 'cygwin', use_webkit2=True)
    5863        self._assert_search_path(['qt-wk2', 'qt-linux', 'qt'], 'linux2', use_webkit2=True)
     64
     65    def test_show_results_html_file(self):
     66        port = self.make_port()
     67        port._executive = MockExecutive(should_log=True)
     68        expected_stderr = "MOCK run_command: ['Tools/Scripts/run-launcher', '--release', '--qt', 'file://test.html']\n"
     69        OutputCapture().assert_outputs(self, port.show_results_html_file, ["test.html"], expected_stderr=expected_stderr)

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py

    r90810 r90826  
    9494        return "build-dumprendertree"
    9595
     96    def _port_flag_for_scripts(self):
     97        # This is overrriden by ports which need a flag passed to scripts to distinguish the use of that port.
     98        # For example --qt on linux, since a user might have both Gtk and Qt libraries installed.
     99        # FIXME: Chromium should override this once ChromiumPort is a WebKitPort.
     100        return None
     101
     102    # This is modeled after webkitdirs.pm argumentsForConfiguration() from old-run-webkit-tests
     103    def _arguments_for_configuration(self):
     104        config_args = []
     105        config_args.append(self._config.flag_for_configuration(self.get_option('configuration')))
     106        # FIXME: We may need to add support for passing --32-bit like old-run-webkit-tests had.
     107        port_flag = self._port_flag_for_scripts()
     108        if port_flag:
     109            config_args.append(port_flag)
     110        return config_args
     111
     112    def _run_script(self, script_name, args=None, include_configuration_arguments=True):
     113        run_script_command = [self._config.script_path(script_name)]
     114        if include_configuration_arguments:
     115            run_script_command.extend(self._arguments_for_configuration())
     116        if args:
     117            run_script_command.extend(args)
     118        return self._executive.run_command(run_script_command, cwd=self._config.webkit_base_dir())  # It's unclear if setting cwd is necessary for all callers.
     119
    96120    def _build_driver(self):
    97         configuration = self.get_option('configuration')
    98121        try:
    99             # FIXME: We should probably have a run_script helper which automatically adds the configuration flags.
    100             self._executive.run_command([
    101                 self._config.script_path(self._driver_build_script_name()),
    102                 self._config.flag_for_configuration(configuration)],
    103                 # FIXME: It's unclear if this cwd= is necessary. Again a helper should do this for us.
    104                 cwd=self._config.webkit_base_dir())
     122            self._run_script(self._driver_build_script_name())
    105123        except ScriptError:
    106124            _log.error("Failed to build %s" % self.driver_name())
Note: See TracChangeset for help on using the changeset viewer.