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

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

Merged
merged 6 commits into from
Dec 18, 2024
Merged

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

merged 6 commits into from
Dec 18, 2024

Conversation

ih-codes
Copy link
Collaborator

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR does two main things:

  1. Improves our preexisting Nimbus unit test override flag to A) allow us to set true or false as the override value, and B) to only override a user's preference for a specific nimbus feature (not the debug setting).
  2. Adds 2 unit tests to check that the Share Manager respects the user's preference for the Sent from Firefox experiment treatment

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@ih-codes ih-codes requested a review from a team as a code owner December 16, 2024 21:04
ih-codes
ih-codes commented Dec 16, 2024
View reviewed changes
Copy link
Collaborator Author

@ih-codes ih-codes left a comment

Choose a reason for hiding this comment

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

Just some comments to explain what I am doing here...

firefox-ios/Client/FeatureFlags/NimbusFlaggableFeature.swift
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)).

BrowserKit/Sources/Shared/Prefs.swift
// 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.

Base automatically changed from ih/FXIOS-10866-SFF-add-shared-to-telemetry to main December 18, 2024 18:26
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 18, 2024

This pull request has conflicts when rebasing. Could you fix it @ih-codes? 🙏

@ih-codes
no message
5c07f90
@ih-codes
Cleanup.
ab9b0bc
@ih-codes
Slightly alter our implementation of the preexisting Nimbus test over…
6438113 
…ride flag to ensure it only overrides the user's preference setting in tests. Make behaviour more explicit and add documentation. Update handling for tests using the old code.
@ih-codes
Add an additional ShareManager test which checks if the ShareManager …
9930655 
…respects the user's Sent from Firefox preference when checking whether WhatsApp share treatment should be appended.
@ih-codes
Minor documentation cleanup.
b2a34f7
@ih-codes
Also add an explicit user opt in test for Sent from Firefox.
e455fae
@ih-codes ih-codes force-pushed the ih/FXIOS-10881-SFF-share-manager-nimbus-user-pref-unit-test branch from 439d51e to e455fae Compare December 18, 2024 18:36
@mobiletest-ci-bot
Copy link

Messages
📖 Project coverage: 33.24%
📖 Edited 7 files
📖 Created 1 files

Client.app: Coverage: 32.07

File Coverage
UserDefaults+valueExists.swift 100.0% ✅
NimbusFlaggableFeature.swift 99.29% ✅

Generated by 🚫 Danger Swift against e455fae

@ih-codes ih-codes merged commit 87f330c into main Dec 18, 2024
13 of 14 checks passed
@ih-codes ih-codes deleted the ih/FXIOS-10881-SFF-share-manager-nimbus-user-pref-unit-test branch December 18, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adudenamedruby adudenamedruby adudenamedruby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ih-codes @mobiletest-ci-bot @adudenamedruby