Chromecast: end-to-end browser test based on content::BrowserTest.

Chromecast: end-to-end browser test based on content::BrowserTest.

R=lcwu@chromium.org,byungchul@chromium.org,phajdan.jr@chromium.org
BUG=409163
Committed: https://crrev.com/3c2a17334b2d458a614af4727b88087379412d96
Cr-Commit-Position: refs/heads/master@{#294523}

[byungchul, 2014-08-30 00:19:00]

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
File chromecast/shell/browser/test/chromecast_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
chromecast/shell/browser/test/chromecast_browser_test.cc:36: void
ChromecastBrowserTest::TearDown() {
necessary while it calls just super class function?

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
File chromecast/shell/browser/test/chromecast_browser_test_runner.cc (right):

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
chromecast/shell/browser/test/chromecast_browser_test_runner.cc:35: class
ChromecastTestLauncherDelegate : public content::TestLauncherDelegate {
Why not defined in anonymous namespace?

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:22: GURL url_;
const?

[gunsch, 2014-08-30 00:21:18]

FYI, feel free not to review yet; I hadn't mailed this because it needs a rebase
on https://codereview.chromium.org/521753002/ once submitted

[gunsch, 2014-08-30 15:28:43]

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
File chromecast/shell/browser/test/chromecast_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
chromecast/shell/browser/test/chromecast_browser_test.cc:36: void
ChromecastBrowserTest::TearDown() {
On 2014/08/30 00:18:59, byungchul wrote:
> necessary while it calls just super class function?

Done.

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
File chromecast/shell/browser/test/chromecast_browser_test_runner.cc (right):

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
chromecast/shell/browser/test/chromecast_browser_test_runner.cc:35: class
ChromecastTestLauncherDelegate : public content::TestLauncherDelegate {
On 2014/08/30 00:19:00, byungchul wrote:
> Why not defined in anonymous namespace?

Done.

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:22: GURL url_;
On 2014/08/30 00:19:00, byungchul wrote:
> const?

Done.

[gunsch, 2014-09-08 18:00:59]

On 2014/08/30 15:28:43, gunsch wrote:
>
https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
> File chromecast/shell/browser/test/chromecast_browser_test.cc (right):
> 
>
https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
> chromecast/shell/browser/test/chromecast_browser_test.cc:36: void
> ChromecastBrowserTest::TearDown() {
> On 2014/08/30 00:18:59, byungchul wrote:
> > necessary while it calls just super class function?
> 
> Done.
> 
>
https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
> File chromecast/shell/browser/test/chromecast_browser_test_runner.cc (right):
> 
>
https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
> chromecast/shell/browser/test/chromecast_browser_test_runner.cc:35: class
> ChromecastTestLauncherDelegate : public content::TestLauncherDelegate {
> On 2014/08/30 00:19:00, byungchul wrote:
> > Why not defined in anonymous namespace?
> 
> Done.
> 
>
https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
> File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):
> 
>
https://codereview.chromium.org/518773003/diff/1/chromecast/shell/browser/tes...
> chromecast/shell/browser/test/chromecast_shell_browser_test.cc:22: GURL url_;
> On 2014/08/30 00:19:00, byungchul wrote:
> > const?
> 
> Done.

ping?

[byungchul, 2014-09-08 18:24:53]

lgtm

[lcwu1, 2014-09-08 19:49:15]

LGTM after fixing a couple of formatting nits.

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_main_parts.h (right):

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_main_parts.h:38: const
content::MainFunctionParams parameters_;
The way the data members are broken up and where you place the comment make it
look like both parameters_ and url_request_context_factory_ are both used for
running browser tests. Maybe we should remove the empty line on line 36, or
append the comment on line 37 to line 38.

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:14:
ChromecastShellBrowserTest() : url_("about:blank") { }
No spaces inside empty braces. (See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Horizontal_Wh...)

[gunsch, 2014-09-08 21:31:48]

gunsch@chromium.org changed reviewers:
+ phajdan.jr@chromium.org
- jam@chromium.org

[gunsch, 2014-09-08 21:34:46]

Done, and added Paweł to review for content/public/test (in DEPS).

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/cast_browser_main_parts.h (right):

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/cast_browser_main_parts.h:38: const
content::MainFunctionParams parameters_;
On 2014/09/08 19:49:15, lcwu1 wrote:
> The way the data members are broken up and where you place the comment make it
> look like both parameters_ and url_request_context_factory_ are both used for
> running browser tests. Maybe we should remove the empty line on line 36, or
> append the comment on line 37 to line 38.

Done.

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:14:
ChromecastShellBrowserTest() : url_("about:blank") { }
On 2014/09/08 19:49:15, lcwu1 wrote:
> No spaces inside empty braces. (See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Horizontal_Wh...)

Done.

[Paweł Hajdan Jr., 2014-09-09 12:24:31]

Also please add me to OWNERS of these test directories if possible.

https://codereview.chromium.org/518773003/diff/80001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/518773003/diff/80001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:259: 'include_dirs': [
Shouldn't this rather depend (directly or indirectly) on gtest.gyp?

https://codereview.chromium.org/518773003/diff/80001/chromecast/shell/browser...
File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/80001/chromecast/shell/browser...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:14:
ChromecastShellBrowserTest() : url_("about:blank") {}
nit: There is a constant for about:blank, please use it.

https://codereview.chromium.org/518773003/diff/80001/chromecast/shell/browser...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:29: // This test
case can be removed once actual assertions are being tested.
Please add a TODO to do this.

[gunsch, 2014-09-09 16:02:11]

Thanks for looking, PTAL

https://codereview.chromium.org/518773003/diff/80001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/518773003/diff/80001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:259: 'include_dirs': [
On 2014/09/09 12:24:31, Paweł Hajdan Jr. wrote:
> Shouldn't this rather depend (directly or indirectly) on gtest.gyp?

It does: dependency on cast_shell_test_support --> testing/gtest.gyp. Looks like
the target only sets include_dirs on direct_dependent_settings, not
all_dependent_settings.

Added direct dependency and removed the include_dirs.

https://codereview.chromium.org/518773003/diff/80001/chromecast/shell/browser...
File chromecast/shell/browser/test/chromecast_shell_browser_test.cc (right):

https://codereview.chromium.org/518773003/diff/80001/chromecast/shell/browser...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:14:
ChromecastShellBrowserTest() : url_("about:blank") {}
On 2014/09/09 12:24:31, Paweł Hajdan Jr. wrote:
> nit: There is a constant for about:blank, please use it.

Done.

https://codereview.chromium.org/518773003/diff/80001/chromecast/shell/browser...
chromecast/shell/browser/test/chromecast_shell_browser_test.cc:29: // This test
case can be removed once actual assertions are being tested.
On 2014/09/09 12:24:31, Paweł Hajdan Jr. wrote:
> Please add a TODO to do this.

Done.

[Paweł Hajdan Jr., 2014-09-11 09:23:19]

LGTM

[gunsch, 2014-09-11 23:29:13]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-09-12 00:15:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/518773003/120001

[commit-bot: I haz the power, 2014-09-12 02:08:48]

Committed patchset #7 (id:120001) as b350415c8e94712d77424ebc69aa83a520f74f30

[commit-bot: I haz the power, 2014-09-12 02:13:08]

Patchset 7 (id:??) landed as
https://crrev.com/3c2a17334b2d458a614af4727b88087379412d96
Cr-Commit-Position: refs/heads/master@{#294523}