Handle host need upgrade in Pin browser test

remoting/webapp/browser_test/browser_test.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * @suppress {checkTypes}  By default, JSCompile is not run on test files.
  *    However, you can modify |remoting_webapp_files.gypi| locally to include
  *    the test in the package to expedite local development.  This suppress
  *    is here so that JSCompile won't complain.
@@ skipping 138 matching lines @@
       }
     }
 
     if (opt_timeout != browserTest.Timeout.NONE) {
       timerId = window.setTimeout(onTimeout, opt_timeout);
     }
     remoting.testEvents.addEventListener(uiModeChanged, onUIModeChanged);
   });
 };
 
+browserTest.connectMe2Me = function() {
+  var AppMode = remoting.AppMode;
+  browserTest.clickOnControl('this-host-connect');
+  return browserTest.onUIMode(AppMode.CLIENT_HOST_NEEDS_UPGRADE).then(
+    function() {
+      // On fulfilled.
+      browserTest.clickOnControl('host-needs-update-connect-button');

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

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.

[kelvinp, 2014/07/11 21:11:22]

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?
 

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

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.

+    }, function() {
+      // On time out.
+      return Promise.resolve();
+    }).then(function() {
+      return browserTest.onUIMode(AppMode.CLIENT_PIN_PROMPT);
+    });
+}
+
 browserTest.expectMe2MeError = function(errorTag) {
   var AppMode = remoting.AppMode;
   var Timeout = browserTest.Timeout;
 
   var onConnected = browserTest.onUIMode(AppMode.IN_SESSION, Timeout.None);
   var onFailure = browserTest.onUIMode(AppMode.CLIENT_CONNECT_FAILED_ME2ME);
 
   onConnected = onConnected.then(function() {
     return Promise.reject(
         'Expected the Me2Me connection to fail.');
@@ skipping 35 matching lines @@
   try {
     var test = new testClass();
     browserTest.expect(typeof test.run == 'function');
     test.run(data);
   } catch (e) {
     browserTest.fail(e);
   }
 };
 
 browserTest.init();