Clean up RemoteConfigFeature+Helpers and RemoteConfigFeatureFlagToolsViewModel #1924
Conversation
|
|
||
| for feature in RemoteConfigFeature.allCases { | ||
| mockRemoteConfigClient.features[feature.rawValue] = false | ||
| } |
There was a problem hiding this comment.
You'll still need to re-record the snapshot when you add a new feature, but won't need to modify this code.
| @@ -88,11 +88,11 @@ internal class TestCase: XCTestCase { | |||
| } | |||
|
|
|||
| internal func preferredSimulatorCheck() { | |||
| let deviceName = ProcessInfo().environment["SIMULATOR_DEVICE_NAME"] | |||
| let deviceName = ProcessInfo().environment["SIMULATOR_VERSION_INFO"] | |||
There was a problem hiding this comment.
@ifosli This was a funny catch. I always name my snapshot simulators something (this one is "New New Snapshot Simulator"). It looks like SIMULATOR_DEVICE_NAME would return "New New Snapshot Simulator" whereas SIMULATOR_VERSION_INFO is like "CoreSimulator 494.33 - Device: iPhone 8 Plus - Runtime: iOS 11.2 (15C107) - DeviceType: iPhone 8 Plus".
Less elegant than your original == but I reaaaaally like my named simulators!
There was a problem hiding this comment.
Makes sense! I didn't even know you could re-name simulators, so thanks for handling that case!
| @@ -3,111 +3,102 @@ import Prelude | |||
| import XCTest | |||
|
|
|||
| final class RemoteConfigFeatureHelpersTests: TestCase { | |||
| func testBlockUsers_RemoteConfig_FeatureFlag_False() { | |||
| func assertFeatureIsFalse(_ featureFlag: Bool, | |||
| whenRemoteConfigFeatureIsFalse feature: RemoteConfigFeature) { | |||
There was a problem hiding this comment.
Normally I'm fine with boilerplate in tests, but in this case, I think the boilerplate was impeding clarity.
|
|
||
| withEnvironment(remoteConfigClient: mockRemoteConfigClient) { | ||
| XCTAssertTrue(featureBlockUsersEnabled()) | ||
| XCTAssertFalse(featureFlag) |
There was a problem hiding this comment.
I don't think these tests are working - we should pass in the featureFlag as a function instead so that it can be evaluated after the remoteConfig has been updated. In particular, I think this test should XCTAssertTrue(featureFlag) instead, and the fact that it passes the way it's written is concerning.
Also, if we want less boilerplate, thoughts on combining these into one function? Ie a testFeatureMatchesRemoteConfig and have it test both the true and the false case? I'm okay with them staying separate, though!
There was a problem hiding this comment.
Hahaha, OK, excellent catch here - on both bugs 😆 I see what you mean. Let me take another stab at this.
There was a problem hiding this comment.
As for combining the tests into one method - I just tried it out but honestly, all the closures make it look kind of ugly 😅 So just leaving them in separate test methods for now.
| @@ -88,11 +88,11 @@ internal class TestCase: XCTestCase { | |||
| } | |||
|
|
|||
| internal func preferredSimulatorCheck() { | |||
| let deviceName = ProcessInfo().environment["SIMULATOR_DEVICE_NAME"] | |||
| let deviceName = ProcessInfo().environment["SIMULATOR_VERSION_INFO"] | |||
There was a problem hiding this comment.
Makes sense! I didn't even know you could re-name simulators, so thanks for handling that case!
|
|
||
| withEnvironment(remoteConfigClient: mockRemoteConfigClient) { | ||
| XCTAssertTrue(featureBlockUsersEnabled()) | ||
| XCTAssertTrue(checkFeatureFlag()) |
There was a problem hiding this comment.
😅 Great catch @ifosli. Soon as I swapped this to XCTAssertTrue the tests started failing!
| self.assertFeatureIsFalse(featureBlockUsersEnabled(), whenRemoteConfigFeatureIsFalse: .blockUsersEnabled) | ||
| self.assertFeatureIsFalse(whenRemoteConfigFeatureIsFalse: .blockUsersEnabled) { | ||
| featureBlockUsersEnabled() | ||
| } |
There was a problem hiding this comment.
Nit: I could be wrong about this, but I don't think you need to do { featureBlockUsersEnabled() } - just featureBlockUsersEnabled should do the trick (though I think you need to pass the function as a normal param instead of trailing closure syntax). I'm okay with this as-is, but I think it's a little clearer when we don't make new closures just to call a single function.
There was a problem hiding this comment.
Ah, you're correct. Will fix, that looks much nicer.
|
|
||
| withEnvironment(remoteConfigClient: mockRemoteConfigClient) { | ||
| XCTAssertTrue(featureBlockUsersEnabled()) | ||
| XCTAssertTrue(checkFeatureFlag()) |
📲 What
Remove boilerplate from our feature flag code.
🤔 Why
👀 See
Note the test runs for the three commits below - I did the code cleanup first, waited for the tests to pass, and then did the test cleanup. Just to ensure I didn't break both the code and tests simultaneously.