Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve unit tests and integration tests to prevent false positives. #46

Open
2 tasks
levibostian opened this issue Feb 7, 2019 · 1 comment
Open
2 tasks
Labels
enhancement Would make an improvement in some way but is not a requirement such as a bug would be.

Comments

@levibostian
Copy link
Owner

levibostian commented Feb 7, 2019

As discussed in this issue comment, there has been a small number of false positives that have been discovered in the Teller tests.

What is the problem: There is the potential of bugs existing in Teller (such as this issue) that are not being caught by the tests.

The issue being used as an example needs to have integration tests to cover it. A unit test will not test this properly.

How do we solve this?: The current tests for OnlineRepository are not integration tests. They are more of unit tests (as they are highly mocked). The functions themselves are testing the correct use cases. However, because we are mocking all of it's dependencies, the OnlineRepository is not being tested property from an integration standpoint.

I propose the following changes:

  • Improve Repository tests to make it more of an integration test Take the existing Repository and PagingRepository tests, remove mocked dependencies (except the ones that fetch from a network) and use real dependencies to have an integration test environment setup for it.

  • Create a new test file for OnlineRepository that can be for unit testing the functions of OnlineRepository (if we find it necessary to test).

@levibostian levibostian added enhancement Would make an improvement in some way but is not a requirement such as a bug would be. good first issue Good for newcomers labels Feb 7, 2019
@levibostian levibostian removed the good first issue Good for newcomers label Jun 4, 2020
@levibostian
Copy link
Owner Author

To help give some more context to the proposed change of Improve Repository tests to make it more of an integration test, here is some example test cases:

    func test_givenFetchWithMorePagesAvailable_expectNextRefreshWhenGoToNextPage() {
        let refresh: ReplaySubject<FetchResponse<PagedFetchResponse<String, Void>, Error>> = ReplaySubject.createUnbounded()
        refresh.onNext(FetchResponse.success(PagedFetchResponse(areMorePages: true, nextPageRequirements: Void(), fetchResponse: "")))
        refresh.onCompleted()
        
        let observe = ReplaySubject<String>.createUnbounded()
        observe.onNext("first")

        initSyncStateManager(syncStateManagerFakeData: getSyncStateManagerFakeData(isDataTooOld: false, hasEverFetchedData: true, lastTimeFetchedData: Date()))
        initDataSource(fakeData: getDataSourceFakeData(isDataEmpty: false, observeCachedData: observe.asObservable(), fetchFreshData: refresh.asSingle()))
        initRepository()
        
        let expectToGoToNextPage = expectation(description: "Expect to go to next page")
        var hasFirstFetchHappened = false

        compositeDisposable += repository.observe()
            .subscribe(onNext: { dataState in
                guard dataState.cache != nil else {
                    return
                }
                
                if dataState.cache!.cache == "first" && dataState.cache!.areMorePages && !dataState.isRefreshing { // first fetch done.
                    // Setup next fetch.
                    let secondRefresh: ReplaySubject<FetchResponse<PagedFetchResponse<String, Void>, Error>> = ReplaySubject.createUnbounded()
                    secondRefresh.onNext(FetchResponse.success(PagedFetchResponse(areMorePages: false, nextPageRequirements: Void(), fetchResponse: "")))
                    secondRefresh.onCompleted()
                    self.dataSource.fakeData.fetchFreshData = secondRefresh.asSingle()
                    
                    
                    hasFirstFetchHappened = true
                    self.repository.goToNextPage()
                    
                    return
                }
                
                if hasFirstFetchHappened && !dataState.isRefreshing { // second refresh is done.
                    XCTAssertFalse(dataState.cache!.areMorePages)
                    
                    expectToGoToNextPage.fulfill()
                    
                    return
                }
            })
        
        // do not trigger refresh until last. setup test code first to avoid flaky
        repository.pagingRequirements = MockPagingRepositoryDataSource.PagingRequirements(pageNumber: 1)
        repository.requirements = MockPagingRepositoryDataSource.Requirements(randomString: nil)
        
        waitForExpectations(timeout: TestConstants.AWAIT_DURATION, handler: nil)
    }
    
    func test_givenFetchWithNoMorePagesAvailable_expectIgnoreRequestWhenGoToNextPage() {
        let refresh: ReplaySubject<FetchResponse<PagedFetchResponse<String, Void>, Error>> = ReplaySubject.createUnbounded()
        refresh.onNext(FetchResponse.success(PagedFetchResponse(areMorePages: false, nextPageRequirements: Void(), fetchResponse: "")))
        refresh.onCompleted()
        
        let observe = ReplaySubject<String>.createUnbounded()
        observe.onNext("first")

        initSyncStateManager(syncStateManagerFakeData: getSyncStateManagerFakeData(isDataTooOld: false, hasEverFetchedData: true, lastTimeFetchedData: Date()))
        initDataSource(fakeData: getDataSourceFakeData(isDataEmpty: false, observeCachedData: observe.asObservable(), fetchFreshData: refresh.asSingle()))
        initRepository()
        
        let expectToGoToNextPage = expectation(description: "Expect to go to next page")
        expectToGoToNextPage.assertForOverFulfill = false // it needs to be called at least once. we don't mind more then that for this specific test.
        let expectToNotGoToNextPage = expectation(description: "Expect to *not* go to next page")
        expectToNotGoToNextPage.isInverted = true
        
        compositeDisposable += repository.observe()
        .subscribe(onNext: { dataState in
            guard dataState.cache != nil else {
                return
            }
                                            
            if self.dataSource.fetchFreshDataCount == 1 && !dataState.isRefreshing { // first refresh is done.
                XCTAssertFalse(dataState.cache!.areMorePages)
                
                // Attempt to go to next page even though we hope it does not happen.
                let secondRefresh: ReplaySubject<FetchResponse<PagedFetchResponse<String, Void>, Error>> = ReplaySubject.createUnbounded()
                secondRefresh.onNext(FetchResponse.success(PagedFetchResponse(areMorePages: false, nextPageRequirements: Void(), fetchResponse: "")))
                secondRefresh.onCompleted()
                self.dataSource.fakeData.fetchFreshData = secondRefresh.asSingle()
                    
                self.repository.goToNextPage()
                
                expectToGoToNextPage.fulfill()
            }
            
            if self.dataSource.fetchFreshDataCount == 2 && !dataState.isRefreshing { // second refresh is done
                expectToNotGoToNextPage.fulfill()
            }
        })
        
        // do not trigger refresh until last. setup test code first to avoid flaky
        repository.pagingRequirements = MockPagingRepositoryDataSource.PagingRequirements(pageNumber: 1)
        repository.requirements = MockPagingRepositoryDataSource.Requirements(randomString: nil)
        
        waitForExpectations(timeout: TestConstants.AWAIT_DURATION, handler: nil)
    }

This code is (1) long and hard to read. (2) requires a lot of setup code in each of the test functions. If it was an integration test, we could do this idea of setting up the state but then after that the real dependencies will be able to take over.

The biggest problem with the current mocked tests is when the repository saves a new cache from a fetch. Right now, the cache is being observed through mocking. That is not realistic. Also when a fetch is complete for the first time, the mock for determining how old the cache is can still say that a cache has never been fetched before.

This code above can remove many of the conditional statements and fulfill items in the observe() functions. They are this complex because it's hard to determine when async events are happening. But if we have more of an integration test, we can write conditionals against the cache state variable to feel really confident with the test, easy to write and maintain, and not as flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Would make an improvement in some way but is not a requirement such as a bug would be.
Projects
None yet
Development

No branches or pull requests

1 participant