Handle host need upgrade in Pin browser test

Handle host need upgrade in Pin browser test

This CL handles create a helper method in browser_tests to dismiss the host-out-of-date warning if it is displayed.

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

[kelvinp, 2014-07-11 19:03:07]

PTAL

[Jamie, 2014-07-11 19:33:22]

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:165:
browserTest.clickOnControl('host-needs-update-connect-button');
Rather than silently dismissing the dialog if it appears, would it be better to
pass in whether or not it's expected? We should have a test for this feature,
not treat the two cases the same. If it's convenient to do that in the short
term to unblock testing then that's okay, but there should at least be a TODO.

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:42:
browserTest.connectMe2Me
Is this a copy/paste error? The previous code was disconnecting, but the new
code is connecting.

[kelvinp, 2014-07-11 21:11:23]

PTAL

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:165:
browserTest.clickOnControl('host-needs-update-connect-button');
On 2014/07/11 19:33:22, Jamie wrote:
> Rather than silently dismissing the dialog if it appears, would it be better
to
> pass in whether or not it's expected? We should have a test for this feature,
> not treat the two cases the same. If it's convenient to do that in the short
> term to unblock testing then that's okay, but there should at least be a TODO.

Good question. Browser tests are configured in the lab to run with various
combinations of the web app against the host.  For instance, Anand is planning
to set up a rolling build bot that runs the web app in the daily build against
the released host in Lorry, which will always "need upgrade"

In my opinion, most test should be agnostic to whether the host needs upgrade or
not. For instance, if the pin can be upgraded correctly, the pin upgrade test
should pass even if the host need upgrade dialog shows up.

Therefore, I don't want to expose an extra flag in the function to fail the test
when the host need upgrade dialog shows up as it may make our test flaky.  In
fact, the C++ browser tests has exactly the same behavior.
RemoteDesktopBrowserTest::ConnectToLocalHost silently dismisses the upgrade
dialog under the hood.

The only exception would be the "host needs upgrade test" suggested in your
comment.  In that case, we can write a separate browser case to verify the
behavior.
What do you think?

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/update_pin_browser_test.js (right):

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/update_pin_browser_test.js:42:
browserTest.connectMe2Me
On 2014/07/11 19:33:22, Jamie wrote:
> Is this a copy/paste error? The previous code was disconnecting, but the new
> code is connecting.

My bad.  Good catch.

[garykac, 2014-07-11 23:25:40]

https://codereview.chromium.org/389673003/diff/60001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/389673003/diff/60001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:159: browserTest.connectMe2Me =
function() {
connectMe2Me doesn't feel like the right name for this function.
This function doesn't actually connect - it just takes the connection flow up to
the Pin dialog prompt.

Perhaps clickConnectButton?

[Jamie, 2014-07-14 18:24:04]

lgtm

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/389673003/diff/40001/remoting/webapp/browser_...
remoting/webapp/browser_test/browser_test.js:165:
browserTest.clickOnControl('host-needs-update-connect-button');
On 2014/07/11 21:11:22, kelvinp wrote:
> On 2014/07/11 19:33:22, Jamie wrote:
> > Rather than silently dismissing the dialog if it appears, would it be better
> to
> > pass in whether or not it's expected? We should have a test for this
feature,
> > not treat the two cases the same. If it's convenient to do that in the short
> > term to unblock testing then that's okay, but there should at least be a
TODO.
> 
> Good question. Browser tests are configured in the lab to run with various
> combinations of the web app against the host.  For instance, Anand is planning
> to set up a rolling build bot that runs the web app in the daily build against
> the released host in Lorry, which will always "need upgrade"
> 
> In my opinion, most test should be agnostic to whether the host needs upgrade
or
> not. For instance, if the pin can be upgraded correctly, the pin upgrade test
> should pass even if the host need upgrade dialog shows up.
> 
> Therefore, I don't want to expose an extra flag in the function to fail the
test
> when the host need upgrade dialog shows up as it may make our test flaky.  In
> fact, the C++ browser tests has exactly the same behavior.
> RemoteDesktopBrowserTest::ConnectToLocalHost silently dismisses the upgrade
> dialog under the hood.
> 
> The only exception would be the "host needs upgrade test" suggested in your
> comment.  In that case, we can write a separate browser case to verify the
> behavior.
> What do you think?
>  

That was pretty much what I had in mind. I think you're right that we don't want
every test to need to know about this dialog, but we'll probably want to re-use
this most of this code when we write one that does. I was thinking along the
lines of an enum { OPTIONAL, REQUIRED, DISALLOWED } for the upgrade dialog, and
make it default to OPTIONAL. It doesn't need to be this CL though.

[kelvinp, 2014-07-15 02:07:20]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-07-15 02:09:52]

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

[commit-bot: I haz the power, 2014-07-15 19:58:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-16 01:14:33]

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

[commit-bot: I haz the power, 2014-07-16 01:14:34]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[kelvinp, 2014-07-17 03:05:41]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:06:42]

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

[commit-bot: I haz the power, 2014-07-17 05:03:07]

Change committed as 283647