headless: Don't run tests during the build

headless: Don't run tests during the build

BUG=595353
Committed: https://crrev.com/1ec102e1cf80a14aee8c6f7892fa3e517f338dcb
Cr-Commit-Position: refs/heads/master@{#424944}

[Sami, 2016-10-11 00:12:40]

Description was changed from

==========
headless: Don't run tests during the build

BUG=595353
==========

to

==========
headless: Don't run tests during the build

BUG=595353
==========

[Sami, 2016-10-11 00:12:40]

skyostil@chromium.org changed reviewers:
+ altimin@chromium.org

[Nico, 2016-10-11 00:18:21]

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

[Nico, 2016-10-11 00:18:21]

re "GN has no built-in support for non-c++ tests" – it doesn't have to have
that, GN only describes how to build stuff. Python tests don't need to be built,
they just are. (Doing `ninja -C out/gn base_unittests` doesn't run
base_unittests, it just builds them.)

And the system responsible for running tests does support running python tests
as far as I know, see eg here:
https://cs.chromium.org/chromium/src/testing/buildbot/chromium.fyi.json?rcl=0...

https://codereview.chromium.org/2411563002/diff/1/headless/BUILD.gn
File headless/BUILD.gn (left):

https://codereview.chromium.org/2411563002/diff/1/headless/BUILD.gn#oldcode258
headless/BUILD.gn:258: ":client_api_generator_tests",
I don't think this helps – client_api_generator_tests is still part of the 'all'
target.

[Sami, 2016-10-11 00:31:03]

skyostil@chromium.org changed reviewers:
+ perezju@chromium.org

[Sami, 2016-10-11 00:31:37]

Juan, can you check chromium.fyi.json makes sense? I'm not sure why none of the
other scripts have full paths there.

https://codereview.chromium.org/2411563002/diff/1/headless/BUILD.gn
File headless/BUILD.gn (left):

https://codereview.chromium.org/2411563002/diff/1/headless/BUILD.gn#oldcode258
headless/BUILD.gn:258: ":client_api_generator_tests",
On 2016/10/11 00:18:21, Nico wrote:
> I don't think this helps – client_api_generator_tests is still part of
the 'all'
> target.

Okay, let's get rid of the entire target then.

By the way, this was cribbed off of here which presumably has the same problem:
https://cs.chromium.org/chromium/src/base/android/jni_generator/BUILD.gn?rcl=...

[Nico, 2016-10-11 00:43:14]

On 2016/10/11 00:31:37, Sami (slow -- traveling) wrote:
> Juan, can you check chromium.fyi.json makes sense? I'm not sure why none of
the
> other scripts have full paths there.

That file is for FYI bots. There are other json files in that directory for main
waterfall bots.

> 
> https://codereview.chromium.org/2411563002/diff/1/headless/BUILD.gn
> File headless/BUILD.gn (left):
> 
> https://codereview.chromium.org/2411563002/diff/1/headless/BUILD.gn#oldcode258
> headless/BUILD.gn:258: ":client_api_generator_tests",
> On 2016/10/11 00:18:21, Nico wrote:
> > I don't think this helps – client_api_generator_tests is still part of
> the 'all'
> > target.
> 
> Okay, let's get rid of the entire target then.
> 
> By the way, this was cribbed off of here which presumably has the same
problem:
>
https://cs.chromium.org/chromium/src/base/android/jni_generator/BUILD.gn?rcl=...

[Sami, 2016-10-11 00:49:53]

On 2016/10/11 00:43:14, Nico wrote:
> On 2016/10/11 00:31:37, Sami (slow -- traveling) wrote:
> > Juan, can you check chromium.fyi.json makes sense? I'm not sure why none of
> the
> > other scripts have full paths there.
> 
> That file is for FYI bots. There are other json files in that directory for
main
> waterfall bots.

Headless is on the way to the main waterfall but currently it's still just an
FYI bot.

[perezju, 2016-10-11 10:25:00]

perezju@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[perezju, 2016-10-11 10:25:01]

https://codereview.chromium.org/2411563002/diff/50001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/2411563002/diff/50001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:9718: "script":
"headless/lib/browser/client_api_generator_unittest.py"
It looks like other scripts are given relative to src/testing/scripts/, and many
are just wrappers around the actual test scripts, see:

https://chromium.googlesource.com/chromium/src/+/master/testing/scripts

+phajdan who should know more about these.

[Paweł Hajdan Jr., 2016-10-11 18:10:00]

phajdan.jr@chromium.org changed reviewers:
+ dpranke@chromium.org

[Paweł Hajdan Jr., 2016-10-11 18:10:01]

https://codereview.chromium.org/2411563002/diff/50001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/2411563002/diff/50001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:9718: "script":
"headless/lib/browser/client_api_generator_unittest.py"
On 2016/10/11 at 10:25:01, perezju wrote:
> It looks like other scripts are given relative to src/testing/scripts/, and
many are just wrappers around the actual test scripts, see:
> 
> https://chromium.googlesource.com/chromium/src/+/master/testing/scripts
> 
> +phajdan who should know more about these.

Yup. Not only they need to be there, but also there's specific protocol for
this. See existing examples and let me know if you have any questions.

My general design suggestion would be to write generic python unit test script,
which takes specific file or directory as argument.

Dirk may have further suggestions, e.g. related to his typ test harness.

[Dirk Pranke, 2016-10-11 18:23:28]

https://codereview.chromium.org/2411563002/diff/50001/testing/buildbot/chromi...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/2411563002/diff/50001/testing/buildbot/chromi...
testing/buildbot/chromium.fyi.json:9718: "script":
"headless/lib/browser/client_api_generator_unittest.py"
On 2016/10/11 18:10:01, Paweł Hajdan Jr. wrote:
> On 2016/10/11 at 10:25:01, perezju wrote:
> > It looks like other scripts are given relative to src/testing/scripts/, and
> many are just wrappers around the actual test scripts, see:
> > 
> > https://chromium.googlesource.com/chromium/src/+/master/testing/scripts
> > 
> > +phajdan who should know more about these.
> 
> Yup. Not only they need to be there, but also there's specific protocol for
> this. See existing examples and let me know if you have any questions.
> 
> My general design suggestion would be to write generic python unit test
script,
> which takes specific file or directory as argument.
> 
> Dirk may have further suggestions, e.g. related to his typ test harness.

You should look at the scripts in //testing/scripts (like telemetry_unittests.py
or webkit_unittests.py), and, yes, using //third_party/typ will probably
simplify this. 

We don't currently have a single wrapper script. It's possible that we could
write one; I'm not sure if the script would end up growing too many arguments to
be worth it, though.

[Sami, 2016-10-13 02:00:00]

Thanks for the typ tip. linux_chromium_headless_dbg seems happy with the patch
now, PTAL.

[Dirk Pranke, 2016-10-13 02:16:43]

lgtm

[Sami, 2016-10-13 02:20:37]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-10-13 02:21:00]

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-10-13 03:12:17]

Description was changed from

==========
headless: Don't run tests during the build

BUG=595353
==========

to

==========
headless: Don't run tests during the build

BUG=595353
==========

[commit-bot: I haz the power, 2016-10-13 03:12:18]

Committed patchset #8 (id:120001)

[commit-bot: I haz the power, 2016-10-13 03:14:42]

Description was changed from

==========
headless: Don't run tests during the build

BUG=595353
==========

to

==========
headless: Don't run tests during the build

BUG=595353

Committed: https://crrev.com/1ec102e1cf80a14aee8c6f7892fa3e517f338dcb
Cr-Commit-Position: refs/heads/master@{#424944}
==========

[commit-bot: I haz the power, 2016-10-13 03:14:43]

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