Add a new 'run_all_unittests' target in base

Add a new 'run_all_unittests' target in base

Make all the places that include run_all_unittests.cc manually depend
on this target instead.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127911

[akalin, 2012-03-16 02:07:58]

+rsleevi for review

[Ryan Sleevi, 2012-03-16 02:45:25]

Not a base/ OWNER, but some GYP-y feedback.

Most of the changes look good, but I'm trying to grok a few of them, noted
below.

http://codereview.chromium.org/9691067/diff/1/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/9691067/diff/1/base/base.gyp#newcode149
base/base.gyp:149: 'test_support_base',
Could you explain the motivation for the export_dependent_settings? I can guess
at a few reasons, but it might be easier just to have you explain.

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:336: '../base/base.gyp:run_all_unittests',
And the motivation for this (and on 443)

[Ryan Sleevi, 2012-03-16 02:48:45]

This is what I get for only taking two passes through:

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi#oldcod...
chrome/chrome_tests.gypi:349: 'test/ui/run_all_unittests.cc',
This is a different test harness.

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi#oldcod...
chrome/chrome_tests.gypi:444: 'test/base/run_all_unittests.cc',
As is this

[akalin, 2012-03-16 06:59:20]

PTAL

http://codereview.chromium.org/9691067/diff/1/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/9691067/diff/1/base/base.gyp#newcode149
base/base.gyp:149: 'test_support_base',
On 2012/03/16 02:45:25, Ryan Sleevi wrote:
> Could you explain the motivation for the export_dependent_settings? I can
guess
> at a few reasons, but it might be easier just to have you explain.

Actually, come to think of it, I don't think it's needed, since one simply
includes run_all_unittests to get a 'main' symbol linked in.

I'm a bit fuzzy on when it's appropriate to use export_dependent_settings; does
that sound right?

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi#oldcod...
chrome/chrome_tests.gypi:349: 'test/ui/run_all_unittests.cc',
On 2012/03/16 02:48:45, Ryan Sleevi wrote:
> This is a different test harness.

Indeed!  reverted.

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi#oldcod...
chrome/chrome_tests.gypi:444: 'test/base/run_all_unittests.cc',
On 2012/03/16 02:48:45, Ryan Sleevi wrote:
> As is this

Reverted this too.

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/9691067/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:336: '../base/base.gyp:run_all_unittests',
On 2012/03/16 02:45:25, Ryan Sleevi wrote:
> And the motivation for this (and on 443)

rendered moot.

[Ryan Sleevi, 2012-03-16 17:27:30]

This looks right and good to me. You'll want to get a base/ OWNERS to stamp.

http://codereview.chromium.org/9691067/diff/7001/sync/sync.gyp
File sync/sync.gyp (left):

http://codereview.chromium.org/9691067/diff/7001/sync/sync.gyp#oldcode253
sync/sync.gyp:253: '../base/base.gyp:test_support_base',
Is this target really not needed anymore?

I'm assuming you're relying on the fact that targets which depend on this will
'probably' include ../base/base.gyp:run_all_unittests and pick up the
dependency?

Might want to double check the code within the 'sources' here to make sure it
doesn't really depend on test_support_base - otherwise/if that's too much work,
I'd suggest leaving this dependency here

Removing 261 is fine

[akalin, 2012-03-16 18:53:50]

+brettw for review

http://codereview.chromium.org/9691067/diff/7001/sync/sync.gyp
File sync/sync.gyp (left):

http://codereview.chromium.org/9691067/diff/7001/sync/sync.gyp#oldcode253
sync/sync.gyp:253: '../base/base.gyp:test_support_base',
On 2012/03/16 17:27:30, Ryan Sleevi wrote:
> Is this target really not needed anymore?
> 
> I'm assuming you're relying on the fact that targets which depend on this will
> 'probably' include ../base/base.gyp:run_all_unittests and pick up the
> dependency?
> 
> Might want to double check the code within the 'sources' here to make sure it
> doesn't really depend on test_support_base - otherwise/if that's too much
work,
> I'd suggest leaving this dependency here
> 
> Removing 261 is fine

Good point.  I audited all the targets and added back in the dep on
test_support_base when they actually do directly depend on it (usually
test_timeouts)

[brettw, 2012-03-16 22:46:43]

lgtm

[commit-bot: I haz the power, 2012-03-16 22:52:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9691067/13002

[commit-bot: I haz the power, 2012-03-16 22:52:48]

Presubmit check for 9691067-13002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    dbus
    cloud_print

[akalin, 2012-03-21 00:11:21]

+satorux for dbus/
+scottbyer for cloud_print/

rsleevi@, do I have a LGTM from you? (you said it looks good, but that doesn't
turn your message green :) )

[Ryan Sleevi, 2012-03-21 00:12:28]

On 2012/03/21 00:11:21, akalin wrote:
> +satorux for dbus/
> +scottbyer for cloud_print/
> 
> rsleevi@, do I have a LGTM from you? (you said it looks good, but that doesn't
> turn your message green :) )

Yes, that was a LGTM, but I didn't want to give you false hope with a green bar
when I am not an OWNER for any of the affected code ;-)

[Scott Byer, 2012-03-21 00:44:55]

lgtm

[satorux1, 2012-03-21 04:51:40]

LGTM. thank you for the cleanup

[commit-bot: I haz the power, 2012-03-21 04:56:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9691067/18001

[commit-bot: I haz the power, 2012-03-21 05:55:08]

Change committed as 127911