Skip to content

Commit af7a465

Browse files
joelverhagenjoelverhagen
authored
Remove A/B test check for the package dependents feature (#8251)
Address #8250
1 parent 53729e8 commit af7a465

7 files changed

Lines changed: 38 additions & 111 deletions

File tree

‎src/NuGetGallery/Controllers/PackagesController.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -880,8 +880,7 @@ public virtual async Task<ActionResult> DisplayPackage(string id, string version
880880
model.IsAtomFeedEnabled = _featureFlagService.IsPackagesAtomFeedEnabled();
881881
model.IsPackageDeprecationEnabled = _featureFlagService.IsManageDeprecationEnabled(currentUser, allVersions);
882882
model.IsPackageRenamesEnabled = _featureFlagService.IsPackageRenamesEnabled(currentUser);
883-
model.IsPackageDependentsEnabled = _featureFlagService.IsPackageDependentsEnabled(currentUser) &&
884-
_abTestService.IsPackageDependendentsABEnabled(currentUser);
883+
model.IsPackageDependentsEnabled = _featureFlagService.IsPackageDependentsEnabled(currentUser);
885884

886885
if (model.IsPackageDependentsEnabled)
887886
{

‎src/NuGetGallery/Infrastructure/ABTestEnrollment.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,21 @@ public ABTestEnrollment(ABTestEnrollmentState state, int schemaVersion, int prev
2727
PreviewSearchBucket = previewSearchBucket;
2828
PackageDependentBucket = packageDependentBucket;
2929
}
30+
3031
public ABTestEnrollmentState State { get; }
3132
public int SchemaVersion { get; }
33+
34+
/// <summary>
35+
/// Currently usabled and is represented by the "ps" property in the "nugetab" cookie.
36+
/// </summary>
3237
public int PreviewSearchBucket { get; }
38+
39+
/// <summary>
40+
/// Currently unused by the <see cref="CookieBasedABTestService"/> but still set in some user's cookies. See
41+
/// <see cref="ABTestEnrollmentFactory"/> for the A/B test cookie data model. This C# property could be renamed
42+
/// and reused for a future A/B test by leveraging the "pd" JSON property in the "nugetab" cookie or could be
43+
/// ignored for the rest of time.
44+
/// </summary>
3345
public int PackageDependentBucket { get; }
3446
}
3547
}

‎src/NuGetGallery/Infrastructure/ABTestEnrollmentFactory.cs‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,28 +4,32 @@
44
using System;
55
using System.Security.Cryptography;
66
using System.Threading;
7-
using Microsoft.Extensions.Logging;
87
using Newtonsoft.Json;
98

109
namespace NuGetGallery
1110
{
11+
/// <summary>
12+
/// This is the implementation that handles all current and past versions of the "nugetab" cookie. This, along
13+
/// with <see cref="CookieBasedABTestService"/>, makes the A/B test settings per browsing session "sticky" so that
14+
/// an A/B test feature is always on or off for a single user browser.
15+
/// </summary>
1216
public class ABTestEnrollmentFactory : IABTestEnrollmentFactory
1317
{
14-
private const int SchemaVersion1 = 1;
15-
private const int SchemaVersion2 = 2;
18+
private const int SchemaVersion1 = 1; // PreviewSearch: {"v":1,"ps":100}
19+
private const int SchemaVersion2 = 2; // PreviewSearch + PackageDependent: {"v":2,"ps":100,"pd":100}
20+
21+
// Note that a new schema version could theoretically reuse any currently unused cookie properties. However
22+
// this does have questionable statistical correctness due treatment assignment of one A/B test being reused
23+
// for another, i.e. each A/B test population is not independent.
1624

1725
private static readonly RNGCryptoServiceProvider _secureRng = new RNGCryptoServiceProvider();
1826
private static readonly ThreadLocal<byte[]> _bytes = new ThreadLocal<byte[]>(() => new byte[sizeof(ulong)]);
1927

2028
private readonly ITelemetryService _telemetryService;
21-
private readonly ILogger<ABTestEnrollmentFactory> _logger;
2229

23-
public ABTestEnrollmentFactory(
24-
ITelemetryService telemetryService,
25-
ILogger<ABTestEnrollmentFactory> logger)
30+
public ABTestEnrollmentFactory(ITelemetryService telemetryService)
2631
{
2732
_telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
28-
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
2933
}
3034

3135
public ABTestEnrollment Initialize()
@@ -85,7 +89,8 @@ public bool TryDeserialize(string serialized, out ABTestEnrollment enrollment)
8589
return false;
8690
}
8791

88-
return TryDeserializeStateVer2(serialized, out enrollment) || TryDeserializeStateVer1(serialized, out enrollment);
92+
return TryDeserializeStateVer2(serialized, out enrollment)
93+
|| TryDeserializeStateVer1(serialized, out enrollment);
8994
}
9095

9196
private bool TryDeserializeStateVer1(string serialized, out ABTestEnrollment enrollment)
@@ -174,4 +179,4 @@ private class StateVersion2
174179
public int PackageDependentBucket { get; set; }
175180
}
176181
}
177-
}
182+
}

