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.

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

Add FXIOS-10881 [Sent from Firefox] Add ability to override a user's preference for a nimbus feature in unit tests #23790

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions BrowserKit/Sources/Shared/Prefs.swift
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ public struct PrefsKeys {
// Representing whether or not the last user session was private
public static let LastSessionWasPrivate = "wasLastSessionPrivate"

// Only used to force nimbus features to true with tests
public static let NimbusFeatureTestsOverride = "NimbusFeatureTestsOverride"
// Only used in unit tests to override the user's setting for nimbus features
public static let NimbusUserEnabledFeatureTestsOverride = "NimbusUserEnabledFeatureTestsOverride"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed because previously using the NimbusUserEnabledFeatureTestsOverride always overrode the value to true. I wanted the value to override either true or false so we can test with both the user preference enabled or disabled.


// Only used to force faster transition of tabs to the inactive state (10 seconds)
public static let FasterInactiveTabsOverride = "FasterInactiveTabsOverride"
Expand Down
4 changes: 4 additions & 0 deletions firefox-ios/Client.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -1868,6 +1868,7 @@
EDD3348E2D109458004516D0 /* ShareTelemetryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EDD3348D2D109458004516D0 /* ShareTelemetryTests.swift */; };
EDD334902D109A5B004516D0 /* ShareTelemetryActivityItemProviderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EDD3348F2D109A5B004516D0 /* ShareTelemetryActivityItemProviderTests.swift */; };
EDD334932D109AE6004516D0 /* MockShareTelemetry.swift in Sources */ = {isa = PBXBuildFile; fileRef = EDD334922D109AE6004516D0 /* MockShareTelemetry.swift */; };
EDD334952D10BEEB004516D0 /* UserDefaults+valueExists.swift in Sources */ = {isa = PBXBuildFile; fileRef = EDD334942D10BEEB004516D0 /* UserDefaults+valueExists.swift */; };
EDDF340A2CDD159F008BB6A4 /* SearchEngineModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = EDDF34092CDD159F008BB6A4 /* SearchEngineModel.swift */; };
EDDF340B2CDD1B7C008BB6A4 /* SearchEngineModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = EDDF34092CDD159F008BB6A4 /* SearchEngineModel.swift */; };
EDF567A02C8B51DC00FDB09D /* SiteImageView in Frameworks */ = {isa = PBXBuildFile; productRef = EDF5679F2C8B51DC00FDB09D /* SiteImageView */; };
Expand Down Expand Up @@ -9413,6 +9414,7 @@
EDD3348D2D109458004516D0 /* ShareTelemetryTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShareTelemetryTests.swift; sourceTree = "<group>"; };
EDD3348F2D109A5B004516D0 /* ShareTelemetryActivityItemProviderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShareTelemetryActivityItemProviderTests.swift; sourceTree = "<group>"; };
EDD334922D109AE6004516D0 /* MockShareTelemetry.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockShareTelemetry.swift; sourceTree = "<group>"; };
EDD334942D10BEEB004516D0 /* UserDefaults+valueExists.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UserDefaults+valueExists.swift"; sourceTree = "<group>"; };
EDDF34092CDD159F008BB6A4 /* SearchEngineModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchEngineModel.swift; sourceTree = "<group>"; };
EDEE4CC7BD65892525632A4E /* ko */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ko; path = ko.lproj/Today.strings; sourceTree = "<group>"; };
EE294A97B40C4FDBD67AE536 /* hr */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = hr; path = hr.lproj/ErrorPages.strings; sourceTree = "<group>"; };
Expand Down Expand Up @@ -10291,6 +10293,7 @@
E1442FC5294782D7003680B0 /* UIAlertController+Extension.swift */,
8A395551299AF83300B2AFBB /* UIControl+Extension.swift */,
E18EA56E28AD3279003F97FC /* UIDevice+Extension.swift */,
EDD334942D10BEEB004516D0 /* UserDefaults+valueExists.swift */,
E12BD0AF28AC3A7E0029AAF0 /* UIEdgeInsets+Extension.swift */,
C80E1A0F2A0943640025B9E1 /* UIFont+Extension.swift */,
E12BD0AD28AC38480029AAF0 /* UIImage+Extension.swift */,
Expand Down Expand Up @@ -16274,6 +16277,7 @@
DF5D47522A9381C700D6AE74 /* FakespotSettingsCardView.swift in Sources */,
C88E7A592A0553440072E638 /* OnboardingCardInfoModel.swift in Sources */,
B2DFB7E32B619E2B0004CEA5 /* AddressCellView.swift in Sources */,
EDD334952D10BEEB004516D0 /* UserDefaults+valueExists.swift in Sources */,
AB6FEA202AEA5CA200E7B2F2 /* FakespotAdView.swift in Sources */,
C87A121A28C2451A0097ED51 /* WallpaperMigrationUtility.swift in Sources */,
AB9CBC032C53B64C00102610 /* TrackingProtectionMiddleware.swift in Sources */,
Expand Down
16 changes: 16 additions & 0 deletions firefox-ios/Client/Extensions/UserDefaults+valueExists.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import Foundation

