Bug 160736 - [GTK] run-gtk-tests should use webkitpy.port.gtk and webkitpy.port.xvfbdriver
Summary: [GTK] run-gtk-tests should use webkitpy.port.gtk and webkitpy.port.xvfbdriver
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
Depends on: 161149
  Show dependency treegraph
Reported: 2016-08-10 04:48 PDT by Carlos Alberto Lopez Perez
Modified: 2016-08-26 09:34 PDT (History)
9 users (show)

See Also:

Patch (18.43 KB, patch)
2016-08-23 08:36 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (18.50 KB, patch)
2016-08-24 10:44 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2016-08-10 04:48:18 PDT
The script run-gtk-tests implements its own _run_xvfb() and its own _setup_testing_environment() instead of relying on webkitpy.port.xvfbdriver:XvfbDriver and webkitpy.port.gtk:GtkPort::setup_environ_for_server()

As a result of this, the glx/egl software rasterizer built by the jhbuild is not used, defaulting to use the system one.

This is causing errors on the ARM bot, that seems don't has a proper one shipped by the system:


ERROR:../../Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:58:static gboolean WebViewTest::webProcessCrashed(WebKitWebView*, WebViewTest*): code should not be reached


GTester: last random seed: R02Sd767ea08b233d8032c8a8ba1cb7a7bf8


  /webkit2/WebKitWebView/menu-dismissed:                               libEGL warning: GLX/DRI2 is not supported

libEGL warning: DRI2: failed to open swrast (search paths /usr/lib/arm-linux-gnueabihf/dri:${ORIGIN}/dri:/usr/lib/dri)

Xlib: sequence lost (0x10096 > 0x97) in reply type 0x0!
Comment 1 Carlos Alberto Lopez Perez 2016-08-23 08:36:30 PDT
Created attachment 286713 [details]
Comment 2 Carlos Garcia Campos 2016-08-23 23:42:51 PDT
Comment on attachment 286713 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=286713&action=review

Excellent! I have only a couple of comments

> Tools/Scripts/run-gtk-tests:186
> +        port_name = "gtk"
> +        self._port = Host().port_factory.get(port_name, port_options)

port_name is only used here, you could probably pass gtk directly to the factory method. Also, this is called create_driver but it's also creating the port, I would move this to the __init__ instead, and create_driver should assume self_port already exists.

self._port = Host().port_factory.get('gtk')
self._driver = self._create_driver()

no need to create and empty port_options list if it's not really used.

> Tools/Scripts/run-gtk-tests:190
> +            self._port._nativexorg = True
> +        if self._options.use_wayland:
> +            self._port._wayland = True

When wayland is true, whether using xvfb or not is just ignored, so I would make it clearer that here with something like:

if self._options.use_wayland:
    self._port._wayland = True
    self._port._nativexorg = not self._options.use_xvfb

> Tools/Scripts/run-gtk-tests:193
> +        if not driver.check_driver(self._port):
> +            raise RuntimeError("Failed to check driver %s" %driver.__class__.__name__)

This is not handled anywhere. In this case we should ensure the script finishes with an exit error code. At the moment the constructor is not expected to fail and we do:

sys.exit(TestRunner(options, args).run_tests())

now we should create the TesRunner first inside a try except block and exit early if the creation fails, and then sys.exit(runner.run_tests())
Comment 3 Carlos Alberto Lopez Perez 2016-08-24 10:44:53 PDT
Created attachment 286865 [details]

Patch for landing addressing reviewer comments. Will not land until blocking bug 161149 is also ready for land. Otherwise the bots could hand without the fix from bug 161149
Comment 4 Carlos Alberto Lopez Perez 2016-08-26 09:34:24 PDT
Committed r205014: <http://trac.webkit.org/changeset/205014>