Replace test_support_main by test_main and get rid of test_support_main_threaded_mac

Replace test_support_main by test_main and get rid of test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
NOTRY=True

Committed: https://crrev.com/26bddb92f0f76c08bd0dbf5efe4909128ef0a90b
Cr-Commit-Position: refs/heads/master@{#15332}

[ehmaldonado_webrtc, 2016-11-29 12:40:19]

The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 12:40:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-11-29 12:42:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 12:42:13]

Dry run: Try jobs failed on following builders:
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20481)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19197)
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/19808)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[ehmaldonado_webrtc, 2016-11-29 12:49:55]

Patchset #1 (id:1) has been deleted

[ehmaldonado_webrtc, 2016-11-29 12:50:18]

The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 12:50:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ehmaldonado_webrtc, 2016-11-29 12:59:45]

Was surprisingly easy

[commit-bot: I haz the power, 2016-11-29 13:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 13:04:43]

Dry run: This issue passed the CQ dry run.

[kjellander_webrtc, 2016-11-29 13:39:34]

I like the removal of test_support_main_threaded_mac but I don't see the point
of replacing test_support_main with test_main.

The more I look at it the more I think test_main (and the .cc file) should be
removed instead.

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc
File webrtc/test/test_suite.cc (left):

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc...
webrtc/test/test_suite.cc:22: DEFINE_bool(logs, false, "print logs to stderr");
test_main.cc is duplicating a lot but it doesn't have this flag.

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc...
webrtc/test/test_suite.cc:34: testing::InitGoogleMock(&argc, argv);  // Runs
InitGoogleTest() internally.
InitGoogleMock doesn't happen in test_main.cc either. Does really all tests pass
if that doesn't happen?

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc...
webrtc/test/test_suite.cc:55: rtc::LogMessage::SetLogToStderr(FLAGS_logs);
This is also not present in test_main.cc. Maybe it's that one that should be
removed in favor for this file instead?
test_suite.{cc,h} was inspired by Chromium initially, and have nice properties
like you can inherit and reuse it in different parts of our code base. That
probably never happened though, so I'm not necessary against removing it but I
don't think there's any need to remove it now (better clean up test_main.cc).

One example is the --logs flag: that's something only a few developers are using
for a few tests:
https://cs.chromium.org/search/?q=%22DEFINE_bool(logs%22&sq=package:chromium&...

[ehmaldonado_webrtc, 2016-11-29 14:04:51]

ehmaldonado@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[ehmaldonado_webrtc, 2016-11-29 14:04:52]

Patrik, what do you think?
Should we keep test_support_main and clean up test_main instead?

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc
File webrtc/test/test_suite.cc (left):

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc...
webrtc/test/test_suite.cc:22: DEFINE_bool(logs, false, "print logs to stderr");
On 2016/11/29 13:39:34, kjellander_webrtc wrote:
> test_main.cc is duplicating a lot but it doesn't have this flag.

Done.

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc...
webrtc/test/test_suite.cc:34: testing::InitGoogleMock(&argc, argv);  // Runs
InitGoogleTest() internally.
On 2016/11/29 13:39:34, kjellander_webrtc wrote:
> InitGoogleMock doesn't happen in test_main.cc either. Does really all tests
pass
> if that doesn't happen?

Yes, they do.

[ehmaldonado_webrtc, 2016-11-29 14:05:53]

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc
File webrtc/test/test_suite.cc (left):

https://codereview.webrtc.org/2540693002/diff/20001/webrtc/test/test_suite.cc...
webrtc/test/test_suite.cc:22: DEFINE_bool(logs, false, "print logs to stderr");
On 2016/11/29 14:04:52, ehmaldonado_webrtc wrote:
> On 2016/11/29 13:39:34, kjellander_webrtc wrote:
> > test_main.cc is duplicating a lot but it doesn't have this flag.
> 
> Done.

Well, I was going to upload a patch adding these flags to test_main.cc.
So, no done :P

[phoglund, 2016-11-29 14:23:39]

lgtm

[phoglund, 2016-11-29 14:27:47]

I don't know which is better out of test_support_main or test_main, but I think
we should choose one of them? They do the same thing right?

[ehmaldonado_webrtc, 2016-11-29 14:29:16]