‎src/NuGetGallery/Infrastructure/CookieBasedABTestService.cs‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,17 +47,6 @@ public bool IsPreviewSearchEnabled(User user)
4747
config => config.PreviewSearchPercentage);
4848
}
4949

50-
public bool IsPackageDependendentsABEnabled(User user)
51-
{
52-
var isActive = IsActive(
53-
nameof(Enrollment.PackageDependentBucket),
54-
user,
55-
enrollment => enrollment.PackageDependentBucket,
56-
config => config.DependentsPercentage);
57-
58-
return isActive;
59-
}
60-
6150
private ABTestEnrollment Enrollment => _lazyEnrollment.Value;
6251
private IABTestConfiguration Config => _contentObjectService.ABTestConfiguration;
6352

‎src/NuGetGallery/Infrastructure/IABTestService.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,5 @@ public interface IABTestService
1111
/// Whether or not the user should see the preview search implementation.
1212
/// </summary>
1313
bool IsPreviewSearchEnabled(User user);
14-
15-
/// <summary>
16-
/// Whether or not the user should see the reverse dependencies implementation.
17-
/// </summary>
18-
bool IsPackageDependendentsABEnabled(User user);
1914
}
2015
}

‎tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs‎

Lines changed: 9 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
using System.Linq.Expressions;
1212
using System.Net;
1313
using System.Text;
14-
using System.Threading;
1514
using System.Threading.Tasks;
1615
using System.Web;
1716
using System.Web.Caching;
@@ -45,7 +44,6 @@
4544
using NuGetGallery.Services.Helpers;
4645
using NuGetGallery.TestUtils;
4746
using Xunit;
48-
using Xunit.Sdk;
4947

5048
namespace NuGetGallery
5149
{
@@ -187,6 +185,9 @@ private static PackagesController CreateController(
187185
contentObjectService
188186
.SetupGet(c => c.GitHubUsageConfiguration)
189187
.Returns(new GitHubUsageConfiguration(Array.Empty<RepositoryInformation>()));
188+
contentObjectService
189+
.SetupGet(c => c.CacheConfiguration)
190+
.Returns(new CacheConfiguration());
190191
}
191192

192193
if (symbolPackageUploadService == null)
@@ -1658,69 +1659,21 @@ public async Task CheckFeatureFlagIsOff()
16581659
}
16591660

