Introduced TestLogHandler to simplify how we test logged messages

[NachoSoto]

We had this fragile pattern in several tests:

var loggedMessages = [String]()
let originalLogHandler = Logger.logHandler
defer { Logger.logHandler = originalLogHandler }

Logger.logHandler = { _, message, _, _, _ in
    loggedMessages.append(message)
}

Which was too verbose and error prone.

This new type works like an RAII type. Its lifetime scopes the messages that are observed, so simply having a reference around for the lifetime of the test allows observing only those messages.

This much simpler abstraction will make it easier for us to test more of what's logged by the SDK.

Examples:

• Initialize in setUp:

private var testLogHandler: TestLogHandler!
override func setUp() {
    super.setUp()
    self.testLogHandler = TestLogHandler()
}
override func tearDown() {
    self.testLogHandler = nil
    super.tearDown()
}

func testExample() {
    // Run code

    expect(self.testLogHandler.messages).to(contain(...))
}

• Alternatively, TestLogHandler can be used locally within a single test:

func testExample() {
    let logHandler = TestLogHandler()

    // Run some code

    expect(logHandler.loggedMessages.onlyElement?.message) == "Expected log"
}

Notes:

This implicitly overrides Purchases.shared.verboseLogHandler while running tests, but it just wraps it by still invoking the default handler, so logs are still printed in the console.

[NachoSoto]

@RevenueCat/core-sdk bump

[tonidero]

Just a couple of questions, but it's looking great!

[tonidero]

Hmm personally, I don't like that an object contruction such as this has a side effect like this... Could we add a method on the TestLogHandler so it gets called only on the tests that require it instead? It might be more verbosity on the call side, but I think it makes it clearer.

[NachoSoto]

This is done lazily only when an instance is created already (Swift properties like this are lazy)

[tonidero]

Hmm so if I understand this correctly, when the TestLogHandler gets released, we remove the observers, but we don't reset the value of verboseLogHandler. I think the way it is right now, the behavior should be correct, but internally, we will be sharing this logHandler between tests (unless it gets reconstructed on every test). I wonder if we could reset Purchases.verboseLogHandler on the deinit as well? Not sure if there would be any issues with that.

[NachoSoto]

We would have to ensure that there aren't any observers left, which would be extra state. It's possible, but I think it's unnecessary for now, and since this only affects tests I think it's a fair compromise. This class doesn't even get compiled outside the tests.

[aboedo]

I'm late to the game but just wanted to say that I love this!!

[NachoSoto]

😍