[MBL-722] Full MockOptimizely Removal #1823
Conversation
scottkicks
force-pushed
the
test-optimizely-removal
branch
from
4988eeb
to
a5e5070
Compare
Generated by π« Danger |
Codecov Report
@@ Coverage Diff @@
## main #1823 +/- ##
==========================================
- Coverage 84.45% 84.35% -0.11%
==========================================
Files 1283 1276 -7
Lines 116650 115449 -1201
Branches 30953 30743 -210
==========================================
- Hits 98521 97390 -1131
+ Misses 17052 16992 -60
+ Partials 1077 1067 -10
... and 2 files with indirect coverage changes π£ Weβre building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Ok, so there's a lot here that is correct. But I really want you to focus on the changes that are needed here and understand why they need be reverted.
Please put together a PR that includes all these changes. I would like it if I only have to review it once so all these comments are considered and changes made. Ideally we should merge that PR after a single pass.
Please ask me any questions you have or if anything is unclear.
| } | ||
| } | ||
|
|
||
| func testFeatureFlagClientConfiguration_IsSuccess() { |
There was a problem hiding this comment.
@scottkicks Can we write a couple similar tests for our new remoteConfigClientConfiguredNotification and remoteConfigClientConfigurationFailedNotification?
| let mockOptimizelyClient = MockOptimizelyClient() | ||
| |> \.features .~ [OptimizelyFeature.commentFlaggingEnabled.rawValue: false] | ||
|
|
||
| func testView_CurrentUser_LoggedIn_IsBacking() { |
There was a problem hiding this comment.
@scottkicks So this exists already - testView_CurrentUser_LoggedIn_IsBacking_True why write it again?
Kickstarter-iOS/Features/ProjectPage/Controller/ProjectPageViewControllerTests.swift
Show resolved
Hide resolved
| @@ -198,8 +198,8 @@ final class LoginToutViewModelTests: TestCase { | |||
| } | |||
|
|
|||
| func testFacebookLoginFlow_Success_WhenFBLoginDeprecationFlagDisabled() { | |||
| let mockOptimizelyClient = MockOptimizelyClient() | |||
| |> \.features .~ [OptimizelyFeature.facebookLoginDeprecationEnabled.rawValue: true] | |||
| let mockRemoteConfigClient = MockRemoteConfigClient() | |||
There was a problem hiding this comment.
@scottkicks Good replacement here.
| lazy var paymentSheetEnabled: Bool = { | ||
| featureSettingsPaymentSheetEnabled() | ||
| true | ||
| }() |
There was a problem hiding this comment.
@scottkicks Can just assign to true, no need to use a closure, and remove lazy
| lazy var paymentSheetEnabled: Bool = { | ||
| featurePaymentSheetEnabled() | ||
| true | ||
| }() |
There was a problem hiding this comment.
@scottkicks Again no need for lazy or the closure.
| self.goToProject.assertValues([project]) | ||
| } | ||
| } | ||
|
|
||
| func testGoToAddNewStripeCard_WithPaymentSheetEnabled_NoStoredCards_Success() { |
There was a problem hiding this comment.
@scottkicks no need to add WithPaymentSheetEnabled to test name, as you did above, simplify the name to reflect the absence of the feature flag.
| private func campaignTabEnabled() -> Bool { | ||
| featureProjectPageStoryTabEnabled() | ||
| true | ||
| } |
There was a problem hiding this comment.
@scottkicks This can be a private var. I know I made these changes for you initially, but think about the changes on your own too. Don't just copy what I do!
π² What
Cleans up and removes deprecated optimizely code, updates library tests, and updates/re-records snapshot tests (this is why there are so many file changes)
π€ Why
Remote Config is fully integrated now so the optimizely code is no longer being used and can be cleaned up.
π How
β Acceptance criteria
Please review this commit by commit. There were a lot of snapshots updated.