16601661
[Fact]
1661-
public async Task CheckABTestIsOff()
1662-
{
1663-
var id = "foo";
1664-
var cacheKey = "CacheDependents_" + id.ToLowerInvariant();
1665-
var packageService = new Mock<IPackageService>();
1666-
var abTestService = new Mock<IABTestService>();
1667-
1668-
abTestService
1669-
.Setup(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()))
1670-
.Returns(false);
1671-
1672-
var controller = CreateController(
1673-
GetConfigurationService(),
1674-
packageService: packageService,
1675-
abTestService: abTestService);
1676-
1677-
var package = new Package
1678-
{
1679-
PackageRegistration = new PackageRegistration()
1680-
{
1681-
Id = id,
1682-
Owners = new List<User>(),
1683-
},
1684-
Version = "2.0.0",
1685-
NormalizedVersion = "2.0.0",
1686-
};
1687-
1688-
var packages = new List<Package> { package };
1689-
packageService
1690-
.Setup(p => p.FindPackagesById(id, /*includePackageRegistration:*/ true))
1691-
.Returns(packages);
1692-
packageService
1693-
.Setup(p => p.FilterLatestPackage(It.IsAny<IReadOnlyCollection<Package>>(), It.IsAny<int?>(), true))
1694-
.Returns(package);
1695-
1696-
var result = await controller.DisplayPackage(id, version: null);
1697-
var model = ResultAssert.IsView<DisplayPackageViewModel>(result);
1698-
1699-
Assert.Null(model.PackageDependents);
1700-
packageService
1701-
.Verify(iup => iup.GetPackageDependents(It.IsAny<string>()), Times.Never());
1702-
Assert.False(model.IsPackageDependentsEnabled);
1703-
Assert.Empty(_cache);
1704-
}
1705-
1706-
[Fact]
1707-
public async Task CheckABTestIsOnAndFeatFlagIsOff()
1662+
public async Task CheckPackageDependentsFeatureFlagIsOff()
17081663
{
17091664
var id = "foo";
17101665
var cacheKey = "CacheDependents_" + id.ToLowerInvariant();
17111666
var packageService = new Mock<IPackageService>();
1712-
var abTestService = new Mock<IABTestService>();
17131667
var featureFlagService = new Mock<IFeatureFlagService>();
17141668

1715-
abTestService
1716-
.Setup(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()))
1717-
.Returns(false);
1718-
17191669
var controller = CreateController(
17201670
GetConfigurationService(),
17211671
packageService: packageService,
1722-
featureFlagService: featureFlagService,
1723-
abTestService: abTestService);
1672+
featureFlagService: featureFlagService);
1673+
1674+
featureFlagService
1675+
.Setup(x => x.IsPackageDependentsEnabled(It.IsAny<User>()))
1676+
.Returns(false);
17241677

17251678
var package = new Package
17261679
{
@@ -1750,8 +1703,6 @@ public async Task CheckABTestIsOnAndFeatFlagIsOff()
17501703
Assert.Null(model.PackageDependents);
17511704
packageService
17521705
.Verify(iup => iup.GetPackageDependents(It.IsAny<string>()), Times.Never());
1753-
abTestService
1754-
.Verify(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()), Times.Never);
17551706
Assert.False(model.IsPackageDependentsEnabled);
17561707
Assert.Empty(_cache);
17571708
}
@@ -1762,14 +1713,9 @@ public async Task WhenCacheIsOccupiedGetProperPackageDependent()
17621713
var id = "foo";
17631714
var cacheKey = "CacheDependents_" + id.ToLowerInvariant();
17641715
var packageService = new Mock<IPackageService>();
1765-
var abTestService = new Mock<IABTestService>();
17661716
var httpContext = new Mock<HttpContextBase>();
17671717
PackageDependents pd = new PackageDependents();
17681718

1769-
abTestService
1770-
.Setup(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()))
1771-
.Returns(true);
1772-
17731719
httpContext
17741720
.Setup(c => c.Cache)
17751721
.Returns(_cache);
@@ -1784,7 +1730,6 @@ public async Task WhenCacheIsOccupiedGetProperPackageDependent()
17841730
var controller = CreateController(
17851731
GetConfigurationService(),
17861732
packageService: packageService,
1787-
abTestService: abTestService,
17881733
httpContext: httpContext);
17891734

