Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FSSDK-10265] fix: UPS Lookup & Save during batched Decide #374

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
+233 −60
Open

[FSSDK-10265] fix: UPS Lookup & Save during batched Decide #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
963fc07
chore: updated during Rider upgrade
mikechu-optimizely Oct 4, 2024
d0f59b6
feat: call UPS once during batch decideForKeys or decideAll
mikechu-optimizely Oct 4, 2024
14dface
revert: SaveVariation to include userProfile
mikechu-optimizely Oct 4, 2024
66708d8
doc: fix param
mikechu-optimizely Oct 4, 2024
aa0d018
Merge branch 'master' into mike/FSSDK-10265-ups-during-batch-decide
mikechu-optimizely Oct 4, 2024
dd072b9
test: fix hacked tests
mikechu-optimizely Oct 4, 2024
f9f3f3f
revert: auto-formatting
mikechu-optimizely Oct 4, 2024
b43e10f
refactor: SaveToUserProfileService for simplicity
mikechu-optimizely Oct 4, 2024
0dd1b74
revert: formatting
mikechu-optimizely Oct 4, 2024
2b17438
bug: only SaveToUserProfileService if batch is finishing
mikechu-optimizely Oct 7, 2024
c9047a0
test: WIP add coverage for single `Lookup` & `Save` via USP
mikechu-optimizely Oct 7, 2024
dfe1367
refactor: less movement of code as the ref implementation
mikechu-optimizely Oct 8, 2024
57c1ccd
Revert "refactor: less movement of code as the ref implementation"
mikechu-optimizely Oct 8, 2024
9ca271c
test: finish coverage
mikechu-optimizely Oct 8, 2024
c97f36e
Update OptimizelySDK.Tests/OptimizelyUserContextTest.cs
mikechu-optimizely Oct 14, 2024
5b7e5ae
fix: reset _userProfile after batch
mikechu-optimizely Oct 16, 2024
c6889e8
test: add content verification of UPS Save()
mikechu-optimizely Oct 16, 2024
2f46b18
Merge remote-tracking branch 'origin/mike/FSSDK-10265-ups-during-batc…
mikechu-optimizely Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions OptimizelySDK.Tests/OptimizelyTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,19 +1394,19 @@ public void TestForcedVariationPreceedsUserProfile()
LoggerMock.Verify(
l => l.Log(LogLevel.INFO,
"No previously activated variation of experiment \"etag1\" for user \"testUser3\" found in user profile."),
Times.Exactly(2));
Times.Once);
LoggerMock.Verify(
l => l.Log(LogLevel.DEBUG,
"Assigned bucket [4969] to user [testUser3] with bucketing ID [testUser3]."),
Times.Exactly(2));
Times.Once);
LoggerMock.Verify(
l => l.Log(LogLevel.INFO,
"User [testUser3] is in variation [vtag2] of experiment [etag1]."),
Times.Exactly(2));
Times.Once);
LoggerMock.Verify(
l => l.Log(LogLevel.INFO,
"Saved variation \"277\" of experiment \"223\" for user \"testUser3\"."),
Times.Exactly(2));
Times.Once);
LoggerMock.Verify(
l => l.Log(LogLevel.DEBUG,
"Set variation \"276\" for experiment \"223\" and user \"testUser3\" in the forced variation map."),
Expand Down
123 changes: 118 additions & 5 deletions OptimizelySDK.Tests/OptimizelyUserContextTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2020-2021, 2022-2023 Optimizely and contributors
* Copyright 2020-2024 Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,7 @@

using System;
using System.Collections.Generic;
using System.Threading;
using System.Linq;
using Castle.Core.Internal;
using Moq;
using NUnit.Framework;
Expand Down Expand Up @@ -62,6 +62,22 @@ public void SetUp()
LoggerMock.Object, ErrorHandlerMock.Object);
}

private Mock<UserProfileService> MakeUserProfileServiceMock()
{
var projectConfig = DatafileProjectConfig.Create(TestData.Datafile, LoggerMock.Object,
ErrorHandlerMock.Object);
var experiment = projectConfig.Experiments[8];
var variation = experiment.Variations[0];
var decision = new Decision(variation.Id);
var userProfile = new UserProfile(UserID, new Dictionary<string, Decision>
{
{ experiment.Id, decision },
});
var userProfileServiceMock = new Mock<UserProfileService>();
userProfileServiceMock.Setup(up => up.Lookup(UserID)).Returns(userProfile.ToMap());
return userProfileServiceMock;
}

[Test]
public void OptimizelyUserContextWithAttributes()
{
Expand Down Expand Up @@ -193,7 +209,7 @@ public void SetAttributeToOverrideAttribute()
Assert.AreEqual(user.GetAttributes()["k1"], true);
}

#region decide
#region Decide

[Test]
public void TestDecide()
Expand Down Expand Up @@ -409,9 +425,71 @@ public void DecideWhenConfigIsNull()
Assert.IsTrue(TestData.CompareObjects(decision, decisionExpected));
}

#endregion decide
[Test]
public void DecideWithUpsShouldOnlyLookupSaveOnce()
{
var flagKeyFromTestDataJson = "double_single_variable_feature";
var userProfileServiceMock = MakeUserProfileServiceMock();
var saveArgsCollector = new List<Dictionary<string, object>>();
userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector)));
var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object,
LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object);
var user = optimizely.CreateUserContext(UserID);
var expectedUserProfile = new UserProfile(UserID, new Dictionary<string, Decision>
{
{ "224", new Decision("280") },
{ "122238", new Decision("122240") },
});

_ = user.Decide(flagKeyFromTestDataJson);

