New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 711515 link

Starred by 2 users
Status: Fixed
Owner:
qyears...@chromium.org
Closed: Apr 2017
Cc:
█ tansell@chromium.org
█ kochi@chromium.org
dpranke@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocking:
issue 605496



Sign in to add a comment

Add support for run-webkit-tests to start xvfb when necessary.

Project Member Reported by qyears...@chromium.org, Apr 13 2017 
Background:

Historically when the layout tests are run in our older CI infrastructure, they're executed via runtest.py which starts if necessary using build/scripts/slave/xvfb.py.

This version starts openbox and then starts a process:
  Xvfb :9 -screen 0 1280x800x24 -ac -dpi 96
It saves the pid to a file so it can clean up this process later.

For swarming, now, layout tests are run via run_isolated_script_test.py, which starts xvfb using src/testing/xvfb.py:

*This* version starts openbox and xcompmgr and then starts:
xvfb-run -a "--server-args=-screen 0 1280x800x24 -ac -nolisten tcp -dpi 96" <run-webkit-tests>

According to comments in src/testing/xvfb.py openbox is for ChromeOS-related tests and so may not be required; xcompmgr may also not be required.

On my workstation:
  Xvfb :8 -screen 0 1280x800x24 -ac -dpi 96
  DISPLAY=:8 ./run-webkit-tests --smoke

passes all the smoke tests,

whereas just `DISPLAY=:8 ./run-webkit-tests --smoke` fails with a "Unable to open X display" error.


The most obvious place to initialize and tear down Xvfb will be _set_up_run and _tear_down_run in Manager:
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py?l=327

We'll want to only start xvfb if we're on linux there's no X server currently. This could be checked with the current "system dependencies check" or by invoking another command that will exit with non-zero status with no X, e.g. `xprop -root`.

Comment 1 by dpranke@chromium.org, Apr 13 2017 

You might consider always running under xvfb, and only using the native display if someone passed a flag like --no-xvfb or something.

Also, checking whether or not there's already a display is literally the only purpose of //tools/xdisplaycheck :).

Comment 2 by qyears...@chromium.org, Apr 14 2017

Status: Started (was: Assigned)
Always running under xvfb:
Pros:
 - don't need to check whether X is running
 - may make test runs more consistent
Cons:
 - requires starting xvfb even if an X server is already running
 - requires adding a flag like --no-xvfb
 - may not be what some people expect on their workstations
Either way seems fine to me.

xdisplaycheck seems to exit quickly if X is running, but it waits and retries if it's not running, so it's a bit slower; it could be used, but is there any advantage to using that over running `xdpycheck` or `xprop -root`?

Now uploaded a CL for review :-)
https://codereview.chromium.org/2821543003

Comment 3 by dpranke@chromium.org, Apr 14 2017 

Dunno if there's a real advantage, though if we don't want to use xdisplaycheck any more, we should probably just remove it.

I think it's better to run under xvfb by default to better match what the bots do.

Comment 5 by qyears...@chromium.org, Apr 18 2017

Status: Fixed (was: Started)
Alright, it appears that this change didn't make much of a difference; the WebKit Linux Trusty jobs are still passing, and the Site Isolation Linux jobs are still failing in the same way; in both cases it logs something like

13:27:12.945 8176 "xdpyinfo -display :99" took 0.06s
13:27:12.946 8176 Starting Xvfb with display ":99".

so we can tell that it's starting Xvfb in run-webkit-tests.

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty
https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53b27041e4b3618926878898d8e57d7c7d73d157

commit 53b27041e4b3618926878898d8e57d7c7d73d157
Author: ojan <ojan@chromium.org>
Date: Wed Apr 19 00:19:03 2017

Revert of In run-webkit-tests, start xvfb if necessary. (patchset #6 id:100001 of https://codereview.chromium.org/2821543003/ )

Reason for revert:
Looks like it broke WebKit Android (Nexus4).

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Nexus4%29/builds/63884

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit_Android__Nexus4_%2F63893%2F%2B%2Frecipes%2Fsteps%2Fwebkit_tests%2F0%2Fstdout

Original issue's description:
> In run-webkit-tests, start xvfb if necessary.
>
> BUG= 711515 
>
> Review-Url: https://codereview.chromium.org/2821543003
> Cr-Commit-Position: refs/heads/master@{#465331}
> Committed: https://chromium.googlesource.com/chromium/src/+/8e209a99612f0af2a91c940967eacef653fa20b1

TBR=dpranke@chromium.org,tansell@chromium.org,qyearsley@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 711515 

Review-Url: https://codereview.chromium.org/2828623002
Cr-Commit-Position: refs/heads/master@{#465428}

[modify] https://crrev.com/53b27041e4b3618926878898d8e57d7c7d73d157/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
[modify] https://crrev.com/53b27041e4b3618926878898d8e57d7c7d73d157/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
[modify] https://crrev.com/53b27041e4b3618926878898d8e57d7c7d73d157/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
[modify] https://crrev.com/53b27041e4b3618926878898d8e57d7c7d73d157/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux_unittest.py

Comment 7 by qyears...@chromium.org, Apr 19 2017

Status: Started (was: Fixed)
In theory, the part of that CL that may affect android would be the change in controllers/manager.py, I think.

The next step will be to make another version of that change with a possible fix, and run it on android_blink_rel to make sure it works.

Comment 8 by qyears...@chromium.org, Apr 19 2017

Blocking: -708681

Comment 10 by raphael....@intel.com, Apr 20 2017 

The change from comment #9 is preventing run-webkit-tests from launching a browser with layout test results here. I'm doing something like this:

  $ ./third_party/WebKit/Tools/Scripts/run-webkit-tests --no-retry-failures --disable-breakpad --no-pixel-tests --debug [paths]

if something fails, DISPLAY will still be set to whatever Xvfb was using, and no browser is launched.

Comment 11 by qyears...@chromium.org, Apr 20 2017

Cc: kochi@chromium.org
Ah, I see! 

In webkitpy/layout_tests/manager.py, showing the results file is done after asking the ports to clean up the test run, so if the linux port can just save the original DISPLAY and restore it when cleaning up, I believe that should fix this issue. Will make a CL to try this now.

Of course, if it were changed to not change DISPLAY, that would also be good.
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4e29231f2da03e0f447b5d4cbc2a4278bb9223e

commit e4e29231f2da03e0f447b5d4cbc2a4278bb9223e
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Apr 20 18:46:14 2017

Save and restore DISPLAY so that results.html can be shown.

BUG= 711515 

Review-Url: https://codereview.chromium.org/2829123002
Cr-Commit-Position: refs/heads/master@{#466084}

[modify] https://crrev.com/e4e29231f2da03e0f447b5d4cbc2a4278bb9223e/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py

Comment 14 by qyears...@chromium.org, Apr 20 2017

Status: Fixed (was: Started)
►

Sign in to add a comment