17901735
var package = new Package
@@ -1820,7 +1765,6 @@ public async Task OccupyEmptyCache()
18201765
var id = "foo";
18211766
var cacheKey = "CacheDependents_" + id.ToLowerInvariant();
18221767
var packageService = new Mock<IPackageService>();
1823-
var abTestService = new Mock<IABTestService>();
18241768
var httpContext = new Mock<HttpContextBase>();
18251769
var contentObjectService = new Mock<IContentObjectService>();
18261770

@@ -1830,9 +1774,6 @@ public async Task OccupyEmptyCache()
18301774
};
18311775
PackageDependents pd = new PackageDependents();
18321776

1833-
abTestService
1834-
.Setup(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()))
1835-
.Returns(true);
18361777
httpContext
18371778
.Setup(c => c.Cache)
18381779
.Returns(_cache);
@@ -1843,7 +1784,6 @@ public async Task OccupyEmptyCache()
18431784
var controller = CreateController(
18441785
GetConfigurationService(),
18451786
packageService: packageService,
1846-
abTestService: abTestService,
18471787
contentObjectService: contentObjectService,
18481788
httpContext: httpContext);
18491789

@@ -1889,7 +1829,6 @@ public async Task CheckThatCacheKeyIsNotCaseSensitive()
18891829
var id2 = "FOObAr";
18901830
var cacheKey = "CacheDependents_foobar";
18911831
var packageService = new Mock<IPackageService>();
1892-
var abTestService = new Mock<IABTestService>();
18931832
var contentObjectService = new Mock<IContentObjectService>();
18941833
var httpContext = new Mock<HttpContextBase>();
18951834

@@ -1902,17 +1841,13 @@ public async Task CheckThatCacheKeyIsNotCaseSensitive()
19021841
contentObjectService
19031842
.Setup(c => c.CacheConfiguration)
19041843
.Returns(cacheConfiguration);
1905-
abTestService
1906-
.Setup(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()))
1907-
.Returns(true);
19081844
httpContext
19091845
.Setup(c => c.Cache)
19101846
.Returns(_cache);
19111847

19121848
var controller = CreateController(
19131849
GetConfigurationService(),
19141850
packageService: packageService,
1915-
abTestService: abTestService,
19161851
contentObjectService: contentObjectService,
19171852
httpContext: httpContext);
19181853

@@ -1960,15 +1895,10 @@ public async Task OneSecondCacheTime()
19601895
var id = "foo";
19611896
var cacheKey = "CacheDependents_" + id.ToLowerInvariant();
19621897
var packageService = new Mock<IPackageService>();
1963-
var abTestService = new Mock<IABTestService>();
19641898
var contentObjectService = new Mock<IContentObjectService>();
19651899
var httpContext = new Mock<HttpContextBase>();
19661900
var pd = new PackageDependents();
19671901

1968-
abTestService
1969-
.Setup(x => x.IsPackageDependendentsABEnabled(It.IsAny<User>()))
1970-
.Returns(true);
1971-
19721902
httpContext
19731903
.Setup(c => c.Cache)
19741904
.Returns(_cache);
@@ -1985,7 +1915,6 @@ public async Task OneSecondCacheTime()
19851915
var controller = CreateController(
19861916
GetConfigurationService(),
19871917
packageService: packageService,
1988-
abTestService: abTestService,
19891918
contentObjectService: contentObjectService);
19901919

19911920
var package = new Package

‎tests/NuGetGallery.Facts/Infrastructure/ABTestEnrollmentFactoryFacts.cs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,7 @@ public abstract class Facts
133133
public Facts()
134134
{
135135
TelemetryService = new Mock<ITelemetryService>();
136-
Logger = new Mock<ILogger<ABTestEnrollmentFactory>>();
137-
138-
Target = new ABTestEnrollmentFactory(TelemetryService.Object, Logger.Object);
136+
Target = new ABTestEnrollmentFactory(TelemetryService.Object);
139137
}
140138

141139
public Mock<ITelemetryService> TelemetryService { get; }

0 commit comments

Comments
 (0)