Bug 57640

How many d&d tests are there? Another way to do this is to put all of the d&d tests into a single shard. I'm loathe to spin up multiple xvfb's, but maybe it makes sense to do so. We can certainly do some profiling to find out. It would be kinda nice to have NRWT automatically start its own xvfb's rather than relying on the user to do so.

(In reply to comment #1) > How many d&d tests are there? Another way to do this is to put all of the d&d tests into a single shard. There probably aren't that many, but they're not all in the same place (off the top of my head, some are in editing, some are in http, and some are in fast).

(In reply to comment #0) > The GTK+ DRT makes real d&d calls which can cause problems if two processes try to run a d&d test at the same time. A simple workaround is to give each NRWT process a separate Xvfb server to use. This could probably happen in 2 different ways: > > 1) Each NRWT thread is taught to start/stop xvfb on it's own. > 2) The buildbot slave wrapper scripts (Tools/BuildSlaveSupport/gtk/xvfb/run and gtk/buildbot/run) can launch N Xvfb processes and NRWT just needs to set the right DISPLAY for each process. > > (1) sounds a bit easier than (2), but either is probably OK. > We are interested in moving GTK+ over NWRT ASAP, so I'm willing to spend some time on this. I've been looking around the code and I was wondering what is the best way to implement this exactly. My first intuition would be to have a GTK+ Driver (like Chromium's) that knows how to deal with Xvfb, but I wonder if I'd be reimplementing too much stuff here. Suggestions?

It should only be a couple of lines of code to launch an Xvfb prior to the DRT, and AFAIK that wouldn't be duplicating anything we have elsewhere, so I say go ahead. Seems like a reasonable fix.

Created attachment 96434 [details] Patch

(In reply to comment #5) > Created an attachment (id=96434) [details] > Patch A couple of comments: - I had to copy the start method from the WebKit driver because I need to add things to the environment and the port method to set it up runs to soon (I don't know the worker number). I guess it can be fixed with some refactoring. - The Xvfb processes are not killed if the test run is stopped with Ctrl-C, not sure how to do this.

Attachment 96434 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/layout_tests/port/gtk.py:41: expected 2 blank lines, found 1 [pep8/E302] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 96434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96434&action=review > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:52 > + environment['DYLD_FRAMEWORK_PATH'] = self._port._build_path() Is this actually need for GTK+? This environment variable controls Framework loading on OS X.

(In reply to comment #8) > (From update of attachment 96434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96434&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:52 > > + environment['DYLD_FRAMEWORK_PATH'] = self._port._build_path() > > Is this actually need for GTK+? This environment variable controls Framework loading on OS X. I don't think we use any of the variables I copied, this one we don't for sure. I guess I just left it there in case we want to make this "more portable" and to keep the exact behavior we have now, but probably we should just make it GTK+-only for now.

Comment on attachment 96434 [details] Patch (In reply to comment #6) > (In reply to comment #5) > > Created an attachment (id=96434) [details] [details] > > Patch > > A couple of comments: > > - I had to copy the start method from the WebKit driver because I need to add things to the environment and the port method to set it up runs to soon (I don't know the worker number). I guess it can be fixed with some refactoring. > Overriding the start method is exactly what I'd expect you to do. I don't think I'd want to change or refactor anything, but maybe I'm not understanding your concern? > - The Xvfb processes are not killed if the test run is stopped with Ctrl-C, not sure how to do this. This is a problem. Is driver.stop() not getting called after the Ctrl-C is received? It should be. You can try adding a _log.debug() message and then running with --verbose to see what's going on one way or another; feel free to post the log or email it to me for help debugging it. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:37 > +from webkitpy.layout_tests.port.webkit import WebKitPort, WebKitDriver Nit: I usually prefer to just import the module, and prefix the symbols with the module name (so, webkit.WebKitPort below instead of just WebKitPort); I think Eric wrote this code originally before we converged on that style. You can change this or not as you like for now. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:49 > + run_xvfb = ["Xvfb", ":%d" % (self._worker_number + 1)] Nit: I'd probably do something like 'display_id = self._worker_number + 1' here to make it clear and avoid having to do the computation twice. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:50 > + self._process = subprocess.Popen(run_xvfb) Can you rename this to self._xvfb_process or something like that? _process is a little too generic. >>> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:52 >>> + environment['DYLD_FRAMEWORK_PATH'] = self._port._build_path() >> >> Is this actually need for GTK+? This environment variable controls Framework loading on OS X. > > I don't think we use any of the variables I copied, this one we don't for sure. I guess I just left it there in case we want to make this "more portable" and to keep the exact behavior we have now, but probably we should just make it GTK+-only for now. Definitely remove the DYLD_* and DUMPRENDERTREE_TEMP variables if you don't need them. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:72 > + self._worker_number = worker_number Do you need self._worker_number? I don't see it being used anywhere.

(In reply to comment #10) > Overriding the start method is exactly what I'd expect you to do. I don't think I'd want to change or refactor anything, but maybe I'm not understanding your concern? I meant that it seems to me I could just override the Port method to setup the initial environment and chain to the parent class' start, but that runs too early for my needs (because I need to know the worker number), so in the end I just did my own start from scratch. No big deal, it's a tiny thing. > > > - The Xvfb processes are not killed if the test run is stopped with Ctrl-C, not sure how to do this. > > This is a problem. Is driver.stop() not getting called after the Ctrl-C is received? It should be. You can try adding a _log.debug() message and then running with --verbose to see what's going on one way or another; feel free to post the log or email it to me for help debugging it. Alright, will do. > > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:72 > > + self._worker_number = worker_number > > Do you need self._worker_number? I don't see it being used anywhere. Nope, leftover from another iteration!

Created attachment 96588 [details] Patch

(In reply to comment #12) > Created an attachment (id=96588) [details] > Patch Cannot reproduce anymore the bug with the Xvfb servers not being killed on Ctrl-C, so I guess my script was buggy when that happened. Sorry for the noise.

Comment on attachment 96588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96588&action=review > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:51 > + "DumpRenderTree", self.cmd_line(), environment) "DumpRenderTree" should probably self._port.driver_name() since you'll want to support WebKit2. > Tools/Scripts/webkitpy/layout_tests/port/gtk.py:67 > + """Starts a new Driver and returns a handle to it.""" I'd get rid of this docstring.

Comment on attachment 96588 [details] Patch Landed with requested changes as r90036

All patches landed, closing.