iZettle / Flow

Flow is a Swift library for working with asynchronous flows and life cycles
MIT License
231 stars 14 forks source link

RFC: Add unsafeCompleteEarly #80

Closed niil-ohlin closed 5 years ago

niil-ohlin commented 5 years ago

I think that a function like this one could be useful when testing. For example for forcing Presentables to complete with a desired result without having interact with that presentable, and in turn make sure that the onValues to that Future works as expected.

What does everybody else think? I tried making some quick tests with this implementation and it seems to work without any major problems. @mansbernhardt do you foresee any major problem with this implementation?

niil-ohlin commented 5 years ago

Maybe we should add a testing target called FlowTest that depends on XCTest and only include functions like this in that, making it impossible to complete early in production. What do you think?

mansbernhardt commented 5 years ago

Perhaps you can use #canImport(XCTest)

mansbernhardt commented 5 years ago

Not sure how this would be used? Could you show an example?

niil-ohlin commented 5 years ago

I feel it's a bit complicated to give a good example of when it's used without having to write too much code. But essentially what I wanted to test in the beginning was something like this:

extension MasterView {
    var somePresentation: Presentation<DetailView>
}

extension DetailView: Presentable {
    public func materialize() -> (UIViewController, Future<String>) {...
}

I wanted to make the future in the presentation to finish without having to actually interact with the DetailView to make sure that the onValues on MasterView was set up correctly.

Of course a lot of these problems could be solved with dependency injection. But I think it would be nice to be able to write the tests for code that is not properly dependency injected before refactoring.

mansbernhardt commented 5 years ago

My intuition says there must be better way to solve similar situations than what was proposed in this PR. E.g. for your example above, perhaps this will do what you like?

somePresententation.map { _ in Future(value: ...) }
niil-ohlin commented 5 years ago

Yes but will that ever run? If you look in the tests that are provided with this PR. For example in FutureCompleteEarlyTests.swift:19. if the delay(by:) would be much larger. For example TimeInterval.greatestFiniteMagnitude a map { _ in Future(value: ...) } would have to wait for that delay to timeout.

The idea of this PR is to force a value you want to get immediately.

I agree that there must be better solutions than this. As this is very hacky.

niil-ohlin commented 5 years ago

I did some more tests. This particular implementation will never work I think. Since onValue creates a new Future It's impossible to know which future should be completed early. There will be no use if only the absolute last Future will be completed.

for example take a look at this:

Let's say I want to write tests for telemetry in a backend -> core data function.

    func getStringFromBackend() -> Future<String> {
        let future = Future("got from backend").delay(by: 0.1)

        future.onValue {
            print("logging network telemetry: \($0)")
        }

        return future.flatMap { value in
            print("got value: \(value)")
            return Future("saved to database").delay(by: 0.1)
        }.onValue {
            print("logging database telemetry: \($0)")
        }
    }

    func testExample() {

        let future = getStringFromBackend()

        future.unsafeCompleteEarly(.success("unsafeComplete"))

        // Hack code to wait for all the delays.
        let exp = expectation(description: "test")
        Scheduler.main.async(after: 1) {
            exp.fulfill()
        }
        wait(for: [exp], timeout: 2)
    }

which only outputs

logging network telemetry: got from backend

when in reality what I wanted to test was that all the onValues run.