Add a basic demo mode browser test.

chrome/browser/chromeos/login/demo_mode/demo_app_launcher_browsertest.cc

+// Copyright (c) 2013 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.
+
+#include "apps/app_window_registry.h"

[xiyuan, 2014/03/20 04:59:54]

seems not used.

[rkc, 2014/03/20 21:45:01]

Done.

+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/path_service.h"
+#include "chrome/browser/apps/app_browsertest_util.h"

[xiyuan, 2014/03/20 04:59:54]

is this needed?

[rkc, 2014/03/20 21:45:01]

Done.

+#include "chrome/browser/chromeos/login/demo_mode/demo_app_launcher.h"
+#include "chrome/browser/extensions/extension_test_message_listener.h"
+#include "chrome/browser/profiles/profile_manager.h"

[xiyuan, 2014/03/20 04:59:54]

no longer needed.

[rkc, 2014/03/20 21:45:01]

Done.

+#include "chrome/common/chrome_paths.h"
+#include "chromeos/chromeos_switches.h"
+#include "content/public/test/test_utils.h"

[bartfab (slow), 2014/03/20 13:22:29]

Nit: Is this used?

[rkc, 2014/03/20 21:45:01]

Done.

+#include "extensions/common/extension.h"

[xiyuan, 2014/03/20 04:59:54]

seems not used.

[rkc, 2014/03/20 21:45:01]

Done.

+
+

[xiyuan, 2014/03/20 01:14:03]

nit: nuke one empty line

[rkc, 2014/03/20 21:45:01]

Done.

+namespace chromeos {
+
+char kDemoAppId[] = "klimoghijjogocdbaikffefjfcfheiel";

[xiyuan, 2014/03/20 04:59:54]

not used.

[rkc, 2014/03/20 21:45:01]

Done.

+
+class DemoAppLauncherTest : public InProcessBrowserTest {

[xiyuan, 2014/03/20 04:59:54]

Let's still derive from ExtensionBrowserTest to be on the safe side.

[bartfab (slow), 2014/03/20 13:22:29]

Nit: #include "chrome/test/base/in_process_browser_test.h"

[rkc, 2014/03/20 21:45:01]

Done.

[rkc, 2014/03/20 21:45:01]

Deriving from ExtensionBrowserTest now, so not relevant anymore.

+ public:
+  DemoAppLauncherTest() {
+    set_exit_when_last_browser_closes(false);
+  }
+  virtual ~DemoAppLauncherTest() {}

[bartfab (slow), 2014/03/20 13:22:29]

Nit: add blank line above.

[rkc, 2014/03/20 21:45:01]

Done.

+
+  virtual void SetUpCommandLine(CommandLine* cmd_line) OVERRIDE {

[bartfab (slow), 2014/03/20 13:22:29]

Nit 1: Avoid abbreviations: s/cmd_line/command_line/
Nit 2: #include "base/compiler_specific.h"

[rkc, 2014/03/20 21:45:01]

Done.

+    cmd_line->AppendSwitch(chromeos::switches::kLoginManager);

[bartfab (slow), 2014/03/20 13:22:29]

Nit: Here and below: No need for the chromeos:: prefix.

[rkc, 2014/03/20 21:45:01]

Done.

+    cmd_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests);
+    cmd_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "user");
+
+    cmd_line->AppendSwitchASCII(chromeos::switches::kDerelictIdleTimeout, "0");
+    cmd_line->AppendSwitchASCII(chromeos::switches::kOobeTimerInterval, "0");
+    cmd_line->AppendSwitchASCII(chromeos::switches::kDerelictDetectionTimeout,
+                                "0");
+  }
+
+  virtual void SetUp() OVERRIDE {
+    chromeos::DemoAppLauncher::SetDemoAppPathForTesting(GetTestDemoAppPath());
+    InProcessBrowserTest::SetUp();
+  }
+
+  base::FilePath GetTestDemoAppPath() {

[bartfab (slow), 2014/03/20 13:22:29]

Why is GetTestDemoAppPath() a separate method? These few lines could just be
part of SetUp().

[bartfab (slow), 2014/03/20 13:22:29]

Nit: This should be a const method.

[rkc, 2014/03/20 21:45:01]

Done.

[rkc, 2014/03/20 21:45:01]

Getting the path to the test demo app is an individual operation in itself,
which can later possibly be more complex. It doesn't really hurt to put this in
a separate method; IMHO it looks cleaner.

+    base::FilePath test_data_dir;
+    PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);

