Automate host start up in browser test

Automate host start up in browser test

After this CL, SetupTestForMe2Me will ensure that the host is up and running with the pin specified from the command line argument --me2me-pin on every Me2Me test run.

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

[kelvinp, 2014-07-19 01:18:58]

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (left):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:348: ConditionalTimeoutWaiter
waiter(
After the CL, we no longer assumes that the host is up and running before
expandme2me.  Instead, we expand me2me first and then start the host if it is
not online.

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:366:
ASSERT_EQ(Chromoting_Main_URL(), GetCurrentURL());
This assert fails as after a PIN setup.  The URL is appended with a hash.  I
don't see why this check is necessary so I removed it.

[weitao, 2014-07-21 18:53:45]

Driving by:

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:473:
LoadScript(app_web_content(), FILE_PATH_LITERAL("browser_test.js"));
Ideally, loading the test script is a setup step and should be done before any
test is run. But I also realize it probably cannot be loaded before the
chromoting app is launched. So maybe we should make it part of launching the app
(loading the script right after launching the app).

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:261: if
(startDaemonButton.offsetWidth != 0) {
Could you please add a comment here? What does the offsetWidth signify?

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:263:
browserTest.clickOnControl('start-daemon');
Have you tested this on all platforms? I assume you will need to interact with
system dialogs (PrefPane for Mac and UAC for Windows).

[Jamie, 2014-07-22 18:47:12]

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/pin_bro...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/pin_bro...
chrome/test/remoting/pin_browsertest.cc:32: "pin: '" + me2me_pin() + "ABC'"
This doesn't appear to be related to this CL? Could it be done separately?

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (left):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:348: ConditionalTimeoutWaiter
waiter(
On 2014/07/19 01:18:58, kelvinp wrote:
> After the CL, we no longer assumes that the host is up and running before
> expandme2me.  Instead, we expand me2me first and then start the host if it is
> not online.

Acknowledged.

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:366:
ASSERT_EQ(Chromoting_Main_URL(), GetCurrentURL());
On 2014/07/19 01:18:58, kelvinp wrote:
> This assert fails as after a PIN setup.  The URL is appended with a hash.  I
> don't see why this check is necessary so I removed it.

Acknowledged.

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:491: EXPECT_TRUE(result) <<
"Cannot start the host with Pin:" << me2me_pin();
Unless the PIN is likely to be a reason for failure, I don't think it's worth
logging it here--it's just extra log noise.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:240: failure.then(function(){
I think you need to update failure, ie:

failure = failure.then...

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:257: var Timeout =
browserTest.Timeout;
You don't seem to be using any of these definitions.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:261: if
(startDaemonButton.offsetWidth != 0) {
On 2014/07/21 18:53:45, weitaosu wrote:
> Could you please add a comment here? What does the offsetWidth signify?

It's an indication that the element is visible, but I don't think it's the right
approach here. The intent of this function is to ensure that the host is
enabled, not that the UI indicates that it is enabled (we should have a separate
test for that). I think it would be better (and self-documenting) to check the
state directly.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:263:
browserTest.clickOnControl('start-daemon');
On 2014/07/21 18:53:45, weitaosu wrote:
> Have you tested this on all platforms? I assume you will need to interact with
> system dialogs (PrefPane for Mac and UAC for Windows).

This is a good point. If we can't implement this cross-platform, is the any
value in making it work on Linux?

[kelvinp, 2014-07-23 22:22:27]

PTAL

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/pin_bro...
File chrome/test/remoting/pin_browsertest.cc (right):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/pin_bro...
chrome/test/remoting/pin_browsertest.cc:32: "pin: '" + me2me_pin() + "ABC'"
On 2014/07/22 18:47:11, Jamie wrote:
> This doesn't appear to be related to this CL? Could it be done separately?

With this change, we will set up the host with the me2me_pin(), if we can try to
connect again with me2me_pin(), it won't be an invalid pin.  Therefore, I append
some random string 'ABC' after the pin to make it invalid.

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:473:
LoadScript(app_web_content(), FILE_PATH_LITERAL("browser_test.js"));
On 2014/07/21 18:53:45, weitaosu wrote:
> Ideally, loading the test script is a setup step and should be done before any
> test is run. But I also realize it probably cannot be loaded before the
> chromoting app is launched. So maybe we should make it part of launching the
app
> (loading the script right after launching the app). 

Done.

https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:491: EXPECT_TRUE(result) <<
"Cannot start the host with Pin:" << me2me_pin();
On 2014/07/22 18:47:11, Jamie wrote:
> Unless the PIN is likely to be a reason for failure, I don't think it's worth
> logging it here--it's just extra log noise.

The EXPECT_TRUE will ONLY log when the setup fails.  I think it is beneficial to
have logs to help debugging test failure.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:240: failure.then(function(){
On 2014/07/22 18:47:11, Jamie wrote:
> I think you need to update failure, ie:
> 
> failure = failure.then...

Done.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:257: var Timeout =
browserTest.Timeout;
On 2014/07/22 18:47:11, Jamie wrote:
> You don't seem to be using any of these definitions.

Done.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:261: if
(startDaemonButton.offsetWidth != 0) {
On 2014/07/22 18:47:11, Jamie wrote:
> On 2014/07/21 18:53:45, weitaosu wrote:
> > Could you please add a comment here? What does the offsetWidth signify?
> 
> It's an indication that the element is visible, but I don't think it's the
right
> approach here. The intent of this function is to ensure that the host is
> enabled, not that the UI indicates that it is enabled (we should have a
separate
> test for that). I think it would be better (and self-documenting) to check the
> state directly.

Done.

https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:263:
browserTest.clickOnControl('start-daemon');
On 2014/07/22 18:47:11, Jamie wrote:
> On 2014/07/21 18:53:45, weitaosu wrote:
> > Have you tested this on all platforms? I assume you will need to interact
with
> > system dialogs (PrefPane for Mac and UAC for Windows).
> 
> This is a good point. If we can't implement this cross-platform, is the any
> value in making it work on Linux?

The code will work on Windows and Linux, as we are disabling UAC on windows on
the test machine.  I spoke to weitao and Anand and we will make the test work
for Windows and Linux working first.  I added a todo comment in the C++ code for
future support on Mac.

[weitao, 2014-07-23 22:38:31]

On 2014/07/23 22:22:27, kelvinp wrote:
> PTAL
> 
>
https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/pin_bro...
> File chrome/test/remoting/pin_browsertest.cc (right):
> 
>
https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/pin_bro...
> chrome/test/remoting/pin_browsertest.cc:32: "pin: '" + me2me_pin() + "ABC'"
> On 2014/07/22 18:47:11, Jamie wrote:
> > This doesn't appear to be related to this CL? Could it be done separately?
> 
> With this change, we will set up the host with the me2me_pin(), if we can try
to
> connect again with me2me_pin(), it won't be an invalid pin.  Therefore, I
append
> some random string 'ABC' after the pin to make it invalid.
> 
>
https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
> File chrome/test/remoting/remote_desktop_browsertest.cc (right):
> 
>
https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
> chrome/test/remoting/remote_desktop_browsertest.cc:473:
> LoadScript(app_web_content(), FILE_PATH_LITERAL("browser_test.js"));
> On 2014/07/21 18:53:45, weitaosu wrote:
> > Ideally, loading the test script is a setup step and should be done before
any
> > test is run. But I also realize it probably cannot be loaded before the
> > chromoting app is launched. So maybe we should make it part of launching the
> app
> > (loading the script right after launching the app). 
> 
> Done.
> 
>
https://codereview.chromium.org/398823005/diff/1/chrome/test/remoting/remote_...
> chrome/test/remoting/remote_desktop_browsertest.cc:491: EXPECT_TRUE(result) <<
> "Cannot start the host with Pin:" << me2me_pin();
> On 2014/07/22 18:47:11, Jamie wrote:
> > Unless the PIN is likely to be a reason for failure, I don't think it's
worth
> > logging it here--it's just extra log noise.
> 
> The EXPECT_TRUE will ONLY log when the setup fails.  I think it is beneficial
to
> have logs to help debugging test failure.
> 
>
https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
> File remoting/webapp/browser_test/browser_test.js (right):
> 
>
https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
> remoting/webapp/browser_test/browser_test.js:240: failure.then(function(){
> On 2014/07/22 18:47:11, Jamie wrote:
> > I think you need to update failure, ie:
> > 
> > failure = failure.then...
> 
> Done.
> 
>
https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
> remoting/webapp/browser_test/browser_test.js:257: var Timeout =
> browserTest.Timeout;
> On 2014/07/22 18:47:11, Jamie wrote:
> > You don't seem to be using any of these definitions.
> 
> Done.
> 
>
https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
> remoting/webapp/browser_test/browser_test.js:261: if
> (startDaemonButton.offsetWidth != 0) {
> On 2014/07/22 18:47:11, Jamie wrote:
> > On 2014/07/21 18:53:45, weitaosu wrote:
> > > Could you please add a comment here? What does the offsetWidth signify?
> > 
> > It's an indication that the element is visible, but I don't think it's the
> right
> > approach here. The intent of this function is to ensure that the host is
> > enabled, not that the UI indicates that it is enabled (we should have a
> separate
> > test for that). I think it would be better (and self-documenting) to check
the
> > state directly.
> 
> Done.
> 
>
https://codereview.chromium.org/398823005/diff/1/remoting/webapp/browser_test...
> remoting/webapp/browser_test/browser_test.js:263:
> browserTest.clickOnControl('start-daemon');
> On 2014/07/22 18:47:11, Jamie wrote:
> > On 2014/07/21 18:53:45, weitaosu wrote:
> > > Have you tested this on all platforms? I assume you will need to interact
> with
> > > system dialogs (PrefPane for Mac and UAC for Windows).
> > 
> > This is a good point. If we can't implement this cross-platform, is the any
> > value in making it work on Linux?
> 
> The code will work on Windows and Linux, as we are disabling UAC on windows on
> the test machine.  I spoke to weitao and Anand and we will make the test work
> for Windows and Linux working first.  I added a todo comment in the C++ code
for
> future support on Mac.

I think the test still needs to interact with some system dialogs even with UAC
turned off. LGTM if you have verified that it indeed works on both Windows and
Linux. And please create a bug for the remaining mac work and reference the bug
# in your comment.

[kelvinp, 2014-07-25 17:32:11]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-07-25 17:33:17]

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

[commit-bot: I haz the power, 2014-07-25 20:03:48]

Change committed as 285651