splitio / ios-client

iOS SDK client for Split Software
https://split.io
Other
11 stars 18 forks source link

SplitResult initializer unavailable #448

Closed Jeff-Meadows closed 1 year ago

Jeff-Meadows commented 1 year ago

I want to write some unit tests and create a mock SplitClient. But SplitClient has two functions

func getTreatmentWithConfig(_ split: String) -> SplitResult
func getTreatmentWithConfig(_ split: String, attributes: [String: Any]?) -> SplitResult

that return SplitResults. And while SplitResult itself is public, its initializer isn't, so I can't create them, and therefore, can't create a class that conforms to SplitClient.

@objc public class SplitResult: NSObject {
    @objc public var treatment: String
    @objc public var config: String?

    init(treatment: String, config: String? = nil) {
        self.treatment = treatment
        self.config = config
    }
}

Please consider making the initializer public, or providing another way to create a SplitResult that can be used in unit tests.

hbqdev commented 1 year ago

Hi @Jeff-Meadows

Thank you, we have let our team know and they will consider this in the future roadmap.

Regards,

hbqdev commented 1 year ago

hi @Jeff-Meadows We just wanna touch base again on this issue.

For your use case, our SDK can be used with a mocked config through the localhost host https://help.split.io/hc/en-us/articles/360020401491-iOS-SDK#localhost-mode

With that you do not need to mock our client, and you can use allowlists to make the corresponding config be returned on each treatment (already knowing which treatment each key gets).

In this case the mock client is the client that get initialized from a localhost factory.

Can you please check and see if that works for you?

Regards,

Jeff-Meadows commented 1 year ago

Hi @hbqdev - I looked at using localhost mode, but I didn't end up using it for the following reasons:

  1. Many of the tests I want to write deal with multiple traffic types, and localhost mode doesn't seem to support more than one traffic type (at least the documentation doesn't mention traffic type at all).
  2. As far as I can tell, localhost mode requires adding a file to the production app bundle, which is not something I want to do for tests.
  3. As far as I can tell, localhost mode requires a host app, but the unit tests I want to write don't run in a test app.
  4. Many of the tests I want to write deal with missing splits, which would mean that I'd need to continually rewrite the localhost mode yaml file in between tests. Since this file has to be part of the app bundle, this seems more difficult to orchestrate than I'd like, and potentially slow.

Since the first thing I listed blocked me from using localhost mode in the first place, the rest of them are based on speculation and the docs, rather than having actually tried. So it's possible I'm wrong about some of these points. But I think I'd need to solve all four points in order to use localhost mode.

Jeff-Meadows commented 1 year ago

Hi @hbqdev any update?

hbqdev commented 1 year ago

Hi @Jeff-Meadows Our apology for the delay.

For 1. The feature flag evaluations are not affected by the traffic type at all, that's why localhost doesn't take those into account. When a feature flag is set in the UI for type user for example, we'll use that TT in the back end to understand what is the type of the keys we see in impressions for this flag. But the evaluation doesn't ever check it.

For 2 it'll need to be on the bundle of the test build, not production.

For 3 the whole idea is that it's for unit tests for modules that use Split, so that would require the Split factory and client or factory and manager.

Our dev teams have a few ways to test as shown below: There are many options that can be used in order to test the SDK The first option would be to create a wrapper for the SDK and then use it to do evaluations in the app. Then for testing, a Stub can be created. Here is an example:

protocol MySplitWrapper { func evaluate(featureFlag: String) -> MyResult }

struct MyResult { let treatment: String let config: String? }

// This would be the wrapper to use in production class MySplitWrapperImpl { let factory: SplitFactory init() { // Init factory here factory = splitInitHere() }

// Then create a function to wrap get treatment func evaluate(featureFlag: String) -> MyResult { let result = factory.client.getTreatment("featureFlag") return MyResult(treatment: result.treatment, config: result.config) } }

// In the test target class MySplitWrapperStub { // Then create a function to wrap get treatment func evaluate(featureFlag: String) -> MyResult { return MyResult(treatment: "on", config: nil) } }

This way, MySplitWrapperStub is injected in the component so that is easy to test

A second option involves importing the SDK as "testable". By doing this, internal components become available to use. Therefore, the SplitResult constructor can be used to create an instance. An example of this usage bellow: @testable import Split