On 2016/11/29 14:27:47, phoglund wrote:
> I don't know which is better out of test_support_main or test_main, but I
think
> we should choose one of them? They do the same thing right?

Yeah, basically.

[phoglund, 2016-11-29 14:30:36]

Ok, so run_all_unittests.cc + TestSuite effectively does the same thing as
test_main.cc. I think it's a wash which one to use; maybe leaning towards
test_main because of its simplicity.

[phoglund, 2016-11-29 14:32:20]

On 2016/11/29 14:30:36, phoglund wrote:
> Ok, so run_all_unittests.cc + TestSuite effectively does the same thing as
> test_main.cc. I think it's a wash which one to use; maybe leaning towards
> test_main because of its simplicity.

... But maybe add in the missing things Henrik mentioned above. I buy the
argument that it's nice to have an overridable test suite, but it's just
speculative generality since no one uses it at this point. I'd argue the
simplicity of test_main.cc is preferable then.

[ehmaldonado_webrtc, 2016-11-29 14:33:17]

On 2016/11/29 14:32:20, phoglund wrote:
> On 2016/11/29 14:30:36, phoglund wrote:
> > Ok, so run_all_unittests.cc + TestSuite effectively does the same thing as
> > test_main.cc. I think it's a wash which one to use; maybe leaning towards
> > test_main because of its simplicity.
> 
> ... But maybe add in the missing things Henrik mentioned above. I buy the
> argument that it's nice to have an overridable test suite, but it's just
> speculative generality since no one uses it at this point. I'd argue the
> simplicity of test_main.cc is preferable then.

SGTM :)

[kjellander_webrtc, 2016-11-29 14:34:51]

On 2016/11/29 14:33:17, ehmaldonado_webrtc wrote:
> On 2016/11/29 14:32:20, phoglund wrote:
> > On 2016/11/29 14:30:36, phoglund wrote:
> > > Ok, so run_all_unittests.cc + TestSuite effectively does the same thing as
> > > test_main.cc. I think it's a wash which one to use; maybe leaning towards
> > > test_main because of its simplicity.
> > 
> > ... But maybe add in the missing things Henrik mentioned above. I buy the
> > argument that it's nice to have an overridable test suite, but it's just
> > speculative generality since no one uses it at this point. I'd argue the
> > simplicity of test_main.cc is preferable then.
> 
> SGTM :)

Then let's go with test_main only. Make sure to bring it up to compatibility.
I'm still annoyed that all tests pass without Gmock initialization. Or does no
tests use gmock properly?

[ehmaldonado_webrtc, 2016-11-29 15:22:24]

On 2016/11/29 14:34:51, kjellander_webrtc wrote:
> On 2016/11/29 14:33:17, ehmaldonado_webrtc wrote:
> > On 2016/11/29 14:32:20, phoglund wrote:
> > > On 2016/11/29 14:30:36, phoglund wrote:
> > > > Ok, so run_all_unittests.cc + TestSuite effectively does the same thing
as
> > > > test_main.cc. I think it's a wash which one to use; maybe leaning
towards
> > > > test_main because of its simplicity.
> > > 
> > > ... But maybe add in the missing things Henrik mentioned above. I buy the
> > > argument that it's nice to have an overridable test suite, but it's just
> > > speculative generality since no one uses it at this point. I'd argue the
> > > simplicity of test_main.cc is preferable then.
> > 
> > SGTM :)
> 
> Then let's go with test_main only. Make sure to bring it up to compatibility.
> I'm still annoyed that all tests pass without Gmock initialization. Or does no
> tests use gmock properly?

I used InitGoogleMock just to be sure.
Is there anything left missing?

[kjellander_webrtc, 2016-11-30 06:29:44]

Two things left to fix.

https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc
File webrtc/test/test_main.cc (left):

https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc#...
webrtc/test/test_main.cc:33: google::AllowCommandLineReparsing();
We need to keep this for the moment. And when it's going to be removed it should
be done in a separate CL.
We should be able to do it soon since our use of mb.py has reduced the passing
of unnecessary flags to only be remaining for the perf tests.

https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc
File webrtc/test/test_main.cc (right):

https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc#...
webrtc/test/test_main.cc:34: rtc::LogMessage::SetLogToStderr(FLAGS_logs);
You should also add:
if (FLAGS_logs)
    trace_to_stderr_.reset(new TraceToStderr);

