Add apps_browsertests target (currently running 0 tests).

Add apps_browsertests target (currently running 0 tests).

Since trybot configurations only really support building test targets, this change adds a test target (that doesn't run any tests) to ensure that app_shell compiles, so that changes that pass the trybots cannot break the main waterfall due to app_shell compile problems.

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

[Yoyo Zhou, 2014-03-19 23:41:22]

jamescook: can you review apps/shell changes?
jam: can you review the rest?

This is more than strictly necessary to get a null test target up, but it's most
of the way to having working tests.

[James Cook, 2014-03-20 00:00:35]

apps/shell LGTM with nits

Thanks for working on this. Getting CQ/tryjob coverage for app_shell will be a
huge win.

https://codereview.chromium.org/205653002/diff/1/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/205653002/diff/1/apps/apps.gypi#newcode238
apps/apps.gypi:238: 'target_name': 'app_shell_browsertests',
nit: I'm not sure how we name these. Should it be "apps_browsertests"?  I see
content_browsertests and components_browsertests targets.

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
File apps/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:70: if (!process_type.empty())
nit: Maybe comment that empty process_type means browser process and that's the
only one we support?

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:74:
browser_main_runner.reset(content::BrowserMainRunner::Create());
nit: Maybe collapse with line above?

scoped_ptr<content::BrowserMainRunner> runner(
  content::BrowserMainRunner::Create());

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:82: // TODO(yoz): close windows?
Not for this CL, but I suspect we'll need to explicitly close the window and/or
app somehow.

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_tests_main.cc
File apps/test/apps_tests_main.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_tests_main.cc...
apps/test/apps_tests_main.cc:5: // TODO(yoz): fold into ?
?

[jam, 2014-03-20 16:58:13]

very cool, just some small comments

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
File apps/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:67: int ShellMainDelegate::RunProcess(
I'm assuming you copied this from content/shell? That app overrides this method
because it does a bunch of custom things when running layout tests (see
ShellBrowserMain).

But for app_shell, this code is just a duplicate of what the standard
BrowserMain does in content\browser\browser_main. so you shouldn't need to
override this method.

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc
File apps/test/app_shell_test.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc#...
apps/test/app_shell_test.cc:13: #include "base/path_service.h"
nit: not needed

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc#...
apps/test/app_shell_test.cc:15: #include
"content/public/browser/notification_service.h"
nit: not needed

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc#...
apps/test/app_shell_test.cc:54: // TODO(yoz): Add some tests.
I'm not understanding this comment. the tests wouldn't be in this file

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.h
File apps/test/app_shell_test.h (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.h#n...
apps/test/app_shell_test.h:35: bool LoadAndLaunchApp(const base::FilePath&
app_dir);
nit: document

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
File apps/test/apps_test_launcher_delegate.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.cc:10: #include
"content/public/app/content_main_delegate.h"
nit: not needd. also the previous include i think as well

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.cc:15: return AppsTestSuiteBase(argc,
argv).Run();
just call content::ContentTestSuiteBase(argc, argv).Run() directly and remove
apps_test_suite_base.* 

In content, we have apps_test_suite_base because we share it with unit tests and
content browser tests. or at least, before my recent refactoring which you saw,
we needed to share some code. but now we don't, and i'm planning on removing
that base class.

if you create apps_unittests, then you can create a test suite for unittests,
but you shouldn't need one for browser tests. that's not needed because now the
browser test harness sets up everything by calling contentmain.

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
File apps/test/apps_test_launcher_delegate.h (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.h:11: class ContentMainDelegate;
nit: by definition this forward declaration isn't necessary since it's used in
an overridden method

[Yoyo Zhou, 2014-03-20 21:17:30]

https://codereview.chromium.org/205653002/diff/1/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/205653002/diff/1/apps/apps.gypi#newcode238
apps/apps.gypi:238: 'target_name': 'app_shell_browsertests',
On 2014/03/20 00:00:35, James Cook wrote:
> nit: I'm not sure how we name these. Should it be "apps_browsertests"?  I see
> content_browsertests and components_browsertests targets.

I couldn't decide between app_shell_tests and apps_browsertests and went with
the worst of both worlds. apps_browsertests it is.

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
File apps/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:67: int ShellMainDelegate::RunProcess(
On 2014/03/20 16:58:13, jam wrote:
> I'm assuming you copied this from content/shell? That app overrides this
method
> because it does a bunch of custom things when running layout tests (see
> ShellBrowserMain).
> 
> But for app_shell, this code is just a duplicate of what the standard
> BrowserMain does in content\browser\browser_main. so you shouldn't need to
> override this method.

Ok. I deleted it for now. As I was trying to get tests to run, I had to make
numerous changes like this, and I'm not sure which of them were extraneous.

See James's comment below about closing the window/app - I'm not sure where in
the test framework that should be done.

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc
File apps/test/app_shell_test.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc#...
apps/test/app_shell_test.cc:13: #include "base/path_service.h"
On 2014/03/20 16:58:13, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc#...
apps/test/app_shell_test.cc:15: #include
"content/public/browser/notification_service.h"
On 2014/03/20 16:58:13, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.cc#...
apps/test/app_shell_test.cc:54: // TODO(yoz): Add some tests.
On 2014/03/20 16:58:13, jam wrote:
> I'm not understanding this comment. the tests wouldn't be in this file

Removed.

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.h
File apps/test/app_shell_test.h (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/app_shell_test.h#n...
apps/test/app_shell_test.h:35: bool LoadAndLaunchApp(const base::FilePath&
app_dir);
On 2014/03/20 16:58:13, jam wrote:
> nit: document

Done.

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
File apps/test/apps_test_launcher_delegate.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.cc:10: #include
"content/public/app/content_main_delegate.h"
On 2014/03/20 16:58:13, jam wrote:
> nit: not needd. also the previous include i think as well

Done.

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.cc:15: return AppsTestSuiteBase(argc,
argv).Run();
On 2014/03/20 16:58:13, jam wrote:
> just call content::ContentTestSuiteBase(argc, argv).Run() directly and remove
> apps_test_suite_base.* 
> 
> In content, we have apps_test_suite_base because we share it with unit tests
and
> content browser tests. or at least, before my recent refactoring which you
saw,
> we needed to share some code. but now we don't, and i'm planning on removing
> that base class.
> 
> if you create apps_unittests, then you can create a test suite for unittests,
> but you shouldn't need one for browser tests. that's not needed because now
the
> browser test harness sets up everything by calling contentmain.

Why is ContentTestSuiteBase's constructor protected?

I'm going to try base::TestSuite instead.

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
File apps/test/apps_test_launcher_delegate.h (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.h:11: class ContentMainDelegate;
On 2014/03/20 16:58:13, jam wrote:
> nit: by definition this forward declaration isn't necessary since it's used in
> an overridden method

Removed.

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_tests_main.cc
File apps/test/apps_tests_main.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_tests_main.cc...
apps/test/apps_tests_main.cc:5: // TODO(yoz): fold into ?
On 2014/03/20 00:00:35, James Cook wrote:
> ?

deleted

[jam, 2014-03-20 23:43:22]

lgtm

https://codereview.chromium.org/205653002/diff/1/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/205653002/diff/1/apps/apps.gypi#newcode238
apps/apps.gypi:238: 'target_name': 'app_shell_browsertests',
On 2014/03/20 21:17:30, Yoyo Zhou wrote:
> On 2014/03/20 00:00:35, James Cook wrote:
> > nit: I'm not sure how we name these. Should it be "apps_browsertests"?  I
see
> > content_browsertests and components_browsertests targets.
> 
> I couldn't decide between app_shell_tests and apps_browsertests and went with
> the worst of both worlds. apps_browsertests it is.

+1, it's good to have a convention of using browsertests in the name of test
suites which use that harness

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
File apps/test/apps_test_launcher_delegate.cc (right):

https://codereview.chromium.org/205653002/diff/1/apps/test/apps_test_launcher...
apps/test/apps_test_launcher_delegate.cc:15: return AppsTestSuiteBase(argc,
argv).Run();
On 2014/03/20 21:17:30, Yoyo Zhou wrote:
> On 2014/03/20 16:58:13, jam wrote:
> > just call content::ContentTestSuiteBase(argc, argv).Run() directly and
remove
> > apps_test_suite_base.* 
> > 
> > In content, we have apps_test_suite_base because we share it with unit tests
> and
> > content browser tests. or at least, before my recent refactoring which you
> saw,
> > we needed to share some code. but now we don't, and i'm planning on removing
> > that base class.
> > 
> > if you create apps_unittests, then you can create a test suite for
unittests,
> > but you shouldn't need one for browser tests. that's not needed because now
> the
> > browser test harness sets up everything by calling contentmain.
> 
> Why is ContentTestSuiteBase's constructor protected?

I think because before, you always needed to derive from it.

> 
> I'm going to try base::TestSuite instead.

sg, i just looked and there's nothing from there (ContentTestSuiteBase) that you
should need

https://codereview.chromium.org/205653002/diff/20001/apps/shell/app/shell_mai...
File apps/shell/app/shell_main_delegate.h (right):

https://codereview.chromium.org/205653002/diff/20001/apps/shell/app/shell_mai...
apps/shell/app/shell_main_delegate.h:27: protected:
nit: remove this and leave the overridden methods together as before

[Yoyo Zhou, 2014-03-21 00:19:25]

https://codereview.chromium.org/205653002/diff/20001/apps/shell/app/shell_mai...
File apps/shell/app/shell_main_delegate.h (right):

https://codereview.chromium.org/205653002/diff/20001/apps/shell/app/shell_mai...
apps/shell/app/shell_main_delegate.h:27: protected:
On 2014/03/20 23:43:22, jam wrote:
> nit: remove this and leave the overridden methods together as before

Done.

[Yoyo Zhou, 2014-03-21 00:19:30]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/205653002/40001

[commit-bot: I haz the power, 2014-03-21 00:29:16]

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

[commit-bot: I haz the power, 2014-03-21 00:29:17]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Yoyo Zhou, 2014-03-21 00:32:29]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:32:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/205653002/40001

[commit-bot: I haz the power, 2014-03-21 00:51:20]

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

[commit-bot: I haz the power, 2014-03-21 00:51:20]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Yoyo Zhou, 2014-03-21 02:14:21]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-03-21 02:15:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/205653002/60001

[commit-bot: I haz the power, 2014-03-21 06:01:43]

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

[commit-bot: I haz the power, 2014-03-21 06:01:44]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Yoyo Zhou, 2014-03-21 06:23:31]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-03-21 06:23:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/205653002/60001

[commit-bot: I haz the power, 2014-03-21 12:47:39]

Change committed as 258549