PlzNavigate: adds Linux runs of content_browsertests to the CQ.

PlzNavigate: adds Linux runs of content_browsertests to the CQ.

Adding Linux runs of content_browsertests with PlzNavigate enabled to the
CQ. Note that it is still using an exclusion filter for the bits of content/
that PlzNavigate is not yet supporting.

If you need assistance getting your change deployed because of
these tests feel free to contact plznavigate@chromium.org.

BUG=475027
Committed: https://crrev.com/783dfd187b3459d85b07a0a84a4c955c72101468
Cr-Commit-Position: refs/heads/master@{#402792}

[carlosk, 2016-06-10 14:40:54]

Patchset #1 (id:1) has been deleted

[carlosk, 2016-06-10 14:42:54]

carlosk@chromium.org changed reviewers:
+ dpranke@chromium.org

[carlosk, 2016-06-10 14:42:54]

dpranke@ PTAL.

[Dirk Pranke, 2016-06-10 20:43:54]

You're tryiing to run this test suite as an additional step on the
linux_chromium_rel_ng builders, right?

If so, this isn't how you do that.

You need to add this as a step to the 'Linux Tests' entry instead. 'Browser Side
Navigation Linux' isn't a bot that exists on the chromium.linux waterfall, and I
don't think we need it to, since it's not compiling differently than the regular
builds (no different GN args, right?).

[carlosk, 2016-06-15 09:27:10]

On 2016/06/10 20:43:54, Dirk Pranke wrote:
> You're tryiing to run this test suite as an additional step on the
> linux_chromium_rel_ng builders, right?
> 
> If so, this isn't how you do that.
> 
> You need to add this as a step to the 'Linux Tests' entry instead. 'Browser
Side
> Navigation Linux' isn't a bot that exists on the chromium.linux waterfall, and
I
> don't think we need it to, since it's not compiling differently than the
regular
> builds (no different GN args, right?).

Done. But due to my ignorance of how these files are handled I wanted to be sure
it is not a problem to have two test suite runs labeled the same way:
'content_browsertests'. I noticed the site isolation run has a different name
for instance. Is that a problem in any way?

[Dirk Pranke, 2016-06-15 15:01:42]

On 2016/06/15 09:27:10, carlosk wrote:
> On 2016/06/10 20:43:54, Dirk Pranke wrote:
> > You're tryiing to run this test suite as an additional step on the
> > linux_chromium_rel_ng builders, right?
> > 
> > If so, this isn't how you do that.
> > 
> > You need to add this as a step to the 'Linux Tests' entry instead. 'Browser
> Side
> > Navigation Linux' isn't a bot that exists on the chromium.linux waterfall,
and
> I
> > don't think we need it to, since it's not compiling differently than the
> regular
> > builds (no different GN args, right?).
> 
> Done. But due to my ignorance of how these files are handled I wanted to be
sure
> it is not a problem to have two test suite runs labeled the same way:
> 'content_browsertests'. I noticed the site isolation run has a different name
> for instance. Is that a problem in any way?

Good question. Yes, it's a problem :).

Add a 'name' field to the dict to give the step a different name, 
like 'plz_navigate_content_browsertests' or something.

[carlosk, 2016-06-15 17:17:52]

On 2016/06/15 15:01:42, Dirk Pranke wrote:
> On 2016/06/15 09:27:10, carlosk wrote:
> > On 2016/06/10 20:43:54, Dirk Pranke wrote:
> > > You're tryiing to run this test suite as an additional step on the
> > > linux_chromium_rel_ng builders, right?
> > > 
> > > If so, this isn't how you do that.
> > > 
> > > You need to add this as a step to the 'Linux Tests' entry instead.
'Browser
> > Side
> > > Navigation Linux' isn't a bot that exists on the chromium.linux waterfall,
> and
> > I
> > > don't think we need it to, since it's not compiling differently than the
> > regular
> > > builds (no different GN args, right?).
> > 
> > Done. But due to my ignorance of how these files are handled I wanted to be
> sure
> > it is not a problem to have two test suite runs labeled the same way:
> > 'content_browsertests'. I noticed the site isolation run has a different
name
> > for instance. Is that a problem in any way?
> 
> Good question. Yes, it's a problem :).
> 
> Add a 'name' field to the dict to give the step a different name, 
> like 'plz_navigate_content_browsertests' or something.

Done that too and it solved the issue of duplicated test names from the try-bot
run of the previous patch set.

But now linux_chromium_rel_ng failed because of another issue. This is the error
log from [1]:

[0615/085716:ERROR:test_launcher.cc(826)] Failed to read the filter file.
Additional test environment:
    CHROME_DEVEL_SANDBOX=/opt/chromium/chrome_sandbox
    LANG=en_US.UTF-8
Command: ./content_browsertests --brave-new-test-launcher
--test-launcher-bot-mode --enable-browser-side-navigation
--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
--test-launcher-summary-output=/tmp/isolated_outrrHNCO/output.json

Based on another reference to a filter file for cast related test, this one
seems correct. Also, the file does exist in that directory! :)

