MD Settings: Adding unit test for ResetSettingsHandler.

chrome/browser/ui/webui/settings/reset_settings_handler_unittest.cc

+// Copyright 2015 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 "base/values.h"
+#include "chrome/browser/profile_resetter/profile_resetter.h"
+#include "chrome/browser/ui/webui/settings/reset_settings_handler.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/browser/web_ui_data_source.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_web_ui.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using settings::ResetSettingsHandler;
+
+namespace {
+
+// Mock version of ProfileResetter to be used in tests.
+class MockProfileResetter : public ProfileResetter {
+ public:
+  explicit MockProfileResetter(TestingProfile* profile)
+      : ProfileResetter(profile) {
+  }
+
+  bool IsActive() const override {
+    return false;
+  }
+
+  void Reset(ResettableFlags resettable_flags,
+             scoped_ptr<BrandcodedDefaultSettings> master_settings,
+             const base::Closure& callback) override {
+    resets_++;
+    callback.Run();
+  }
+
+  int resets() const {

[Dan Beam, 2015/12/02 04:40:08]

size_t

[dpapad, 2015/12/02 19:55:22]

Done.

+    return resets_;
+  }
+
+ private:
+  size_t resets_ = 0;
+};
+

[Dan Beam, 2015/12/02 04:40:08]

remove extra \n

[dpapad, 2015/12/02 19:55:22]

Styleguide does not prohibit double blank lines for separating functions/classes
(https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace). I find
that useful since it helps visually parse a wall of text, by easily identifying
start/end of functions and classes (given that double blank lines do not appear
within a function).

[Dan Beam, 2015/12/02 20:08:36]

the style guide hurts your case by strongly encouraging you NOT to add extra
vertical whitespace.  this is not closure JS, this is Chrome's C++

[dpapad, 2015/12/02 21:33:35]

Removed per request, I don't mind that much either way.

For discussion's sake, I have found the double blank line useful (regardless of
language, since visual parsing of start/end of functions/classes applies to both
JS and C++).

This really boils down to what is considered "extra" whitespace. Specifically
the sentence "Of course, readability can suffer from code being too dense as
well as too spread out, so use your judgement. But in general, minimize use of
vertical whitespace". I interpret that as "use whitespace if you think it
improves readability". The styleguide wording also seems fairly mild, so I don't
interpret it as "strongly encourages NOT to add extra whitespace".

+
+class TestingResetSettingsHandler : public ResetSettingsHandler {
+ public:
+  TestingResetSettingsHandler(
+      TestingProfile* profile, bool allow_powerwash, content::WebUI* web_ui)
+      : ResetSettingsHandler(profile, allow_powerwash),
+        resetter_(profile) {
+    set_web_ui(web_ui);
+  }
+
+  ~TestingResetSettingsHandler() override {
+    set_web_ui(nullptr);

[Dan Beam, 2015/12/02 04:40:08]

why is this necessary?  destruction order issue?  if so: you could just do
handler_.reset() instead, which is better IMO

[dpapad, 2015/12/02 19:55:22]

Removed, it is not necessary, removed. It was there because I copied it from
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

+  }
+
+  MockProfileResetter* GetResetter() override {

[Dan Beam, 2015/12/02 04:40:08]

how is this override working if you're using MockProfileResetter* instead of
ProfileResetter*?

[dpapad, 2015/12/02 19:55:23]

Because C++ allows covariant return types, see example at
http://www.lwithers.me.uk/articles/covariant.html.

+    return &resetter_;
+  }
+
+  using settings::ResetSettingsHandler::HandleResetProfileSettings;
+
+ private:
+  MockProfileResetter resetter_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingResetSettingsHandler);
+};
+
+// The fixture for testing class Foo.
+class ResetSettingsHandlerTest : public testing::Test {
+ public:
+  ResetSettingsHandlerTest() : handler_(&profile_, false, &web_ui_) {
+  }
+
+  TestingResetSettingsHandler* handler() {
+    return &handler_;
+  }
+
+  content::TestWebUI* web_ui() {
+    return &web_ui_;
+  }
+
+ private:
+  // The order here matters.
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  content::TestWebUI web_ui_;
+  TestingResetSettingsHandler handler_;
+};
+

[Dan Beam, 2015/12/02 04:40:08]

remove extra \n

[dpapad, 2015/12/02 19:55:22]

Done.

+
+TEST_F(ResetSettingsHandlerTest, HandleResetProfileSettings) {
+  base::ListValue list;
+  list.AppendBoolean(false);
+  handler()->HandleResetProfileSettings(&list);
+  // Check that the delegate ProfileResetter was called.
+  EXPECT_EQ(1, handler()->GetResetter()->resets());

[Dan Beam, 2015/12/02 04:40:08]

nit: add

 public:
  size_t resets() const { return resetter_.resets(); }

to TestingResetSettingsHandler and check

  EXPECT_EQ(1, handler()->resets());

instead (reduces unneeded access)

[dpapad, 2015/12/02 19:55:22]

Done.

+  // Check that Javascript side is notified after resetting is done.
+  EXPECT_EQ("SettingsResetPage.doneResetting",
+            web_ui()->call_data()[0]->function_name());
+}
+

[Dan Beam, 2015/12/02 04:40:08]

remove extra \n

[dpapad, 2015/12/02 19:55:22]

Done.

+
+}  // namespace