extension UserDefaults {
/// Returns `true` if this value exists, and `false` if the value is `nil`.
///
/// @discussion:
/// Calling `.bool(forKey:)` for a value that does NOT exist always returns false. However, sometimes we want to check
/// that the value has actually been set first.
func valueExists(forKey key: String) -> Bool {
return object(forKey: key) != nil
}
}
15 changes: 9 additions & 6 deletions firefox-ios/Client/FeatureFlags/NimbusFlaggableFeature.swift
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,6 @@ struct NimbusFlaggableFeature: HasNimbusSearchBar {

// MARK: - Public methods
public func isNimbusEnabled(using nimbusLayer: NimbusFeatureFlagLayer) -> Bool {
// Provide a way to override nimbus feature enabled for tests
if AppConstants.isRunningUnitTest, UserDefaults.standard.bool(forKey: PrefsKeys.NimbusFeatureTestsOverride) {
return true
}

return nimbusLayer.checkNimbusConfigFor(featureID)
}

Expand All @@ -166,7 +161,15 @@ struct NimbusFlaggableFeature: HasNimbusSearchBar {
public func isUserEnabled(using nimbusLayer: NimbusFeatureFlagLayer) -> Bool {
guard let optionsKey = featureKey,
let option = profile.prefs.boolForKey(optionsKey)
else { return isNimbusEnabled(using: nimbusLayer) }
else {
// In unit tests only, we provide a way to return an override value to simulate a user's preference for a feature
if AppConstants.isRunningUnitTest,
UserDefaults.standard.valueExists(forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride) {
return UserDefaults.standard.bool(forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I basically just hijacked some existing code and moved it down into the isUserEnabled() method instead of isNimbusEnabled(), so it only overrides the user's preference.

I had to move this code because isNimbusEnabled() also gets called in getNimbusOrDebugSetting(), which was overriding both the nimbus flag and the user preference flag in our unit tests... We don't need to override the nimbus flag though because we have better ways of doing that (e.g. #23725 (comment)).


return isNimbusEnabled(using: nimbusLayer)
}

return option
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ final class LaunchScreenViewModelTests: XCTestCase {
LegacyFeatureFlagsManager.shared.initializeDeveloperFeatures(with: profile)
messageManager = MockGleanPlumbMessageManagerProtocol()

UserDefaults.standard.set(true, forKey: PrefsKeys.NimbusFeatureTestsOverride)
UserDefaults.standard.set(true, forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)
}

override func tearDown() {
Expand All @@ -31,7 +31,7 @@ final class LaunchScreenViewModelTests: XCTestCase {
messageManager = nil
delegate = nil

UserDefaults.standard.set(false, forKey: PrefsKeys.NimbusFeatureTestsOverride)
UserDefaults.standard.removeObject(forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)
}

func testLaunchDoesntCallLoadedIfNotStarted() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,6 @@ class FeatureFlagManagerTests: XCTestCase, FeatureFlaggable {
let mockProfile = MockProfile(databasePrefix: "FeatureFlagsManagerTests_")
mockProfile.prefs.clearAll()
LegacyFeatureFlagsManager.shared.initializeDeveloperFeatures(with: mockProfile)
UserDefaults.standard.set(false, forKey: PrefsKeys.NimbusFeatureTestsOverride)
}

override func tearDown() {
UserDefaults.standard.removeObject(forKey: PrefsKeys.NimbusFeatureTestsOverride)
}

// MARK: - Tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class UpdateViewModelTests: XCTestCase {
override func tearDown() {
super.tearDown()
profile = nil
UserDefaults.standard.removeObject(forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)
}

// MARK: Enable cards
Expand Down Expand Up @@ -104,7 +105,7 @@ class UpdateViewModelTests: XCTestCase {
func testShouldNotShowCoverSheetForSameVersion() {
let subject = createSubject()
let currentTestAppVersion = "22.0"
UserDefaults.standard.set(true, forKey: PrefsKeys.NimbusFeatureTestsOverride)
UserDefaults.standard.set(true, forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)

// Setting clean install to false
profile.prefs.setString(currentTestAppVersion, forKey: PrefsKeys.AppVersion.Latest)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import UniformTypeIdentifiers
import XCTest
import Shared

@testable import Client

final class ShareManagerTests: XCTestCase {
Expand All @@ -23,6 +25,7 @@ final class ShareManagerTests: XCTestCase {

override func tearDown() {
testTab = nil
UserDefaults.standard.removeObject(forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)
super.tearDown()
}

Expand Down Expand Up @@ -510,6 +513,122 @@ final class ShareManagerTests: XCTestCase {
XCTAssertTrue(itemForShareActivity is NSNull)
}

/// This test ensures that the `ShareManager` does not enforce Sent from Firefox treatment for users enrolled in the
/// experiment who have explicitly opted out (for example, using the "Include Firefox Download Link on WhatsApp Shares"
/// toggle on the general settings screen).
func testGetActivityItems_forTab_withSentFromFirefoxEnabled_respectsUserPreferencesToOptOut() throws {
// Setup Nimbus to emulate a user enrolled in Sent from Firefox with the Treatment A branch
setupNimbusSentFromFirefoxTesting(isEnabled: true, isTreatmentA: true)

// Simulate the user having disabled the Sent from Firefox toggle in the Settings
UserDefaults.standard.set(false, forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)

let whatsAppActivityIdentifier = "net.whatsapp.WhatsApp.ShareExtension"
let whatsAppActivity = UIActivity.ActivityType(rawValue: whatsAppActivityIdentifier)

let activityItems = ShareManager.getActivityItems(
forShareType: .tab(url: testWebURL, tab: testTab),
withExplicitShareMessage: nil
)

// Check we get all types of share items for tabs below:
let urlActivityItemProvider = try XCTUnwrap(activityItems[safe: 0] as? URLActivityItemProvider)
let urlDataIdentifier = urlActivityItemProvider.activityViewController(
createStubActivityViewController(),
dataTypeIdentifierForActivityType: whatsAppActivity
)
let itemForActivity = urlActivityItemProvider.activityViewController(
createStubActivityViewController(),
itemForActivityType: whatsAppActivity
)

// The rest of the content should be unchanged from other tests:
_ = try XCTUnwrap(activityItems[safe: 1] as? TabPrintPageRenderer)

_ = try XCTUnwrap(activityItems[safe: 2] as? TabWebView)

let titleActivityItemProvider = try XCTUnwrap(activityItems[safe: 3] as? TitleActivityItemProvider)
let itemForTitleActivity = titleActivityItemProvider.activityViewController(
createStubActivityViewController(),
itemForActivityType: whatsAppActivity
)

let telemetryActivityItemProvider = try XCTUnwrap(activityItems[safe: 4] as? ShareTelemetryActivityItemProvider)
let itemForShareActivity = telemetryActivityItemProvider.activityViewController(
createStubActivityViewController(),
itemForActivityType: whatsAppActivity
)

XCTAssertEqual(activityItems.count, 5)
XCTAssertEqual(urlDataIdentifier, UTType.url.identifier)
XCTAssertEqual(itemForActivity as? URL, testWebURL)
XCTAssertEqual(
itemForTitleActivity as? String,
testWebpageDisplayTitle,
"When no explicit share message is set, we expect to see the webpage's title."
)
XCTAssertTrue(itemForShareActivity is NSNull)
}

/// This test ensures that the `ShareManager` applies Sent from Firefox treatment for users enrolled in the experiment
/// who have explicitly opted in (for example, using the "Include Firefox Download Link on WhatsApp Shares" toggle on the
/// general settings screen).
func testGetActivityItems_forTab_withSentFromFirefoxEnabled_respectsUserPreferencesToOptIn() throws {
let expectedShareContentA = "https://mozilla.org Sent from Firefox 🦊 Try the mobile browser: https://mzl.la/4fOWPpd"

// Setup Nimbus to emulate a user enrolled in Sent from Firefox with the Treatment A branch
setupNimbusSentFromFirefoxTesting(isEnabled: true, isTreatmentA: true)

// Simulate the user having enabled the Sent from Firefox toggle in the Settings
UserDefaults.standard.set(true, forKey: PrefsKeys.NimbusUserEnabledFeatureTestsOverride)

let whatsAppActivityIdentifier = "net.whatsapp.WhatsApp.ShareExtension"
let whatsAppActivity = UIActivity.ActivityType(rawValue: whatsAppActivityIdentifier)

let activityItems = ShareManager.getActivityItems(
forShareType: .tab(url: testWebURL, tab: testTab),
withExplicitShareMessage: nil
)

// Check we get all types of share items for tabs below:
let urlActivityItemProvider = try XCTUnwrap(activityItems[safe: 0] as? URLActivityItemProvider)
let urlDataIdentifier = urlActivityItemProvider.activityViewController(
createStubActivityViewController(),
dataTypeIdentifierForActivityType: whatsAppActivity
)
let itemForActivity = urlActivityItemProvider.activityViewController(
createStubActivityViewController(),
itemForActivityType: whatsAppActivity
)

// The rest of the content should be unchanged from other tests:
_ = try XCTUnwrap(activityItems[safe: 1] as? TabPrintPageRenderer)

_ = try XCTUnwrap(activityItems[safe: 2] as? TabWebView)

let titleActivityItemProvider = try XCTUnwrap(activityItems[safe: 3] as? TitleActivityItemProvider)
let itemForTitleActivity = titleActivityItemProvider.activityViewController(
createStubActivityViewController(),
itemForActivityType: whatsAppActivity
)

let telemetryActivityItemProvider = try XCTUnwrap(activityItems[safe: 4] as? ShareTelemetryActivityItemProvider)
let itemForShareActivity = telemetryActivityItemProvider.activityViewController(
createStubActivityViewController(),
itemForActivityType: whatsAppActivity
)

XCTAssertEqual(activityItems.count, 5)
XCTAssertEqual(urlDataIdentifier, UTType.plainText.identifier)
XCTAssertEqual(itemForActivity as? String, expectedShareContentA)
XCTAssertEqual(
itemForTitleActivity as? String,
testWebpageDisplayTitle,
"When no explicit share message is set, we expect to see the webpage's title."
)
XCTAssertTrue(itemForShareActivity is NSNull)
}

// MARK: - Helpers

private func createStubActivityViewController() -> UIActivityViewController {
Expand Down
Loading