I'm re-running the bot again to make sure this was not a fluke but otherwise I'm
don't understand what the problem could be.

[1] https://chromium-swarm.appspot.com/user/task/2f6d9903cd64d910

[Dirk Pranke, 2016-06-16 00:48:01]

On 2016/06/15 17:17:52, carlosk wrote:
> On 2016/06/15 15:01:42, Dirk Pranke wrote:
> > On 2016/06/15 09:27:10, carlosk wrote:
> > > On 2016/06/10 20:43:54, Dirk Pranke wrote:
> > > > You're tryiing to run this test suite as an additional step on the
> > > > linux_chromium_rel_ng builders, right?
> > > > 
> > > > If so, this isn't how you do that.
> > > > 
> > > > You need to add this as a step to the 'Linux Tests' entry instead.
> 'Browser
> > > Side
> > > > Navigation Linux' isn't a bot that exists on the chromium.linux
waterfall,
> > and
> > > I
> > > > don't think we need it to, since it's not compiling differently than the
> > > regular
> > > > builds (no different GN args, right?).
> > > 
> > > Done. But due to my ignorance of how these files are handled I wanted to
be
> > sure
> > > it is not a problem to have two test suite runs labeled the same way:
> > > 'content_browsertests'. I noticed the site isolation run has a different
> name
> > > for instance. Is that a problem in any way?
> > 
> > Good question. Yes, it's a problem :).
> > 
> > Add a 'name' field to the dict to give the step a different name, 
> > like 'plz_navigate_content_browsertests' or something.
> 
> Done that too and it solved the issue of duplicated test names from the
try-bot
> run of the previous patch set.
> 
> But now linux_chromium_rel_ng failed because of another issue. This is the
error
> log from [1]:
> 
> [0615/085716:ERROR:test_launcher.cc(826)] Failed to read the filter file.
> Additional test environment:
>     CHROME_DEVEL_SANDBOX=/opt/chromium/chrome_sandbox
>     LANG=en_US.UTF-8
> Command: ./content_browsertests --brave-new-test-launcher
> --test-launcher-bot-mode --enable-browser-side-navigation
>
--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
> --test-launcher-summary-output=/tmp/isolated_outrrHNCO/output.json
> 
> Based on another reference to a filter file for cast related test, this one
> seems correct. Also, the file does exist in that directory! :)
> 
> I'm re-running the bot again to make sure this was not a fluke but otherwise
I'm
> don't understand what the problem could be.
> 
> [1] https://chromium-swarm.appspot.com/user/task/2f6d9903cd64d910

The problem is that the filter file is not declared as a "data" dependency of
the content_browsertests
target, and so it's not included in the isolate that is sent to the swarming
job.

You need to add a block of code like the block on 

https://cs.chromium.org/chromium/src/content/test/BUILD.gn?rcl=0&l=383

but add it to the data variable for content_browsertests itself, e.g.:

https://cs.chromium.org/chromium/src/content/test/BUILD.gn?rcl=0&l=426

You don't need to create a separate GN target like the site_isolation target.

[carlosk, 2016-06-23 09:20:49]

Patchset #4 (id:80001) has been deleted

[carlosk, 2016-06-23 09:28:54]

