Use Xvfb in isolated testing only if use_x11 is set (reland)

Use Xvfb in isolated testing only if use_x11 is set (reland)

We don't need a virtual X server for ozone testing, and it is causing
problems running the tests with swarming. This changes all isolate files
to only run Xvfb if use_x11==1 is set in GYP_DEFINES, and merges ozone
with Windows & Mac (none of which need to run their own display server).

BUG=440882
TEST=isolate.py run -s out_ozone/Debug/<various>
TBR=maruel

Committed: https://crrev.com/386febffff89e5b08b7dc0a5d6bd992df385e0b8
Cr-Commit-Position: refs/heads/master@{#319919}

[spang, 2015-02-10 23:33:02]

Patchset #1 (id:1) has been deleted

[M-A Ruel, 2015-02-18 22:11:34]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2015-02-18 22:11:34]

https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
chrome/browser_tests.isolate:9: '../testing/test_env.py',
So when use_x11==0 is set, chrome is started instead of browser_tests?

[spang, 2015-02-18 22:38:48]

On 2015/02/18 22:11:34, M-A Ruel wrote:
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> File chrome/browser_tests.isolate (right):
> 
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> chrome/browser_tests.isolate:9: '../testing/test_env.py',
> So when use_x11==0 is set, chrome is started instead of browser_tests?

It's tab_capture_end2end_tests that is breaking with this particular patch. If
you look at the bug, that's what got my original attempt reverted, and this
problem also shows in the red try runs for this patch.

[spang, 2015-02-18 22:39:14]

On 2015/02/18 22:38:48, spang wrote:
> On 2015/02/18 22:11:34, M-A Ruel wrote:
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > File chrome/browser_tests.isolate (right):
> > 
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > So when use_x11==0 is set, chrome is started instead of browser_tests?
> 
> It's tab_capture_end2end_tests that is breaking with this particular patch. If
> you look at the bug, that's what got my original attempt reverted, and this
> problem also shows in the red try runs for this patch.

The symptom is that instead of running just the tab capture test, it runs all of
browser_tests.

[M-A Ruel, 2015-02-18 23:49:47]

