Refactor MainHook into TestSuite.

Invoke the iOS hook from TestSuite so each run_all_unittests.cc file does not
have to remember to install MainHook.


BUG=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188376

[lliabraa, 2013-02-25 18:56:44]

This is required for these tests to run on iOS devices and should be a no-op for
other platforms.

[Paweł Hajdan Jr., 2013-02-25 19:58:16]

Oops, I think it'd help if I had a chance to review
https://chromiumcodereview.appspot.com/10690161 in the first place.

I'm sorry this will possibly have a bigger scope than expected.

This design seems fragile. base::TestSuite also has access to argc and argv -
are there any reasons you can't hook there?

[lliabraa, 2013-02-26 12:16:51]

On 2013/02/25 19:58:16, Paweł Hajdan Jr. wrote:
> Oops, I think it'd help if I had a chance to review
> https://chromiumcodereview.appspot.com/10690161 in the first place.
> 
> I'm sorry this will possibly have a bigger scope than expected.
> 
> This design seems fragile. base::TestSuite also has access to argc and argv -
> are there any reasons you can't hook there?

Can you please elaborate on your concern?

[Paweł Hajdan Jr., 2013-02-26 17:18:42]

On 2013/02/26 12:16:51, Lane LiaBraaten wrote:
> On 2013/02/25 19:58:16, Paweł Hajdan Jr. wrote:
> > Oops, I think it'd help if I had a chance to review
> > https://chromiumcodereview.appspot.com/10690161 in the first place.
> > 
> > I'm sorry this will possibly have a bigger scope than expected.
> > 
> > This design seems fragile. base::TestSuite also has access to argc and argv
-
> > are there any reasons you can't hook there?
> 
> Can you please elaborate on your concern?

content/test/run_all_unittests.cc is not the only file that would need this
modification. There are so many others. However, they all share base::TestSuite,
so this is the most maintainable place to put the shared logic, that doesn't
require adjusting all the call sites.

I hope I don't need to mention that when people _forget_ to install the MainHook
where it's needed things will be broken. Some test binaries having it and some
not having it is a huge mess.

[lliabraa, 2013-02-26 18:36:44]

On 2013/02/26 17:18:42, Paweł Hajdan Jr. wrote:
> On 2013/02/26 12:16:51, Lane LiaBraaten wrote:
> > On 2013/02/25 19:58:16, Paweł Hajdan Jr. wrote:
> > > Oops, I think it'd help if I had a chance to review
> > > https://chromiumcodereview.appspot.com/10690161 in the first place.
> > > 
> > > I'm sorry this will possibly have a bigger scope than expected.
> > > 
> > > This design seems fragile. base::TestSuite also has access to argc and
argv
> -
> > > are there any reasons you can't hook there?
> > 
> > Can you please elaborate on your concern?
> 
> content/test/run_all_unittests.cc is not the only file that would need this
> modification. There are so many others. However, they all share
base::TestSuite,
> so this is the most maintainable place to put the shared logic, that doesn't
> require adjusting all the call sites.
> 
> I hope I don't need to mention that when people _forget_ to install the
MainHook
> where it's needed things will be broken. Some test binaries having it and some
> not having it is a huge mess.

Thanks for the clarification - I understand what you're saying.

