Use improved VariationParamsManager to hide details.

components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc

 // 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 "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
 
 #include <map>
 #include <utility>
 
 #include "base/json/json_reader.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #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"
 #include "components/prefs/testing_pref_service.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
 #include "components/signin/core/browser/fake_signin_manager.h"
 #include "components/signin/core/browser/test_signin_client.h"
 #include "components/variations/entropy_provider.h"
 #include "components/variations/variations_params_manager.h"
@@ skipping 161 matching lines @@
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, base::Bind(&ParseJson, json, std::move(success_callback),
                             std::move(error_callback)),
       base::TimeDelta::FromMilliseconds(kTestJsonParsingLatencyMs));
 }
 
 }  // namespace
 
 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)),
         account_tracker_(base::MakeUnique<AccountTrackerService>()),
         fake_signin_manager_(
             base::MakeUnique<FakeSigninManagerBase>(signin_client_.get(),
                                                     account_tracker_.get())),
         fake_token_service_(base::MakeUnique<FakeProfileOAuth2TokenService>()),
         pref_service_(base::MakeUnique<TestingPrefServiceSimple>()),
         test_url_(gurl) {
@@ skipping 76 matching lines @@
         language_model->OnPageVisited(code);
       }
     }
     return language_model;
   }
 
   TestingPrefServiceSimple* pref_service() const { return pref_service_.get(); }
 
  private:
   // TODO(fhorschig): Make it a simple member as soon as it resets properly.
   std::unique_ptr<variations::testing::VariationParamsManager> params_manager_;

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

What do you mean by "resets properly"? Cannot you call ClearAllVariationParams()
and SetVariationParamsWithFeatureAssociations() in
SetFetchingPersonalizationVariation()?

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

Thanks for pointing this out! This is the big vision after crbug.com/672010 is
fixed. Currently, I have to destroy the manager.

   scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_;
   base::ThreadTaskRunnerHandle mock_task_runner_handle_;
   FailingFakeURLFetcherFactory failing_url_fetcher_factory_;
   // Initialized lazily in SetFakeResponse().
   std::unique_ptr<net::FakeURLFetcherFactory> fake_url_fetcher_factory_;
   std::unique_ptr<TestSigninClient> signin_client_;
   std::unique_ptr<AccountTrackerService> account_tracker_;
   std::unique_ptr<SigninManagerBase> fake_signin_manager_;
   std::unique_ptr<OAuth2TokenService> fake_token_service_;
   std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher_;
@@ skipping 194 matching lines @@
 
   EXPECT_THAT(builder.PreviewRequestBodyForTesting(),
               EqualsJSON("{"
                          "  \"regularlyVisitedHostNames\": [],"
                          "  \"priority\": \"BACKGROUND_PREFETCH\","
                          "  \"excludedSuggestionIds\": []"
                          "}"));
 }
 
 TEST_F(NTPSnippetsFetcherTest, BuildRequestWithTwoLanguages) {
-  RequestBuilderWithMockedFlagsForTesting builder;
+  NTPSnippetsFetcher::RequestBuilder builder;
   std::unique_ptr<translate::LanguageModel> language_model =
       MakeLanguageModel({"de", "en"});
   NTPSnippetsFetcher::Params params = test_params();
   params.language_code = "en";
   builder.SetParams(params)
       .SetLanguageModel(language_model.get())
       .SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal)
       .SetFetchAPI(
           NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API);
 
@@ skipping 10 matching lines @@
                          "    },"
                          "    {"
                          "      \"language\" : \"de\","
                          "      \"frequency\" : 0.5"
                          "    }"
                          "  ]"
                          "}"));
 }
 
 TEST_F(NTPSnippetsFetcherTest, BuildRequestWithUILanguageOnly) {
-  RequestBuilderWithMockedFlagsForTesting builder;
+  NTPSnippetsFetcher::RequestBuilder builder;
   std::unique_ptr<translate::LanguageModel> language_model =
       MakeLanguageModel({"en"});
   NTPSnippetsFetcher::Params params = test_params();
   params.language_code = "en";
   builder.SetParams(params)
       .SetLanguageModel(language_model.get())
       .SetPersonalization(NTPSnippetsFetcher::Personalization::kNonPersonal)
       .SetFetchAPI(
           NTPSnippetsFetcher::FetchAPI::CHROME_CONTENT_SUGGESTIONS_API);
 
@@ skipping 513 matching lines @@
     const NTPSnippetsFetcher::OptionalFetchedCategories& fetched_categories) {
   if (fetched_categories) {
     // Matchers above aren't any more precise than this, so this is sufficient
     // for test-failure diagnostics.
     return os << "list with " << fetched_categories->size() << " elements";
   }
   return os << "null";
 }
 
 }  // namespace ntp_snippets