Make it possible to run gtests on iOS.

Make it possible to run gtests on iOS.

Adds MainHook object to run_all_unittests main() and run_all_perftest main(),  so tests run without beng killed by the OS. (If an application does not check in within a certain amount of time after launch, it will be killed.)  The object is empty for non-iOS.

Adds test_listener_ios to test_suite, which registers to be notified on test end. At test end, it runs the run loop to prevent the test run from being killed by the OS.

Disables two tests for iOS:  BooleanOperators and StringPattern.

BUG=b/6753976
TEST=None

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

[leng, 2012-07-12 09:03:43]

This requires changes to the PRESUBMIT.py script to be submitted.  Those changes
are in a separate CL:
https://chromiumcodereview.appspot.com/10695172/

[stuartmorgan, 2012-07-12 09:36:31]

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h
File base/test/main_hook.h (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:8: class MainHook {
Needs a class level comment (just explaining that it provides a way of running
code intended to be run before starting gtest-based tests, or something like
that).

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:9: public:
Indent 1

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:12: #if !defined(OS_IOS)
While it seems a bit silly, I think it would probably be better to make a
main_hook.cc with an #if !defined(OS_IO) implementation and get rid of the ugly
ifdef.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:17: private:
Indent 1

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:22: // (i.e.-Run All Tests)
Remove this last line since it's subject to becoming stale.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:27: // (unittest, etc.) into chunks via a timer so
the iOS eventloop can drive it.
I think this comment predates test_listener_ios; it seems out of date.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:66: [NSThread sleepUntilDate: [NSDate
dateWithTimeIntervalSinceNow:2.0]];
Remove the space after :

We should add a TODO: with a bug reference to store the
application:didFinishLaunching time, and then make the sleep here [some min
time] - [elapsed time].

It's not a priority, but it's worth doing at some point since it will make
running slower sets of unit tests a bit faster.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:83: static bool ran_hook;
 = false

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:91: NSAutoreleasePool * pool = [[NSAutoreleasePool
alloc] init];
We may as well use a scoped autorelease pool here.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
File base/test/test_listener_ios.h (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
base/test/test_listener_ios.h:8: #include "build/build_config.h"
No longer necessary.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
File base/test/test_listener_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
base/test/test_listener_ios.mm:9: #include <gtest/gtest.h>
This should be:

#import <Foundation/Foundation.h>

#include "base/mac/scoped_nsautorelease_pool.h"
#include "testing/gtest/include/gtest/gtest.h"

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_suite.c...
base/test/test_suite.cc:28: #if !defined(OS_IOS)
Reverse the order so it's note an if/else of a negative condition.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/trace_event_...
File base/test/trace_event_analyzer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/trace_event_...
base/test/trace_event_analyzer_unittest.cc:204: #if defined(OS_IOS)
Let's pull this file out of the CL; it's not conceptually related.

[leng, 2012-07-12 10:44:42]

Thanks, PTAL

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h
File base/test/main_hook.h (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:8: class MainHook {
On 2012/07/12 09:36:31, stuartmorgan wrote:
> Needs a class level comment (just explaining that it provides a way of running
> code intended to be run before starting gtest-based tests, or something like
> that).

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:9: public:
On 2012/07/12 09:36:31, stuartmorgan wrote:
> Indent 1

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:12: #if !defined(OS_IOS)
On 2012/07/12 09:36:31, stuartmorgan wrote:
> While it seems a bit silly, I think it would probably be better to make a
> main_hook.cc with an #if !defined(OS_IO) implementation and get rid of the
ugly
> ifdef.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook.h#...
base/test/main_hook.h:17: private:
On 2012/07/12 09:36:31, stuartmorgan wrote:
> Indent 1

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:22: // (i.e.-Run All Tests)
On 2012/07/12 09:36:31, stuartmorgan wrote:
> Remove this last line since it's subject to becoming stale.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:27: // (unittest, etc.) into chunks via a timer so
the iOS eventloop can drive it.
On 2012/07/12 09:36:31, stuartmorgan wrote:
> I think this comment predates test_listener_ios; it seems out of date.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:66: [NSThread sleepUntilDate: [NSDate
dateWithTimeIntervalSinceNow:2.0]];
On 2012/07/12 09:36:31, stuartmorgan wrote:
> Remove the space after :
> 
> We should add a TODO: with a bug reference to store the
> application:didFinishLaunching time, and then make the sleep here [some min
> time] - [elapsed time].
> 
> It's not a priority, but it's worth doing at some point since it will make
> running slower sets of unit tests a bit faster.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:83: static bool ran_hook;
On 2012/07/12 09:36:31, stuartmorgan wrote:
>  = false

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/main_hook_io...
base/test/main_hook_ios.mm:91: NSAutoreleasePool * pool = [[NSAutoreleasePool
alloc] init];
On 2012/07/12 09:36:31, stuartmorgan wrote:
> We may as well use a scoped autorelease pool here.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
File base/test/test_listener_ios.h (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
base/test/test_listener_ios.h:8: #include "build/build_config.h"
On 2012/07/12 09:36:31, stuartmorgan wrote:
> No longer necessary.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
File base/test/test_listener_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_listene...
base/test/test_listener_ios.mm:9: #include <gtest/gtest.h>
On 2012/07/12 09:36:31, stuartmorgan wrote:
> This should be:
> 
> #import <Foundation/Foundation.h>
> 
> #include "base/mac/scoped_nsautorelease_pool.h"
> #include "testing/gtest/include/gtest/gtest.h"

Done.

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/1/base/test/test_suite.c...
base/test/test_suite.cc:28: #if !defined(OS_IOS)
On 2012/07/12 09:36:31, stuartmorgan wrote:
> Reverse the order so it's note an if/else of a negative condition.

Done.

[stuartmorgan, 2012-07-12 10:59:23]

LGTM with nits.

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook.cc
File base/test/main_hook.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook.cc:9: }
Maybe move this up to the previous line (it's pretty standard in Chromium code
for empty methods).

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook.h
File base/test/main_hook.h (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook.h:14: private:
Blank line before private:

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook_ios.mm:11: #include "base/debug/debugger.h"
Move this up a line so the headers are in order.

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook_ios.mm:62: // TODO(ios): b/6810854 Figure out how much time
is actually needed.
Sorry, I meant a crbug.com bug; I should have said tha :( Can you close this one
and open one there?

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_list...
File base/test/test_listener_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_list...
base/test/test_listener_ios.mm:28: NSRunLoop* current_loop = [NSRunLoop
currentRunLoop];
Just inline this below.

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_suit...
File base/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_suit...
base/test/test_suite.cc:30: #else // !OS_IOS
Two spaces before //

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_suit...
base/test/test_suite.cc:32: #endif  // OS_IOS
I hate this not matching the comment on the section it's ending (the #else), but
I don't know what's less bad. Maybe just don't comment on the #else?

[leng, 2012-07-12 11:17:31]

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook.cc
File base/test/main_hook.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook.cc:9: }
On 2012/07/12 10:59:23, stuartmorgan wrote:
> Maybe move this up to the previous line (it's pretty standard in Chromium code
> for empty methods).

I couldn't find it in the style guides.  Thanks!

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook.h
File base/test/main_hook.h (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook.h:14: private:
On 2012/07/12 10:59:23, stuartmorgan wrote:
> Blank line before private:

Done.

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook_ios.mm:11: #include "base/debug/debugger.h"
On 2012/07/12 10:59:23, stuartmorgan wrote:
> Move this up a line so the headers are in order.

Two lines.  :)

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/main_hook...
base/test/main_hook_ios.mm:62: // TODO(ios): b/6810854 Figure out how much time
is actually needed.
On 2012/07/12 10:59:23, stuartmorgan wrote:
> Sorry, I meant a http://crbug.com bug; I should have said tha :( Can you close
this one
> and open one there?

Done.

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_list...
File base/test/test_listener_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_list...
base/test/test_listener_ios.mm:28: NSRunLoop* current_loop = [NSRunLoop
currentRunLoop];
On 2012/07/12 10:59:23, stuartmorgan wrote:
> Just inline this below.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_suit...
File base/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_suit...
base/test/test_suite.cc:30: #else // !OS_IOS
On 2012/07/12 10:59:23, stuartmorgan wrote:
> Two spaces before //

N/A

https://chromiumcodereview.appspot.com/10690161/diff/7002/base/test/test_suit...
base/test/test_suite.cc:32: #endif  // OS_IOS
On 2012/07/12 10:59:23, stuartmorgan wrote:
> I hate this not matching the comment on the section it's ending (the #else),
but
> I don't know what's less bad. Maybe just don't comment on the #else?

I find it more readable to not have a comment on the #else.

[Mark Mentovai, 2012-07-12 13:37:50]

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:16: // MainHook saves the chrome main() and calls
UIApplicationMain(),
Oh, patch-cest!

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:19: // notification is received, it fires main()
again to have the real work done.
Give an example of usage, showing that the MainHook object should be the first
thing in main (to avoid duplicating the stuff that happens before MainHook), and
that it should be instantiated with |main, argc, argv| as arguments to its
constructor.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:22: // window displaying the app name. If a bunch of
apps using this are being run
“using this” isn’t clear. You mean MainHook, but I thought you meant “the
window.” Say MainHook.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:40: // Yes, this is leaked, it's just to make what's
running visible.
You don’t have to leak it, and it’s easy enough to not leak it. Save it in the
object and get rid of it after g_main_func (or your sleep) in -runTests.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:63: [NSThread sleepUntilDate:[NSDate
dateWithTimeIntervalSinceNow:2.0]];
This also adds a delay to test apps that weren’t too fast. Is it better to take
a time since start (since -application:didFinishLaunchingWithOptions: was
called) to enforce some minimum time, but not cause already long-running tests
to run even longer?

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:69: [application
performSelector:@selector(_terminateWithStatus:)
Rather than using -performSelector: and this ugly (id) cast, why not provide a
declaration of the private function so you can just call it like normal:
[application _terminateWithStatus:exitStatus]?

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:89: int exitStatus = UIApplicationMain(argc, argv,
nil,
Outside of an Objective-C section, name_variables_like_this.

[stuartmorgan, 2012-07-12 13:41:35]

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:63: [NSThread sleepUntilDate:[NSDate
dateWithTimeIntervalSinceNow:2.0]];
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> This also adds a delay to test apps that weren’t too fast. Is it better to
take
> a time since start (since -application:didFinishLaunchingWithOptions: was
> called) to enforce some minimum time, but not cause already long-running tests
> to run even longer?

That's exactly what the TODO is for; I think we should do that as well, but
would rather not block getting tests running upstream on it.

[Mark Mentovai, 2012-07-12 14:04:09]

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:63: [NSThread sleepUntilDate:[NSDate
dateWithTimeIntervalSinceNow:2.0]];
stuartmorgan wrote:
> On 2012/07/12 13:37:50, Mark Mentovai wrote:
> > This also adds a delay to test apps that weren’t too fast. Is it better to
> take
> > a time since start (since -application:didFinishLaunchingWithOptions: was
> > called) to enforce some minimum time, but not cause already long-running
tests
> > to run even longer?
> 
> That's exactly what the TODO is for; I think we should do that as well, but
> would rather not block getting tests running upstream on it.

The TODO says “figure out how much time is needed,” not “don’t make this delay
cumulative with the time the test has already taken.”

[stuartmorgan, 2012-07-12 14:31:57]

On 2012/07/12 14:04:09, Mark Mentovai wrote:
> The TODO says “figure out how much time is needed,” not “don’t make this delay
> cumulative with the time the test has already taken.”

But that's what my comment in the bug says :)

[leng, 2012-07-12 15:18:06]

Thanks, PTAL

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
File base/test/main_hook_ios.mm (right):

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:16: // MainHook saves the chrome main() and calls
UIApplicationMain(),
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> Oh, patch-cest!

Stuart had to explain that to me.  :)

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:19: // notification is received, it fires main()
again to have the real work done.
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> Give an example of usage, showing that the MainHook object should be the first
> thing in main (to avoid duplicating the stuff that happens before MainHook),
and
> that it should be instantiated with |main, argc, argv| as arguments to its
> constructor.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:22: // window displaying the app name. If a bunch of
apps using this are being run
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> “using this” isn’t clear. You mean MainHook, but I thought you meant “the
> window.” Say MainHook.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:40: // Yes, this is leaked, it's just to make what's
running visible.
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> You don’t have to leak it, and it’s easy enough to not leak it. Save it in the
> object and get rid of it after g_main_func (or your sleep) in -runTests.

Done.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:63: [NSThread sleepUntilDate:[NSDate
dateWithTimeIntervalSinceNow:2.0]];
On 2012/07/12 14:04:09, Mark Mentovai wrote:
> stuartmorgan wrote:
> > On 2012/07/12 13:37:50, Mark Mentovai wrote:
> > > This also adds a delay to test apps that weren’t too fast. Is it better to
> > take
> > > a time since start (since -application:didFinishLaunchingWithOptions: was
> > > called) to enforce some minimum time, but not cause already long-running
> tests
> > > to run even longer?
> > 
> > That's exactly what the TODO is for; I think we should do that as well, but
> > would rather not block getting tests running upstream on it.
> 
> The TODO says “figure out how much time is needed,” not “don’t make this delay
> cumulative with the time the test has already taken.”

I hope the comment is clearer now.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:69: [application
performSelector:@selector(_terminateWithStatus:)
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> Rather than using -performSelector: and this ugly (id) cast, why not provide a
> declaration of the private function so you can just call it like normal:
> [application _terminateWithStatus:exitStatus]?

Done.

https://chromiumcodereview.appspot.com/10690161/diff/4006/base/test/main_hook...
base/test/main_hook_ios.mm:89: int exitStatus = UIApplicationMain(argc, argv,
nil,
On 2012/07/12 13:37:50, Mark Mentovai wrote:
> Outside of an Objective-C section, name_variables_like_this.

Done.

[Mark Mentovai, 2012-07-12 15:47:41]

LGTM

[commit-bot: I haz the power, 2012-07-13 08:01:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leng@chromium.org/10690161/12003

[commit-bot: I haz the power, 2012-07-13 09:21:36]

Change committed as 146542