LoggerMock.Verify(
l => l.Log(LogLevel.INFO,
"We were unable to get a user profile map from the UserProfileService."),
Times.Never);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."),
Times.Never);
userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once);
userProfileServiceMock.Verify(l => l.Save(It.IsAny<Dictionary<string, object>>()),
Times.Once);
Assert.AreEqual(saveArgsCollector.First(), expectedUserProfile.ToMap());
}

#endregion Decide

#region decideAll
#region DecideForKeys

[Test]
public void DecideForKeysWithUpsShouldOnlyLookupSaveOnceWithMultipleFlags()
{
var flagKeys = new[] { "double_single_variable_feature", "boolean_feature" };
var userProfileServiceMock = MakeUserProfileServiceMock();
var saveArgsCollector = new List<Dictionary<string, object>>();
userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector)));
var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object,
LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object);
var userContext = optimizely.CreateUserContext(UserID);
var expectedUserProfile = new UserProfile(UserID, new Dictionary<string, Decision>
{
{ "224", new Decision("280") },
{ "122238", new Decision("122240") },
});

_ = userContext.DecideForKeys(flagKeys);

LoggerMock.Verify(
l => l.Log(LogLevel.INFO,
"We were unable to get a user profile map from the UserProfileService."),
Times.Never);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."),
Times.Never);
userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once);
userProfileServiceMock.Verify(l => l.Save(It.IsAny<Dictionary<string, object>>()),
Times.Once);
Assert.AreEqual(saveArgsCollector.First(), expectedUserProfile.ToMap());
}

[Test]
public void DecideForKeysWithOneFlag()
Expand Down Expand Up @@ -443,6 +521,41 @@ public void DecideForKeysWithOneFlag()
Assert.IsTrue(TestData.CompareObjects(decision, expDecision));
}

#endregion DecideForKeys

#region DecideAll

[Test]
public void DecideAllWithUpsShouldOnlyLookupSaveOnce()
{
var userProfileServiceMock = MakeUserProfileServiceMock();
var saveArgsCollector = new List<Dictionary<string, object>>();
userProfileServiceMock.Setup(up => up.Save(Capture.In(saveArgsCollector)));
var optimizely = new Optimizely(TestData.Datafile, EventDispatcherMock.Object,
LoggerMock.Object, ErrorHandlerMock.Object, userProfileServiceMock.Object);
var user = optimizely.CreateUserContext(UserID);
var expectedUserProfile = new UserProfile(UserID, new Dictionary<string, Decision>
{
{ "224", new Decision("280") },
{ "122238", new Decision("122240") },
{ "122241", new Decision("122242") },
{ "122235", new Decision("122236") },
});

_ = user.DecideAll();

LoggerMock.Verify(
l => l.Log(LogLevel.INFO,
"We were unable to get a user profile map from the UserProfileService."),
Times.Never);
LoggerMock.Verify(
l => l.Log(LogLevel.ERROR, "The UserProfileService returned an invalid map."),
Times.Never);
userProfileServiceMock.Verify(l => l.Lookup(UserID), Times.Once);
userProfileServiceMock.Verify(l => l.Save(It.IsAny<Dictionary<string,object>>()), Times.Once);
Assert.AreEqual(saveArgsCollector.First(), expectedUserProfile.ToMap());
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests look good. Wondering if we have tests covering the contents of the final UPS save call, combining all decisions in the session.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also have the similar coverage (lookup once - save once, the ups save contents) with legacy APIs (activate, getVariation) for regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added coverage of the final UPS Save as requested.

I'm researching the Activate and GetVariation tests where UPS is used.

[Test]
public void DecideAllTwoFlag()
{
Expand Down
5 changes: 5 additions & 0 deletions OptimizelySDK.sln.DotSettings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,15 @@
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=Interfaces/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=LocalConstants/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/PredefinedNamingRules/=PrivateStaticReadonly/@EntryIndexedValue">&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=15b5b1f1_002D457c_002D4ca6_002Db278_002D5615aedc07d3/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static readonly fields (private)"&gt;&lt;ElementKinds&gt;&lt;Kind Name="READONLY_FIELD" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="aaBb" /&gt;&lt;/Policy&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=8b8504e3_002Df0be_002D4c14_002D9103_002Dc732f2bddc15/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Enum members"&gt;&lt;ElementKinds&gt;&lt;Kind Name="ENUM_MEMBER" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AaBb"&gt;&lt;ExtraRule Prefix="" Suffix="" Style="AaBb" /&gt;&lt;/Policy&gt;&lt;/Policy&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=a4f433b8_002Dabcd_002D4e55_002Da08f_002D82e78cef0f0c/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Local constants"&gt;&lt;ElementKinds&gt;&lt;Kind Name="LOCAL_CONSTANT" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="" Suffix="" Style="AA_BB" /&gt;&lt;/Policy&gt;</s:String>
<s:String x:Key="/Default/CodeStyle/Naming/CSharpNaming/UserRules/=a7a3339e_002D4e89_002D4319_002D9735_002Da9dc4cb74cc7/@EntryIndexedValue">&lt;Policy&gt;&lt;Descriptor Staticness="Any" AccessRightKinds="Any" Description="Interfaces"&gt;&lt;ElementKinds&gt;&lt;Kind Name="INTERFACE" /&gt;&lt;/ElementKinds&gt;&lt;/Descriptor&gt;&lt;Policy Inspect="True" Prefix="I" Suffix="" Style="AaBb" /&gt;&lt;/Policy&gt;</s:String>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpKeepExistingMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateBlankLinesAroundFieldToBlankLinesAroundProperty/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EPredefinedNamingRulesToUserRulesUpgrade/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Bucketer/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=ODP_0027s/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Optly/@EntryIndexedValue">True</s:Boolean>
Expand Down
Loading
Loading