Before I go and refactor how this whole thing works I want to make sure you
understand that this hook is only used for iOS (main_hook.cc is an empty
implementation) and that all the test suites that are run for iOS already use
main hook. (i.e. content_unittests was a place where people forgot to install
the MainHook, and it's not a huge mess--just a two-line CL).

[Paweł Hajdan Jr., 2013-02-26 19:56:39]

On 2013/02/26 18:36:44, Lane LiaBraaten wrote:
> Before I go and refactor how this whole thing works I want to make sure you
> understand that this hook is only used for iOS (main_hook.cc is an empty
> implementation)

Thanks, I know. An OS-specific design issue is still an issue.

> and that all the test suites that are run for iOS already use
> main hook. (i.e. content_unittests was a place where people forgot to install
> the MainHook, and it's not a huge mess--just a two-line CL).

I know. With the mess I was referring not to a two-line CL, but to the fact that
you'd need to make a two-line CL like this for every new target for iOS. Or
people who do not know that can run into mysterious issues.

[lliabraa, 2013-02-27 15:27:19]

@pawel - I've refactored the iOS hook into TestSuite - is this what you had in
mind?

@rohitrao, tvl - I'd like to get a few more eyes from iOS on this change.

[rohitrao (ping after 24h), 2013-02-27 15:30:49]

+Mark for thoughts, since he was on the original MainHook reviews.

Any objections to moving the hook into base::TestSuite?

[Mark Mentovai, 2013-02-27 15:38:58]

In principle, this is almost exactly the same. I don’t care where it lives. This
is fine.

[TVL, 2013-02-27 15:59:35]

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/base.gyp
File base/base.gyp (left):

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/base.gyp#oldc...
base/base.gyp:815: 'test/main_hook.cc',
I don't see this being deleted in the cl

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/test/test_sui...
File base/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/test/test_sui...
base/test/test_suite.cc:41: #include "base/test/test_support_ios.h"
since you already have this file, does it make more sense to put the support in
there stead of the new file?

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/test/test_sui...
base/test/test_suite.cc:153: // normally, but in the context of the iOS app.
I'd probably just put this comment on the RunTestsFromApp method, this file is
already full of ifdefs and hard to read, so the comment doesn't seem like it's
helping much.

[lliabraa, 2013-02-27 20:31:31]

@tvl - thanks for the review

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/base.gyp
File base/base.gyp (left):

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/base.gyp#oldc...
base/base.gyp:815: 'test/main_hook.cc',
On 2013/02/27 15:59:35, TVL wrote:
> I don't see this being deleted in the cl

Done.

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/test/test_sui...
File base/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/test/test_sui...
base/test/test_suite.cc:41: #include "base/test/test_support_ios.h"
On 2013/02/27 15:59:35, TVL wrote:
> since you already have this file, does it make more sense to put the support
in
> there stead of the new file?

Done.

https://chromiumcodereview.appspot.com/12321117/diff/14001/base/test/test_sui...
base/test/test_suite.cc:153: // normally, but in the context of the iOS app.
On 2013/02/27 15:59:35, TVL wrote:
> I'd probably just put this comment on the RunTestsFromApp method, this file is
> already full of ifdefs and hard to read, so the comment doesn't seem like it's
> helping much.

Done.

[Paweł Hajdan Jr., 2013-03-01 16:42:32]

LGTM

Great to see that it was possible to use base::TestSuite. Thank you for making
that work.

[commit-bot: I haz the power, 2013-03-07 00:03:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lliabraa@chromium.org/12321117/7003

[commit-bot: I haz the power, 2013-03-07 00:03:26]

Presubmit check for 12321117-7003 failed and returned exit status 1.

INFO:root:Found 12 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/crypto/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    media/base/run_all_unittests.cc
    net/base/run_all_unittests.cc
    crypto/run_all_unittests.cc
    ui/test/run_all_unittests.cc
    base/base.gyp
    sql/run_all_unittests.cc

Presubmit checks took 2.9s to calculate.

[lliabraa, 2013-03-14 17:56:46]

Adding various OWNERS to reviewers. If you are in the following list, you LGTM'd
the CL that added MainHook to the run_all_unittests.cc file in your directory.
Please review this CL that removes MainHook.

fischman@ for media/OWNERS
wtc for net/OWNERS, crypto/OWNERS
thakis for ui/OWNERS
mark for base/OWNERS
shess for sql/OWNERS

[Mark Mentovai, 2013-03-14 18:01:22]

I already looked at this a couple of weeks ago. It’s fine. LGTM.

[Scott Hess - ex-Googler, 2013-03-14 18:01:37]

lgtm

[wtc, 2013-03-14 18:37:48]

Patch set 5 LGTM. I suggest two small changes.

https://chromiumcodereview.appspot.com/12321117/diff/7003/crypto/run_all_unit...
File crypto/run_all_unittests.cc (right):

https://chromiumcodereview.appspot.com/12321117/diff/7003/crypto/run_all_unit...
crypto/run_all_unittests.cc:9: 

Nit: delete this blank line.

https://chromiumcodereview.appspot.com/12321117/diff/7003/net/base/run_all_un...
File net/base/run_all_unittests.cc (right):

https://chromiumcodereview.appspot.com/12321117/diff/7003/net/base/run_all_un...
net/base/run_all_unittests.cc:26: 

Nit: delete this blank line.

[Ami GONE FROM CHROMIUM, 2013-03-14 19:01:10]

LGTM

[Nico, 2013-03-14 19:45:30]

ui/ lgtm

[lliabraa, 2013-03-15 11:02:27]

Thanks for the reviews!

https://chromiumcodereview.appspot.com/12321117/diff/7003/crypto/run_all_unit...
File crypto/run_all_unittests.cc (right):

https://chromiumcodereview.appspot.com/12321117/diff/7003/crypto/run_all_unit...
crypto/run_all_unittests.cc:9: 
On 2013/03/14 18:37:48, wtc wrote:
> 
> Nit: delete this blank line.

Done.

https://chromiumcodereview.appspot.com/12321117/diff/7003/net/base/run_all_un...
File net/base/run_all_unittests.cc (right):

https://chromiumcodereview.appspot.com/12321117/diff/7003/net/base/run_all_un...
net/base/run_all_unittests.cc:26: 
On 2013/03/14 18:37:48, wtc wrote:
> 
> Nit: delete this blank line.

Done.

[commit-bot: I haz the power, 2013-03-15 11:02:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lliabraa@chromium.org/12321117/27001

[commit-bot: I haz the power, 2013-03-15 13:14:04]

Change committed as 188376