Use improved VariationParamsManager to hide details.

components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc

Index: components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
index c02ffa4753d5056f26eddc47663323831c844361..0d0eec64d0d18f01d82f46478c6e3905134d6475 100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
@@ -16,6 +16,7 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
 #include "components/ntp_snippets/user_classifier.h"
@@ -197,28 +198,19 @@ void ParseJsonDelayed(
 
 class NTPSnippetsFetcherTest : public testing::Test {
  public:
-  // TODO(fhorschig): As soon as crbug.com/645447 is resolved, use
-  // variations::testing::VariationParamsManager to configure these values.
-  class RequestBuilderWithMockedFlagsForTesting
-      : public NTPSnippetsFetcher::RequestBuilder {
-   public:
-    RequestBuilderWithMockedFlagsForTesting() : RequestBuilder() {}
-
-   private:
-    bool IsSendingUserClassEnabled() const override { return true; }
-    bool IsSendingTopLanguagesEnabled() const override { return true; }
-  };
-
   NTPSnippetsFetcherTest()

[tschumann, 2016/12/06 17:58:16]

nit: As you're touching this code, can we clean up the class hierarchy?

I'm not a huge fan of subclassing tests; in this case it's actually easy enough,
assuming we make that clear.
We should rename this class to NTPSnippetsFetcherTestBase (we can delete the
default constructor on that one) and then have two very simple subclasses,
NTPSnippetsFetcherTest (bonus points for renaming to something like
ChromeReaderSnippetsFetcherTest) and NTPSnippetsContentSuggestionsFetcherTest.

[fhorschig, 2016/12/12 14:16:37]

Makes sense. Done.

-      : NTPSnippetsFetcherTest(GURL(kTestChromeReaderUrl),
-                               std::map<std::string, std::string>()) {}
+      : NTPSnippetsFetcherTest(
+            GURL(kTestChromeReaderUrl),
+            {{"send_top_languages", "true"}, {"send_user_class", "true"}}) {}
 
   NTPSnippetsFetcherTest(const GURL& gurl,
                          const std::map<std::string, std::string>& params)
       : params_manager_(
             base::MakeUnique<variations::testing::VariationParamsManager>(
                 ntp_snippets::kStudyName,
-                params)),
+                params,
+                std::set<std::string>(

[jkrcal, 2016/12/06 16:50:47]

Do you need to wrap the initializer list with "std::set<std::string>()"?

[fhorschig, 2016/12/12 14:16:37]

For the unique_ptr: Sadly, yes.

[jkrcal, 2016/12/12 14:27:14]

Interesting :)

+                    {ntp_snippets::kArticleSuggestionsFeature.name}))),
         mock_task_runner_(new base::TestMockTimeTaskRunner()),
         mock_task_runner_handle_(mock_task_runner_),
         signin_client_(base::MakeUnique<TestSigninClient>(nullptr)),
@@ -530,7 +522,7 @@ TEST_F(NTPSnippetsFetcherTest, BuildRequestNoUserClass) {
 }
 
 TEST_F(NTPSnippetsFetcherTest, BuildRequestWithTwoLanguages) {
-  RequestBuilderWithMockedFlagsForTesting builder;
+  NTPSnippetsFetcher::RequestBuilder builder;
   std::unique_ptr<translate::LanguageModel> language_model =
       MakeLanguageModel({"de", "en"});
   NTPSnippetsFetcher::Params params = test_params();
@@ -561,7 +553,7 @@ TEST_F(NTPSnippetsFetcherTest, BuildRequestWithTwoLanguages) {
 }
 
 TEST_F(NTPSnippetsFetcherTest, BuildRequestWithUILanguageOnly) {
-  RequestBuilderWithMockedFlagsForTesting builder;
+  NTPSnippetsFetcher::RequestBuilder builder;
   std::unique_ptr<translate::LanguageModel> language_model =
       MakeLanguageModel({"en"});
   NTPSnippetsFetcher::Params params = test_params();