New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 672010 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

variations::testing::VariationParamsManager does not set values after clearing inital values

Project Member Reported by fhorschig@chromium.org, Dec 7 2016

Issue description

The method |SetVariationParamsWithFeatureAssociations| does not set parameters after calling |ClearAllVariationParams|.

(1) Call |SetVariationParamsWithFeatureAssociations| with params {"blubb", "true"}. "blubb" is set.
(2) Call |ClearAllVariation{Param,ID}s| to delete all variation. --> "blubb" is unset.
(3) Call |SetVariationParamsWithFeatureAssociations| with params {"blubb", "true"}. "blubb" is still unset.


What is the expected result?
The values are set to the requested values.


What happens instead?
Nothing. It doesn't crash or show any errors, the values just stay undefined.


How you can work around this issue:
Destroy the instance of the VariationParamsManager completely and create a new instance.


How |SetFetchingPersonalization| in /components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc [1] should work: (expectations optional)

void SetFetchingPersonalizationVariation(
    const std::string& personalization_string) {
  EXPECT_THAT(variations::GetVariationParamValue(ntp_snippets::kStudyName,
                                                 "send_top_languages"),
              Eq("true"));
  EXPECT_THAT(variations::GetVariationParamValue(ntp_snippets::kStudyName,
                                                 "fetching_personalization"),
              Eq(""));

  params_manager_.ClearAllVariationIDs();    
  params_manager_.ClearAllVariationParams();

  std::map<std::string, std::string> params = GetDefaultVariationParameters();
  params["fetching_personalization"] = personalization_string;

  params_manager_.SetVariationParamsWithFeatureAssociations(
      ntp_snippets::kStudyName, params,
      {ntp_snippets::kArticleSuggestionsFeature.name});

  EXPECT_THAT(variations::GetVariationParamValue(ntp_snippets::kStudyName,
                                                 "send_top_languages"),
              Eq("true"));
  EXPECT_THAT(variations::GetVariationParamValue(ntp_snippets::kStudyName,
                                                 "fetching_personalization"),
              Eq("personalization_string"));
}

[1] This file might be named /components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc by now.

 
Status: Started (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0bf6128f18eedd05c1525c09d258df0080e8583

commit e0bf6128f18eedd05c1525c09d258df0080e8583
Author: fhorschig <fhorschig@chromium.org>
Date: Tue Dec 20 16:53:06 2016

Make FieldTrialList usable after clearing VariationsParamsManager.

Calling |ClearAllVariationParams| will now create a new instance of the
FieldTrialList so |SetVariationParamsWithFeatureAssociations| will
work again.

BUG= 672010 

Review-Url: https://codereview.chromium.org/2590883002
Cr-Commit-Position: refs/heads/master@{#439824}

[modify] https://crrev.com/e0bf6128f18eedd05c1525c09d258df0080e8583/components/variations/variations_params_manager.cc

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08db2becc667bf907330d414f372f4c297b8a54f

commit 08db2becc667bf907330d414f372f4c297b8a54f
Author: fhorschig <fhorschig@chromium.org>
Date: Tue Dec 20 17:46:42 2016

NTP: VariationParamsManager resetting works, so make it a member.

This CL makes use of the resetting function VariationParamsManager.
This should work now and therefore it is sufficient to hold the manager
as a simple member.

BUG= 672010 

Review-Url: https://codereview.chromium.org/2592663003
Cr-Commit-Position: refs/heads/master@{#439839}

[modify] https://crrev.com/08db2becc667bf907330d414f372f4c297b8a54f/components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc

Sign in to add a comment