nalexn / ViewInspector

Runtime introspection and unit testing of SwiftUI views
MIT License
2.18k stars 150 forks source link

Does testing NavigationStack's destination view probably create memory leaks during testing? #337

Open greenerchen opened 3 weeks ago

greenerchen commented 3 weeks ago

Hi,

I'm learning SwiftUI, Swift concurrency, and trying TDD using ViewInspector. I use the code snippet[1] to detect possible memory leaks in my unit tests, and I found that the @ObservedObject property couldn't be released after test teardown. @ObservedObject property implies it's a MainActor instance, so my tests all annotate @MainActor to suppress the warnings of actor-isolated messages. I'm curious if I did the test wrong and how I can fix it. Thanks!

code snippets

  1. memory leaks detection
    func trackForMemoryLeaks(_ instance: AnyObject, file: StaticString = #filePath, line: UInt = #line) {
        addTeardownBlock { [weak instance] in
            XCTAssertNil(instance, "Instance should have been deallocated. Potential memory leak.", file: file, line: line)
        }
    }
  2. system under testing (SUT)
    @MainActor private func makeSUT(session: SHManagedSessionProtocol = SHManagedSessionMock()) -> MatchView {
        let matcher = ShazamMatcher(session: session)
        let sut = MatchView(matcher: matcher)
        trackForMemoryLeaks(matcher)
        return sut
    }
  3. View using @ObservedObject

    struct MatchView: View {
    @ObservedObject var matcher: ShazamMatcher
    @State private(set) var showResult: Bool = false
    var resultViewDidAppear: ((Self) -> Void)?
    
    var body: some View {
        NavigationStack {
            VStack {
                switch matcher.state {
                case .matched:
                    ShazamStartView()
                        .accessibilityIdentifier("match_idle_state_view")
                        .onTapGesture {
                            Task {
                                try await matcher.match()
                            }
                        }
                        .onAppear {
                            showResult = matcher.state == .matched
                            resultViewDidAppear?(self)
                        }
                ...
                }
    
            }
            .navigationDestination(isPresented: $showResult, destination: {
                if let result = matcher.currentMatchResult {
                    ShazamResultView(vm: ShazamResultViewModel(result: result))
                        .onAppear(perform: {
                            matcher.reset()
                        })
                        .accessibilityIdentifier("match_matched_state_view")
                }
            })
        }
    }
    }
  4. The test

    @MainActor func test_state_matched() async throws {
        var sut = makeSUT()
    
        sut.matcher.state = .matched
        sut.matcher.currentMatchResult = ShazamMatchResult(match: matchStub)
    
        XCTAssertNoThrow(try sut.inspect().find(viewWithAccessibilityIdentifier: "match_idle_state_view"), "Expected to find the idle state view")
    
        let exp = sut.on(\.resultViewDidAppear) { view in
            XCTAssertTrue(try view.actualView().showResult)
            XCTAssertNoThrow(try view.actualView().inspect().find(viewWithAccessibilityIdentifier: "match_matched_state_view"))
            XCTAssertNoThrow(try view.actualView().inspect().find(text: "Way Maker"))
            XCTAssertNoThrow(try view.actualView().inspect().find(text: "Leeland"))
        }
        ViewHosting.host(view: sut)
        await fulfillment(of: [exp], timeout: 1)
    }
nalexn commented 3 weeks ago

Try to add ViewHosting.expel() at the end of the sut.on(\.resultViewDidAppear) inspection closure. Also, you might find it more convenient to use async inspection, as it handles the view expelling implicitly

greenerchen commented 3 weeks ago

Adding ViewHosting.expel() at the end of the sut.on(\.resultViewDidAppear) inspection closure works! async inspection looks nice. I'll give it a shot. Thank you!! :D

greenerchen commented 3 weeks ago

Just put a note here. After I use async inspection instead, it requires more resource to compile on my CI service (Bitrise M1 medium 4 CPU @ 3.2GHz, 6 GB RAM). With Bitrise Support's help, we've tested it needs Bitrise M1 Large 8 CPU @ 3.2GHz, 12 GB RAM) to compile successfully. Seems I can try another CI service.

greenerchen commented 1 week ago

I glanced the source code of ViewHosting.expel(), and found the design is to test and expel a single view. For my case, it was testing a NavigationStack's destination view. Seems this is the root cause why I encountered memory leaks.

nalexn commented 1 week ago

@greenerchen since you've reopened the ticket - what do you think should be done here? I thought after you added view expelling the memory issue was gone

greenerchen commented 1 week ago

@nalexn yes, the memory issue was gone on my laptop after I added view expelling. I wanna help update document about async inspection with NavigationDestination, so I reopened the ticket for the record.