On 2016/06/16 00:48:01, Dirk Pranke wrote:
> On 2016/06/15 17:17:52, carlosk wrote:
> > On 2016/06/15 15:01:42, Dirk Pranke wrote:
> > > On 2016/06/15 09:27:10, carlosk wrote:
> > > > On 2016/06/10 20:43:54, Dirk Pranke wrote:
> > > > > You're tryiing to run this test suite as an additional step on the
> > > > > linux_chromium_rel_ng builders, right?
> > > > > 
> > > > > If so, this isn't how you do that.
> > > > > 
> > > > > You need to add this as a step to the 'Linux Tests' entry instead.
> > 'Browser
> > > > Side
> > > > > Navigation Linux' isn't a bot that exists on the chromium.linux
> waterfall,
> > > and
> > > > I
> > > > > don't think we need it to, since it's not compiling differently than
the
> > > > regular
> > > > > builds (no different GN args, right?).
> > > > 
> > > > Done. But due to my ignorance of how these files are handled I wanted to
> be
> > > sure
> > > > it is not a problem to have two test suite runs labeled the same way:
> > > > 'content_browsertests'. I noticed the site isolation run has a different
> > name
> > > > for instance. Is that a problem in any way?
> > > 
> > > Good question. Yes, it's a problem :).
> > > 
> > > Add a 'name' field to the dict to give the step a different name, 
> > > like 'plz_navigate_content_browsertests' or something.
> > 
> > Done that too and it solved the issue of duplicated test names from the
> try-bot
> > run of the previous patch set.
> > 
> > But now linux_chromium_rel_ng failed because of another issue. This is the
> error
> > log from [1]:
> > 
> > [0615/085716:ERROR:test_launcher.cc(826)] Failed to read the filter file.
> > Additional test environment:
> >     CHROME_DEVEL_SANDBOX=/opt/chromium/chrome_sandbox
> >     LANG=en_US.UTF-8
> > Command: ./content_browsertests --brave-new-test-launcher
> > --test-launcher-bot-mode --enable-browser-side-navigation
> >
>
--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
> > --test-launcher-summary-output=/tmp/isolated_outrrHNCO/output.json
> > 
> > Based on another reference to a filter file for cast related test, this one
> > seems correct. Also, the file does exist in that directory! :)
> > 
> > I'm re-running the bot again to make sure this was not a fluke but otherwise
> I'm
> > don't understand what the problem could be.
> > 
> > [1] https://chromium-swarm.appspot.com/user/task/2f6d9903cd64d910
> 
> The problem is that the filter file is not declared as a "data" dependency of
> the content_browsertests
> target, and so it's not included in the isolate that is sent to the swarming
> job.
> 
> You need to add a block of code like the block on 
> 
> https://cs.chromium.org/chromium/src/content/test/BUILD.gn?rcl=0&l=383
> 
> but add it to the data variable for content_browsertests itself, e.g.:
> 
> https://cs.chromium.org/chromium/src/content/test/BUILD.gn?rcl=0&l=426
> 
> You don't need to create a separate GN target like the site_isolation target.

Following your instructions I modified the BUILD.gn file but I'm still getting
the same error. Am I still missing something?

[Dirk Pranke, 2016-06-24 01:57:44]

Here's my guess at what's wrong.

https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
File testing/buildbot/chromium.linux.json (right):

https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
testing/buildbot/chromium.linux.json:1062:
"--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
I think this path is wrong. When you run binaries under swarming, the current
working directory is out/Release, and so this needs to be
"--test-launcher-filter-file=../../testing/buildbot/etc".

[carlosk, 2016-06-24 12:40:21]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org

[carlosk, 2016-06-24 12:40:22]

+clamy@ for BUILD.gn review.

Your suggestion worked dpranke@. I also updated the test filter to include the
latest round of added tests that broke with PlzNavigate enabled.

https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
File testing/buildbot/chromium.linux.json (right):

https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
testing/buildbot/chromium.linux.json:1062:
"--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
On 2016/06/24 01:57:44, Dirk Pranke wrote:
> I think this path is wrong. When you run binaries under swarming, the current
> working directory is out/Release, and so this needs to be
> "--test-launcher-filter-file=../../testing/buildbot/etc".

Indeed that worked.

Now I'm puzzled: there is a similar reference to a cast filter file at line 845,
also for a swarming enabled content_browsertests run, and it uses that same
initial path structure. How come that one works?

[Dirk Pranke, 2016-06-24 23:37:27]

https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
File testing/buildbot/chromium.linux.json (right):

https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
testing/buildbot/chromium.linux.json:1062:
"--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
On 2016/06/24 12:40:21, carlosk wrote:
> On 2016/06/24 01:57:44, Dirk Pranke wrote:
> > I think this path is wrong. When you run binaries under swarming, the
current
> > working directory is out/Release, and so this needs to be
> > "--test-launcher-filter-file=../../testing/buildbot/etc".
> 
> Indeed that worked.
> 
> Now I'm puzzled: there is a similar reference to a cast filter file at line
845,
> also for a swarming enabled content_browsertests run, and it uses that same
> initial path structure. How come that one works?

