Summary: | [GTK] run-gtk-tests should use webkitpy.port.gtk and webkitpy.port.xvfbdriver | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||
Component: | Tools / Tests | Assignee: | Carlos Alberto Lopez Perez <clopez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, bugs-noreply, cgarcia, commit-queue, dbates, glenn, lforschler, mcatanzaro, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 161149 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Carlos Alberto Lopez Perez
2016-08-10 04:48:18 PDT
Created attachment 286713 [details]
Patch
Comment on attachment 286713 [details] Patch 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 else: 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()) Created attachment 286865 [details] Patch 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 Committed r205014: <http://trac.webkit.org/changeset/205014> |