[bartfab (slow), 2014/03/20 13:22:29]

Nit: PathService::Get() is normally wrapped in an ASSERT_TRUE(). You cannot do
it here because your method has a return value, but you can do the following
instead:

if (!PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir))
  FAIL();

[rkc, 2014/03/20 21:45:01]

ASSERT and FAIL have issues with returning a FilePath.
EXPECT_TRUE does the job.
Done.

[xiyuan, 2014/03/20 21:59:44]

We can also CHECK. Some tests do that when the enclosing function does not
return void.

[bartfab (slow), 2014/03/21 10:37:32]

I think CHECKs in tests are a bad idea. They crash the test while an EXPECT_TRUE
would allow the test to wind down properly, cleanly reporting a failure at the
end.

+    return test_data_dir.Append(FILE_PATH_LITERAL("chromeos/demo_app"));
+  }
+
+  void VerifyDemoAppLaunch() {
+    ExtensionTestMessageListener

[bartfab (slow), 2014/03/20 13:22:29]

Nit: You can do this with a temporary:

EXPECT_TRUE(ExtensionTestMessageListener("launchedDemoApp",
false).WaitUntilSatisfied());

[rkc, 2014/03/20 21:45:01]

Done.

+        launch_data_check_listener("launchedDemoApp", false);
+    EXPECT_TRUE(launch_data_check_listener.WaitUntilSatisfied());

[bartfab (slow), 2014/03/20 13:22:29]

1: Overall, this is racy. What if the app manages to launch before the
ExtensionTestMessageListener has been instantiated? You should instantiate the
listener during setup.

2: Nit: #include "testing/gtest/include/gtest/gtest.h"

[rkc, 2014/03/20 21:45:01]

1.) We don't actually run the message loop until we setup the listener - in
fact, WaitUntilSatisfied is what runs the loop. How would the app load?
2.) Done.

[bartfab (slow), 2014/03/21 10:37:32]

1.) InProcessBrowserTest spins the loop before we even get to the test body.
This is why setting up a listener in the test body is racy.

[xiyuan, 2014/03/21 16:38:16]

How about we make it a member of DemoAppLauncherTest to ensure it starts
listening before everything?

[rkc, 2014/03/21 23:31:18]

Xiyuan and I discussed this offline. This test is unique in the way that we
don't really control when the test starts. It starts pretty much as soon as the
browser is launched. Hence controlling 'when' the app will launch is not really
possible. To fix the situation, I've changed the verification completely to
instead wait for the app window to come up. Using that as the verification
criteria, if the app has already launched, we're still fine since we can just
check if the app window exists already and know that our test passed, otherwise,
we simply wait for the window to show up.

+}

[xiyuan, 2014/03/20 01:14:03]

nit: wrong indent

[rkc, 2014/03/20 21:45:01]

Done.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DemoAppLauncherTest);

[bartfab (slow), 2014/03/20 13:22:29]

Nit: #include "base/basictypes.h"

[rkc, 2014/03/20 21:45:01]

Done.

+};
+
+IN_PROC_BROWSER_TEST_F(DemoAppLauncherTest, Basic) {
+  VerifyDemoAppLaunch();

[bartfab (slow), 2014/03/20 13:22:29]

Nit: Move the body of your test into here. No need for a VerifyDemoAppLaunch()
method.

[rkc, 2014/03/20 21:45:01]

I prefer this in a separate method. This will potentially be called from any
tests added here.

[bartfab (slow), 2014/03/21 10:37:32]

The problem is that the EXPECT* and ASSERT* macros print the line number when
things fail. If the macros are used in a helper method, any failures will point
at the helper, making it more difficult to figure out where in the main test
flow things went wrong. It's trivial here of course but the moment a test
happens to call VerifyDemoAppLaunch() twice, it will be impossible to tell which
of the calls failed. This is why putting EXPECT* and ASSERT* in helper methods
is a bad pattern that should be avoided in tests.

[rkc, 2014/03/21 23:31:18]

Changed this to have the EXPECT_ in the main test body instead.

+}
+
+}  // namespace chromeos