[ehmaldonado_webrtc, 2016-11-30 11:57:04]

Patchset #4 (id:80001) has been deleted

[ehmaldonado_webrtc, 2016-11-30 11:58:46]

On 2016/11/30 06:29:44, kjellander_webrtc wrote:
> Two things left to fix.
> 
> https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc
> File webrtc/test/test_main.cc (left):
> 
>
https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc#...
> webrtc/test/test_main.cc:33: google::AllowCommandLineReparsing();
> We need to keep this for the moment. And when it's going to be removed it
should
> be done in a separate CL.
> We should be able to do it soon since our use of mb.py has reduced the passing
> of unnecessary flags to only be remaining for the perf tests.
> 
> https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc
> File webrtc/test/test_main.cc (right):
> 
>
https://codereview.webrtc.org/2540693002/diff/60001/webrtc/test/test_main.cc#...
> webrtc/test/test_main.cc:34: rtc::LogMessage::SetLogToStderr(FLAGS_logs);
> You should also add:
> if (FLAGS_logs)
>     trace_to_stderr_.reset(new TraceToStderr);

PTAL

[ehmaldonado_webrtc, 2016-11-30 12:13:02]

The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 12:13:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kjellander_webrtc, 2016-11-30 12:24:09]

lgtm

[ehmaldonado_webrtc, 2016-11-30 12:27:04]

Description was changed from

==========
Replace test_support_main by test_main and get rid of
test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
==========

to

==========
Replace test_support_main by test_main and get rid of
test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
NOTRY=True
==========

[commit-bot: I haz the power, 2016-11-30 12:31:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 12:31:53]

Dry run: This issue passed the CQ dry run.

[phoglund, 2016-11-30 12:46:44]

lgtm

[ehmaldonado_webrtc, 2016-11-30 12:50:42]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2016-11-30 12:50:52]

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

[commit-bot: I haz the power, 2016-11-30 12:54:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 12:54:02]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10863)

[kjellander_webrtc, 2016-11-30 13:04:50]

On 2016/11/30 12:54:02, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10863)

You need more OWNERS approval

[ehmaldonado_webrtc, 2016-11-30 13:32:40]

ehmaldonado@webrtc.org changed reviewers:
+ henrika@webrtc.org

[ehmaldonado_webrtc, 2016-11-30 13:32:43]

henrika: Can you rubberstamp, please?

[henrika_webrtc, 2016-11-30 13:51:20]

LGTM

[ehmaldonado_webrtc, 2016-11-30 14:10:12]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2016-11-30 14:10:23]

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

[commit-bot: I haz the power, 2016-11-30 14:12:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480515012579760,
"parent_rev": "da883992eef935f9c4772e84fbeee430b2e8d401", "commit_rev":
"e8805c8be14155b752bbbbccb131eb836df2b1de"}

[commit-bot: I haz the power, 2016-11-30 14:12:05]

Description was changed from

==========
Replace test_support_main by test_main and get rid of
test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
NOTRY=True
==========

to

==========
Replace test_support_main by test_main and get rid of
test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
NOTRY=True
==========

[commit-bot: I haz the power, 2016-11-30 14:12:07]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-11-30 14:12:15]

Description was changed from

==========
Replace test_support_main by test_main and get rid of
test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
NOTRY=True
==========

to

==========
Replace test_support_main by test_main and get rid of
test_support_main_threaded_mac

test_support_main_threaded_mac doesn't seem to be used. It looks like it was
last used about a year and a half ago, and was removed in
https://webrtc-codereview.appspot.com/55379004

BUG=webrtc:6424
R=kjellander@webrtc.org
NOTRY=True

Committed: https://crrev.com/26bddb92f0f76c08bd0dbf5efe4909128ef0a90b
Cr-Commit-Position: refs/heads/master@{#15332}
==========

[commit-bot: I haz the power, 2016-11-30 14:12:16]

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

[nisse-webrtc, 2016-11-30 14:19:38]

On 2016/11/29 14:27:47, phoglund wrote:
> I don't know which is better out of test_support_main or test_main, but I
think
> we should choose one of them? They do the same thing right?

There's also base/unittest_main.cc, in the target rtc_base_tests_utils. No idea
what's using which main function.