class MySplitStub: SplitClient { .... func getTreatmentWithConfig(_ split: String) -> SplitResult { return SplitResult(treatment: "hi", config: nil) } }

final class MyTests: XCTestCase {

func testExample() throws { let mySplitStub = MySplitStub() le result = mySplitStub.getTreatmentWithConfig("my_ff")

   print("Result: \(result.treatment) -- \(result.config)")

} }

The last option relies on using localhost in conjunction with the previous technique. Importing the SDK as "testable" makes the 'setBundle' function of the SplitFactoryBuilder available. Then it is possible to make the SDK take files from the test bundle with the following code:

@testable import Split

final class MyTests: XCTestCase { func testExample() throws { let apiKey = "localhost" let matchingKey = "CUSTOMER_ID" let splitConfig: SplitClientConfig = SplitClientConfig() // This config allows using different file for each test splitConfig.splitFile = "localhost.yaml"

       let key: Key = Key(matchingKey: "key")
       let builder = DefaultSplitFactoryBuilder()
       let bundle = Bundle(for: type(of: self))
       // This function is internal, but available using @testable import
       builder.setBundle(bundle)
       // Now files in test bundle will be used
       let factory = builder.setApiKey(apiKey).setKey(key).setConfig(splitConfig).build()
       let client = factory?.client

       ...
   }

}

Can you please review that and let us know? Regarding #2 is necessary in order for the test above to work. That would be importing the SDK as @testable and setting the bundle as in the code example.

Regards

Jeff-Meadows commented 1 year ago

I've tried importing the Split SDK as @testable, and when it works, that solves my problem. However, we don't always want to compile the Split SDK with testing enabled, which is required for that to work. So this doesn't fully solve the problem.

Yes, it's possible to wrap usage of the Split SDK to make code that uses it more testable. We've already done that, and app developers only interact with our wrapper, so code that uses feature flags is plenty testable.

What I'm trying to do is write good unit tests for our wrapper. Our wrapper uses the manager and inspects the traffic type of feature flags, making decisions based on that. So localhost mode is of no use to me for testing this, because its feature flags don't know about traffic type.

What's a little frustrating is that the SDK is almost mockable. It mostly exposes public protocols, which is a good sign for mockability.

eg

@objc public protocol SplitFactory {
    var client: SplitClient { get }
    var manager: SplitManager { get }
    var version: String { get }
}

Great! I can create my own class that conforms to SplitFactory. All I need to do is create a class that has these 3 properties. OK, well then I need some classes that conform to SplitClient and SplitManager.

@objc public protocol SplitManager {
    var splits: [SplitView] { get }
    var splitNames: [String] { get }
    func split(featureName: String) -> SplitView?
}

Conforming to SplitManager is straightforward, and although it has a property and function that return a concrete class SplitView, that class is public and I can create one. (Sure, it would be a "cleaner" interface if SplitView were also a protocol) The problem comes when trying to conform to SplitClient. I won't paste the entire definition here because it's got 24 members, but 2 of them return a SplitResult, a concrete class that's unfortunately not possible to create.

@objc public class SplitResult: NSObject {
    @objc public var treatment: String
    @objc public var config: String?

    init(treatment: String, config: String? = nil) {
        self.treatment = treatment
        self.config = config
    }
}

The internal-only initializer on this class prevents us from creating an instance of SplitResult. Because we can't create our own SplitResults, we can't create a mocked SplitClient, and so in turn can't create a mocked SplitFactory. Everything else about this part of the SDK's design is good for mocking, except for this. This seems unintentional, and tarnishes an otherwise good design.

Am I missing something? Is there a good reason for this class to be public yet have no public initializers?

Jeff-Meadows commented 1 year ago

It would also be nice if localhost mode (in all SDKs) just supported traffic type as well.

hbqdev commented 1 year ago

hi @Jeff-Meadows Our apology for the long delay on this

In version 2.21.0 to make the SDK more testable, we've made the SplitResult init method public. Although is not the best solution, as you stated in a previous post, is the one that will allow you to remove the @testable import without being a breaking change. In the future, we plan to replace SplitResult and SplitView classes with protocols to maintain consistency.

Thank you for your suggestions