Refactoring startup logic for upcoming FRE changes (non-Win 10).

chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc

Index: chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc b/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b6d8d62cd6dbdb1626eb66b4c32c47c989018708
--- /dev/null
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
@@ -0,0 +1,125 @@
+// Copyright 2016 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 "chrome/browser/ui/startup/startup_browser_creator_impl.h"
+
+#include "base/command_line.h"
+#include "chrome/browser/ui/startup/startup_tab_provider.h"
+#include "chrome/common/url_constants.cc"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class StartupBrowserCreatorImplTest : public testing::Test {};
+
+class MockStartupTabProvider : public StartupTabProvider {
+ public:
+  MOCK_METHOD0(GetOnboardingTabs, StartupTabs());
+  MOCK_METHOD1(GetMasterPreferencesTabs,
+               StartupTabs(StartupBrowserCreator* browser_creator));
+  MOCK_METHOD1(GetResetTriggerTabs, StartupTabs(Profile* profile));
+  MOCK_METHOD0(GetPinnedTabs, StartupTabs());
+  MOCK_METHOD0(GetPreferencesTabs, StartupTabs());
+};
+
+// Helper method which modifies the given MockStartupTabProvider, such that for
+// each boolean argument passed as true, the corresponding mock getter method
+// returns a sentinel value. (By default, the mock getters return empty
+// vectors.)
+void PrepMockProvider(MockStartupTabProvider* provider,
+                      bool onboarding,
+                      bool master_preferences,
+                      bool reset_trigger,
+                      bool pinned,
+                      bool preferences) {
+  if (onboarding) {
+    StartupTab onboarding_tab{GURL("https://onboarding"), false};

[grt (UTC plus 2), 2016/09/09 11:40:30]

nit: prefer ctor syntax over uniform syntax as per
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a....

+    StartupTabs onboarding_result;

[grt (UTC plus 2), 2016/09/09 11:40:30]

better yet, roll these three lines into one:
    StartupTabs onboarding_result = {StartupTab(GURL("https://onboarding"),
false)}

+    onboarding_result.push_back(onboarding_tab);
+    ON_CALL(*provider, GetOnboardingTabs())

[grt (UTC plus 2), 2016/09/09 11:40:30]

by using ON_CALL, this mock object isn't actually enforcing any expectations;
i.e., the test won't fail on account of these methods not being called (though
it may fail due to the wrong return values being used). if you really don't mean
to assert that the method(s) are called, perhaps a fake would be simpler than a
mock and easier to understand for those who dislike gmock (there are many).

+        .WillByDefault(testing::Return(onboarding_result));
+  }
+
+  if (master_preferences) {
+    StartupTab master_prefs_tab{GURL("https://master-prefs"), false};
+    StartupTabs master_prefs_result;
+    master_prefs_result.push_back(master_prefs_tab);
+    ON_CALL(*provider, GetMasterPreferencesTabs(testing::_))
+        .WillByDefault(testing::Return(master_prefs_result));
+  }
+
+  if (reset_trigger) {
+    StartupTab reset_trigger_tab{GURL("https://reset-trigger"), false};
+    StartupTabs reset_trigger_result;
+    reset_trigger_result.push_back(reset_trigger_tab);
+    ON_CALL(*provider, GetResetTriggerTabs(testing::_))
+        .WillByDefault(testing::Return(reset_trigger_result));
+  }
+
+  if (pinned) {
+    StartupTab pinned_tab{GURL("https://pinned"), true};
+    StartupTabs pinned_result;
+    pinned_result.push_back(pinned_tab);
+    ON_CALL(*provider, GetPinnedTabs())
+        .WillByDefault(testing::Return(pinned_result));
+  }
+
+  if (preferences) {
+    StartupTab preferences_tab{GURL("https://prefs"), false};
+    StartupTabs preferences_result;
+    preferences_result.push_back(preferences_tab);
+    ON_CALL(*provider, GetPreferencesTabs())
+        .WillByDefault(testing::Return(preferences_result));
+  }
+}
+
+// "Standard" case: Tabs specified in onboarding, reset trigger, pinned tabs, or
+// preferences shouldn't interfere with other content. Pinned tabs appear first.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs) {
+  MockStartupTabProvider provider;
+  PrepMockProvider(&provider, true, false, true, true, true);

[grt (UTC plus 2), 2016/09/09 11:40:30]

while PrepMockProvider may reduce pure lines-of-code, it unfortunately serves to
obscure what the test is actually doing. in cases like this, i think it's better
to make the test as clear as possible. this is in your best interest as well,
since it makes it more likely that someone breaking the test in the future will
be able to figure out what's going on without having to ask you. :-)

i wonder if a fake would be easier to use and understand. imagine for example:

// Bits for provider options.
constexpr uint32_t kOnboardingTabs = 0x01;
constexpr uint32_t kFirstRunTabs = 0x02;
constexpr uint32_t kResetTrigger = 0x04;
constexpr uint32_t kPinnedTabs = 0x08;
constexpr uint32_t kPrefsTabs = 0x10;

class FakeStartupTabProvider : public StartupTabProvider {
 public:
  explicit FakeStartupTabProvider(uint32_t options) : options_(options) {}
  StartupTabs GetOnboardingTabs() override {
    if (options_ & kOnboardingTabs)
      return StartupTabs{StartupTab(GURL("https://onboarding"), false)};
    return StartupTabs();
  }

then in the test functions we have things like:

  FakeStartupTabProvider provider(kOnboardingTabs | kResetTrigger | kPinnedTabs
| kPrefsTabs);

then it's clear in the test what is being done, and the impls of the various
provider functions are also quite straightforward.

+  base::CommandLine dummy(base::CommandLine::NO_PROGRAM);

[grt (UTC plus 2), 2016/09/09 11:40:30]

inline this in the call below rather than introducing a long-lived local var

+  StartupBrowserCreatorImpl impl(base::FilePath(), dummy,
+                                 chrome::startup::IS_FIRST_RUN);
+
+  StartupTabs output = impl.DetermineStartupTabs(&provider, false, false);
+  ASSERT_EQ(4U, output.size());
+  EXPECT_EQ("pinned", output[0].url.host());
+  EXPECT_EQ("onboarding", output[1].url.host());
+  EXPECT_EQ("reset-trigger", output[2].url.host());
+  EXPECT_EQ("prefs", output[3].url.host());
+}
+
+// All content is blocked in Incognito mode, or when recovering from a crash.
+// Only the New Tab Page should appear in either case.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_IncognitoOrCrash) {
+  MockStartupTabProvider provider;
+  PrepMockProvider(&provider, true, true, true, true, true);
+  base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
+  StartupBrowserCreatorImpl impl(base::FilePath(), dummy,
+                                 chrome::startup::IS_FIRST_RUN);
+
+  // Incognito
+  StartupTabs output = impl.DetermineStartupTabs(&provider, true, false);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL).host(), output[0].url.host());
+
+  // Crash Recovery
+  output = impl.DetermineStartupTabs(&provider, false, true);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL).host(), output[0].url.host());
+}
+
+// If Master Preferences specifies content, this should block all other
+// policies.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_MasterPrefs) {
+  MockStartupTabProvider provider;
+  PrepMockProvider(&provider, true, true, true, true, true);
+  base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
+  StartupBrowserCreatorImpl impl(base::FilePath(), dummy,
+                                 chrome::startup::IS_FIRST_RUN);
+
+  StartupTabs output = impl.DetermineStartupTabs(&provider, false, false);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ("master-prefs", output[0].url.host());
+}