On 2015/02/18 22:39:14, spang wrote:
> On 2015/02/18 22:38:48, spang wrote:
> > On 2015/02/18 22:11:34, M-A Ruel wrote:
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > File chrome/browser_tests.isolate (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > > So when use_x11==0 is set, chrome is started instead of browser_tests?
> > 
> > It's tab_capture_end2end_tests that is breaking with this particular patch.
If
> > you look at the bug, that's what got my original attempt reverted, and this
> > problem also shows in the red try runs for this patch.
> 
> The symptom is that instead of running just the tab capture test, it runs all
of
> browser_tests.

Ok, so you state that with this CL;
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/tab_captur...
doesn't take effect, right? That's not what you asked initially.

[spang, 2015-02-18 23:57:06]

On 2015/02/18 23:49:47, M-A Ruel wrote:
> On 2015/02/18 22:39:14, spang wrote:
> > On 2015/02/18 22:38:48, spang wrote:
> > > On 2015/02/18 22:11:34, M-A Ruel wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > File chrome/browser_tests.isolate (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > > > So when use_x11==0 is set, chrome is started instead of browser_tests?
> > > 
> > > It's tab_capture_end2end_tests that is breaking with this particular
patch.
> If
> > > you look at the bug, that's what got my original attempt reverted, and
this
> > > problem also shows in the red try runs for this patch.
> > 
> > The symptom is that instead of running just the tab capture test, it runs
all
> of
> > browser_tests.
> 
> Ok, so you state that with this CL;
>
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/tab_captur...
> doesn't take effect, right? That's not what you asked initially.

Right.

I'm sorry for asking a different question. My investigation led to many
questions, and it seemed much more concise example of something going wrong.

[M-A Ruel, 2015-02-19 00:00:34]

On 2015/02/18 23:57:06, spang wrote:
> On 2015/02/18 23:49:47, M-A Ruel wrote:
> > On 2015/02/18 22:39:14, spang wrote:
> > > On 2015/02/18 22:38:48, spang wrote:
> > > > On 2015/02/18 22:11:34, M-A Ruel wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > File chrome/browser_tests.isolate (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > > > > So when use_x11==0 is set, chrome is started instead of browser_tests?
> > > > 
> > > > It's tab_capture_end2end_tests that is breaking with this particular
> patch.
> > If
> > > > you look at the bug, that's what got my original attempt reverted, and
> this
> > > > problem also shows in the red try runs for this patch.
> > > 
> > > The symptom is that instead of running just the tab capture test, it runs
> all
> > of
> > > browser_tests.
> > 
> > Ok, so you state that with this CL;
> >
>
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/tab_captur...
> > doesn't take effect, right? That's not what you asked initially.
> 
> Right.
> 
> I'm sorry for asking a different question. My investigation led to many
> questions, and it seemed much more concise example of something going wrong.

Ok, so do this;
#1 reset GYP_DEFINES to something simple normal chromium build, not ozone.
#2 gclient runhooks
#3 ninja -C out/Release tab_capture_end2end_tests_run
#4 python -m json.tool out/Release/tab_capture_end2end_tests.isolated | less

Look at the command to see if it is what is expected.

[spang, 2015-02-19 01:29:20]

On 2015/02/19 00:00:34, M-A Ruel wrote:
> On 2015/02/18 23:57:06, spang wrote:
> > On 2015/02/18 23:49:47, M-A Ruel wrote:
> > > On 2015/02/18 22:39:14, spang wrote:
> > > > On 2015/02/18 22:38:48, spang wrote:
> > > > > On 2015/02/18 22:11:34, M-A Ruel wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > > File chrome/browser_tests.isolate (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > > > > > So when use_x11==0 is set, chrome is started instead of
browser_tests?
> > > > > 
> > > > > It's tab_capture_end2end_tests that is breaking with this particular
> > patch.
> > > If
> > > > > you look at the bug, that's what got my original attempt reverted, and
> > this
> > > > > problem also shows in the red try runs for this patch.
> > > > 
> > > > The symptom is that instead of running just the tab capture test, it
runs
> > all
> > > of
> > > > browser_tests.
> > > 
> > > Ok, so you state that with this CL;
> > >
> >
>
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/tab_captur...
> > > doesn't take effect, right? That's not what you asked initially.
> > 
> > Right.
> > 
> > I'm sorry for asking a different question. My investigation led to many
> > questions, and it seemed much more concise example of something going wrong.
> 
> Ok, so do this;
> #1 reset GYP_DEFINES to something simple normal chromium build, not ozone.
> #2 gclient runhooks
> #3 ninja -C out/Release tab_capture_end2end_tests_run
> #4 python -m json.tool out/Release/tab_capture_end2end_tests.isolated | less
> 
> Look at the command to see if it is what is expected.

Sure, if I test for the usual linux x11 build it shows that it's getting the
command from browser_tests.isolate instead of the one from
tab_capture_end2end_tests.isolate. Which isn't what I expect because I'm not
making any changes to that build.

Here's my command line:

  GYP_DEFINES="use_goma=1 test_isolation_mode=check archive_gpu_tests=1"
GYP_GENERATOR_FLAGS="output_dir=out_linux_isolate" gclient runhooks

  ninja -C out_linux_isolate/Debug -j1000 -k50 tab_capture_end2end_tests_run
browser_tests_run

  python -m json.tool
out_linux_isolate/Debug/tab_capture_end2end_tests.isolated|less

Do you see anything in my CL that would cause a change to linux x11? All the
conditions should have equivalent results when evaluated.

To narrow things down a bit, this patch alone breaks it:
https://codereview.chromium.org/936303002/

It causes this output for tab_capture_end2end_tests:

    "command": [
        "../testing/xvfb.py",
        "../out_linux_isolate/Debug",
        "../out_linux_isolate/Debug/browser_tests",
        "--test-launcher-bot-mode",
        "--asan=0",
        "--lsan=0",
        "--msan=0",
        "--tsan=0"
    ],

.. which is the wrong command.

The thing I'm stuck on is understanding why the output has changed.

[M-A Ruel, 2015-02-19 01:44:06]

On 2015/02/19 01:29:20, spang wrote:
> On 2015/02/19 00:00:34, M-A Ruel wrote:
> > On 2015/02/18 23:57:06, spang wrote:
> > > On 2015/02/18 23:49:47, M-A Ruel wrote:
> > > > On 2015/02/18 22:39:14, spang wrote:
> > > > > On 2015/02/18 22:38:48, spang wrote:
> > > > > > On 2015/02/18 22:11:34, M-A Ruel wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > > > File chrome/browser_tests.isolate (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > > > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > > > > > > So when use_x11==0 is set, chrome is started instead of
> browser_tests?
> > > > > > 
> > > > > > It's tab_capture_end2end_tests that is breaking with this particular
> > > patch.
> > > > If
> > > > > > you look at the bug, that's what got my original attempt reverted,
and
> > > this
> > > > > > problem also shows in the red try runs for this patch.
> > > > > 
> > > > > The symptom is that instead of running just the tab capture test, it
> runs
> > > all
> > > > of
> > > > > browser_tests.
> > > > 
> > > > Ok, so you state that with this CL;
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/tab_captur...
> > > > doesn't take effect, right? That's not what you asked initially.
> > > 
> > > Right.
> > > 
> > > I'm sorry for asking a different question. My investigation led to many
> > > questions, and it seemed much more concise example of something going
wrong.
> > 
> > Ok, so do this;
> > #1 reset GYP_DEFINES to something simple normal chromium build, not ozone.
> > #2 gclient runhooks
> > #3 ninja -C out/Release tab_capture_end2end_tests_run
> > #4 python -m json.tool out/Release/tab_capture_end2end_tests.isolated | less
> > 
> > Look at the command to see if it is what is expected.
> 
> Sure, if I test for the usual linux x11 build it shows that it's getting the
> command from browser_tests.isolate instead of the one from
> tab_capture_end2end_tests.isolate. Which isn't what I expect because I'm not
> making any changes to that build.
> 
> Here's my command line:
> 
>   GYP_DEFINES="use_goma=1 test_isolation_mode=check archive_gpu_tests=1"
> GYP_GENERATOR_FLAGS="output_dir=out_linux_isolate" gclient runhooks
> 
>   ninja -C out_linux_isolate/Debug -j1000 -k50 tab_capture_end2end_tests_run
> browser_tests_run
> 
>   python -m json.tool
> out_linux_isolate/Debug/tab_capture_end2end_tests.isolated|less
> 
> Do you see anything in my CL that would cause a change to linux x11? All the
> conditions should have equivalent results when evaluated.
> 
> To narrow things down a bit, this patch alone breaks it:
> https://codereview.chromium.org/936303002/
> 
> It causes this output for tab_capture_end2end_tests:
> 
>     "command": [
>         "../testing/xvfb.py",
>         "../out_linux_isolate/Debug",
>         "../out_linux_isolate/Debug/browser_tests",
>         "--test-launcher-bot-mode",
>         "--asan=0",
>         "--lsan=0",
>         "--msan=0",
>         "--tsan=0"
>     ],
> 
> .. which is the wrong command.
> 
> The thing I'm stuck on is understanding why the output has changed.

Ok, use 'OS=="linux" and use_x11=1' in the meantime. That should workaround the
problem.

[spang, 2015-02-19 02:15:36]

On 2015/02/19 01:44:06, M-A Ruel wrote:
> On 2015/02/19 01:29:20, spang wrote:
> > On 2015/02/19 00:00:34, M-A Ruel wrote:
> > > On 2015/02/18 23:57:06, spang wrote:
> > > > On 2015/02/18 23:49:47, M-A Ruel wrote:
> > > > > On 2015/02/18 22:39:14, spang wrote:
> > > > > > On 2015/02/18 22:38:48, spang wrote:
> > > > > > > On 2015/02/18 22:11:34, M-A Ruel wrote:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > > > > File chrome/browser_tests.isolate (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/908313004/diff/20001/chrome/browser_tests.iso...
> > > > > > > > chrome/browser_tests.isolate:9: '../testing/test_env.py',
> > > > > > > > So when use_x11==0 is set, chrome is started instead of
> > browser_tests?
> > > > > > > 
> > > > > > > It's tab_capture_end2end_tests that is breaking with this
particular
> > > > patch.
> > > > > If
> > > > > > > you look at the bug, that's what got my original attempt reverted,
> and
> > > > this
> > > > > > > problem also shows in the red try runs for this patch.
> > > > > > 
> > > > > > The symptom is that instead of running just the tab capture test, it
> > runs
> > > > all
> > > > > of
> > > > > > browser_tests.
> > > > > 
> > > > > Ok, so you state that with this CL;
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/tab_captur...
> > > > > doesn't take effect, right? That's not what you asked initially.
> > > > 
> > > > Right.
> > > > 
> > > > I'm sorry for asking a different question. My investigation led to many
> > > > questions, and it seemed much more concise example of something going
> wrong.
> > > 
> > > Ok, so do this;
> > > #1 reset GYP_DEFINES to something simple normal chromium build, not ozone.
> > > #2 gclient runhooks
> > > #3 ninja -C out/Release tab_capture_end2end_tests_run
> > > #4 python -m json.tool out/Release/tab_capture_end2end_tests.isolated |
less
> > > 
> > > Look at the command to see if it is what is expected.
> > 
> > Sure, if I test for the usual linux x11 build it shows that it's getting the
> > command from browser_tests.isolate instead of the one from
> > tab_capture_end2end_tests.isolate. Which isn't what I expect because I'm not
> > making any changes to that build.
> > 
> > Here's my command line:
> > 
> >   GYP_DEFINES="use_goma=1 test_isolation_mode=check archive_gpu_tests=1"
> > GYP_GENERATOR_FLAGS="output_dir=out_linux_isolate" gclient runhooks
> > 
> >   ninja -C out_linux_isolate/Debug -j1000 -k50 tab_capture_end2end_tests_run
> > browser_tests_run
> > 
> >   python -m json.tool
> > out_linux_isolate/Debug/tab_capture_end2end_tests.isolated|less
> > 
> > Do you see anything in my CL that would cause a change to linux x11? All the
> > conditions should have equivalent results when evaluated.
> > 
> > To narrow things down a bit, this patch alone breaks it:
> > https://codereview.chromium.org/936303002/
> > 
> > It causes this output for tab_capture_end2end_tests:
> > 
> >     "command": [
> >         "../testing/xvfb.py",
> >         "../out_linux_isolate/Debug",
> >         "../out_linux_isolate/Debug/browser_tests",
> >         "--test-launcher-bot-mode",
> >         "--asan=0",
> >         "--lsan=0",
> >         "--msan=0",
> >         "--tsan=0"
> >     ],
> > 
> > .. which is the wrong command.
> > 
> > The thing I'm stuck on is understanding why the output has changed.
> 
> Ok, use 'OS=="linux" and use_x11=1' in the meantime. That should workaround
the
> problem.

Thanks, works locally. Running it through the tryserver to see if anything else
is amiss

[M-A Ruel, 2015-02-19 16:31:06]

On 2015/02/19 02:15:36, spang wrote:
> Thanks, works locally. Running it through the tryserver to see if anything
else
> is amiss

Saw the failures, will try to take a look asap.

[alexst (slow to review), 2015-02-25 19:09:34]

alexst@chromium.org changed reviewers:
+ alexst@chromium.org

[alexst (slow to review), 2015-02-25 19:09:34]

Hi Marc-Antoine, any luck?

[M-A Ruel, 2015-02-26 02:04:47]

On 2015/02/25 19:09:34, alexst wrote:
> Hi Marc-Antoine, any luck?

https://codereview.appspot.com/208830043/

[M-A Ruel, 2015-03-10 00:09:49]

Roll is in now (I was OOO for the last 10 days), please try now.

[spang, 2015-03-10 00:20:06]

The CQ bit was checked by spang@chromium.org

[spang, 2015-03-10 00:20:06]

The CQ bit was unchecked by spang@chromium.org

[M-A Ruel, 2015-03-10 16:10:36]

On 2015/03/10 00:09:49, M-A Ruel wrote:
> Roll is in now (I was OOO for the last 10 days), please try now.

Please try again, thanks.

[spang, 2015-03-10 16:20:02]

On 2015/03/10 16:10:36, M-A Ruel wrote:
> On 2015/03/10 00:09:49, M-A Ruel wrote:
> > Roll is in now (I was OOO for the last 10 days), please try now.
> 
> Please try again, thanks.

Looks green, can you review?

[M-A Ruel, 2015-03-10 16:24:52]

Do you mind one last change? Move all the use_x11==0 / use_x11==1 pairs at the
top of the file. The rationale is that they are defining the command, and I'd
like this to be consistent across isolate files, so that they all be at the same
place.

https://codereview.chromium.org/908313004/diff/60001/build/isolate.gypi
File build/isolate.gypi (right):

https://codereview.chromium.org/908313004/diff/60001/build/isolate.gypi#newco...
build/isolate.gypi:99: '--config-variable', 'use_x11=<(use_x11)',
Please keep the list sorted.

https://codereview.chromium.org/908313004/diff/60001/chrome/browser_tests.iso...
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/908313004/diff/60001/chrome/browser_tests.iso...
chrome/browser_tests.isolate:18: ['OS=="linux" and use_x11==1', {
You should be able to use: 'use_x11==1' here now.

[spang, 2015-03-10 16:34:28]

Sure, moved them to the beginning. I had moved them next to one another already.

https://codereview.chromium.org/908313004/diff/60001/build/isolate.gypi
File build/isolate.gypi (right):

https://codereview.chromium.org/908313004/diff/60001/build/isolate.gypi#newco...
build/isolate.gypi:99: '--config-variable', 'use_x11=<(use_x11)',
On 2015/03/10 16:24:52, M-A Ruel wrote:
> Please keep the list sorted.

Done.

https://codereview.chromium.org/908313004/diff/60001/chrome/browser_tests.iso...
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/908313004/diff/60001/chrome/browser_tests.iso...
chrome/browser_tests.isolate:18: ['OS=="linux" and use_x11==1', {
On 2015/03/10 16:24:52, M-A Ruel wrote:
> You should be able to use: 'use_x11==1' here now.

Done.

[M-A Ruel, 2015-03-10 16:39:28]

Awesome, thanks.

lgtm

Sorry for the painful process.

[spang, 2015-03-10 16:50:07]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2015-03-10 16:50:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908313004/80001

[commit-bot: I haz the power, 2015-03-10 18:01:18]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-03-10 18:02:16]

Patchset 4 (id:??) landed as
https://crrev.com/386febffff89e5b08b7dc0a5d6bd992df385e0b8
Cr-Commit-Position: refs/heads/master@{#319919}