Despite the fact that it says that "can_use_on_swarming_builders": true in that
entry,
the test isn't actually being run under swarming. When it's run locally, the CWD
is the build directory,
and so that works. And, yes, it's really a flaw that we use different CWDs in
the two scenarios because
it'll break things like this.

[carlosk, 2016-06-27 09:30:12]

On 2016/06/24 23:37:27, Dirk Pranke wrote:
>
https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
> File testing/buildbot/chromium.linux.json (right):
> 
>
https://codereview.chromium.org/2058743003/diff/100001/testing/buildbot/chrom...
> testing/buildbot/chromium.linux.json:1062:
>
"--test-launcher-filter-file=src/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
> On 2016/06/24 12:40:21, carlosk wrote:
> > On 2016/06/24 01:57:44, Dirk Pranke wrote:
> > > I think this path is wrong. When you run binaries under swarming, the
> current
> > > working directory is out/Release, and so this needs to be
> > > "--test-launcher-filter-file=../../testing/buildbot/etc".
> > 
> > Indeed that worked.
> > 
> > Now I'm puzzled: there is a similar reference to a cast filter file at line
> 845,
> > also for a swarming enabled content_browsertests run, and it uses that same
> > initial path structure. How come that one works?
> 
> Despite the fact that it says that "can_use_on_swarming_builders": true in
that
> entry,
> the test isn't actually being run under swarming. When it's run locally, the
CWD
> is the build directory,
> and so that works. And, yes, it's really a flaw that we use different CWDs in
> the two scenarios because
> it'll break things like this.

Got it. But is this LGTM as is now for the testing/**?

[clamy, 2016-06-27 12:45:12]

https://codereview.chromium.org/2058743003/diff/180001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2058743003/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:424: data += [
"//testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
]
This is different from what is done for site-isolation. Shouldn't we follow
what's being done there?

[carlosk, 2016-06-27 13:27:35]

Thanks.

https://codereview.chromium.org/2058743003/diff/180001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2058743003/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:424: data += [
"//testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
]
On 2016/06/27 12:45:12, clamy wrote:
> This is different from what is done for site-isolation. Shouldn't we follow
> what's being done there?

See comment #8 where dpranke@ suggests to not do that. He says creating a new
build target would be unnecessary.

[clamy, 2016-06-27 13:29:30]

lgtm

https://codereview.chromium.org/2058743003/diff/180001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2058743003/diff/180001/content/test/BUILD.gn#...
content/test/BUILD.gn:424: data += [
"//testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter"
]
On 2016/06/27 13:27:35, carlosk wrote:
> On 2016/06/27 12:45:12, clamy wrote:
> > This is different from what is done for site-isolation. Shouldn't we follow
> > what's being done there?
> 
> See comment #8 where dpranke@ suggests to not do that. He says creating a new
> build target would be unnecessary.

Acknowledged.

[Dirk Pranke, 2016-06-28 16:16:11]

lgtm

[clamy, 2016-06-29 11:04:27]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2016-06-29 11:04:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 13:56:56]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-06-29 13:59:46]

Description was changed from

==========
PlzNavigate: adds Linux runs of content_browsertests to the CQ.

Adding Linux runs of content_browsertests with PlzNavigate enabled to the
CQ. Note that it is still using an exclusion filter for the bits of content/
that PlzNavigate is not yet supporting.

If you need assistance getting your change deployed because of
these tests feel free to contact plznavigate@chromium.org.

BUG=475027
==========

to

==========
PlzNavigate: adds Linux runs of content_browsertests to the CQ.

Adding Linux runs of content_browsertests with PlzNavigate enabled to the
CQ. Note that it is still using an exclusion filter for the bits of content/
that PlzNavigate is not yet supporting.

If you need assistance getting your change deployed because of
these tests feel free to contact plznavigate@chromium.org.

BUG=475027

Committed: https://crrev.com/783dfd187b3459d85b07a0a84a4c955c72101468
Cr-Commit-Position: refs/heads/master@{#402792}
==========

[commit-bot: I haz the power, 2016-06-29 13:59:48]

Patchset 8 (id:??) landed as
https://crrev.com/783dfd187b3459d85b07a0a84a4c955c72101468
Cr-Commit-Position: refs/heads/master@{#402792}