search

mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.14k stars 2.91k forks source link

Hitting selectTab method causes memory leak failure for TabManager tests. #21088

Open data-sync-user opened 1 month ago

data-sync-user commented 1 month ago

Background context:

When working on writing tests for addTabsForURLs, the test seems to fail when shouldSelectTab is set to true. By doing so, this hits the selectTab method, which causes the test to fail on the memory leak check in createSubject. As a workaround, I set shouldSelectTab to false, which still provides valuable tests as I was working on the private mode logic.

Repro steps:

To isolate select tab from the addTabsForURLs method, you can attempt to run the following tests:

func test_addSelectTab_forPrivateMode() {
        let subject = createSubject()
        addTabs(to: subject, count: 3)

        subject.selectTab(subject.tabs.first)

        XCTAssertEqual(subject.tabs.count, 3)
}

!image-20240718-170553.png|width=732,height=69,alt="image-20240718-170553.png"!

Next Steps: To investigate the selectTab and why is the tab manager not being deallocated.

https://github.com/mozilla-mobile/firefox-ios/pull/20975

┆Issue is synchronized with this Jira Task

data-sync-user commented 1 month ago

➤ ih-codes commented:

This ticket and the related PR are currently on hold until we have a discussion at mozweek regarding how we want to go forward with testing code using the new swift concurrency library.

data-sync-user commented 1 month ago

➤ ih-codes commented:

Update 2: A new PR has been made which only addresses this ticket and no other refactors related to the TabManagerTests.

https://github.com/mozilla-mobile/firefox-ios/pull/21439 ( https://github.com/mozilla-mobile/firefox-ios/pull/21439|smart-link )

Cyndi Chin While it seems to help, it does not fix the original addTabsForURLs issues you spotted.

Do we want to make a separate test task for those? I’m pretty sure the true culprit in that case has something to do with the storeChanges call inside addTabs, not the selectTab method itself.

data-sync-user commented 2 weeks ago

➤ ih-codes commented:

https://github.com/mozilla-mobile/firefox-ios/pull/21496 ( https://github.com/mozilla-mobile/firefox-ios/pull/21496|smart-link )

The above PR should actually eliminate some of the async issues with this test. My PR for this been closed.

Cyndi Chin I’m going to mark this ticket as open if you want to take another look. Fingers crossed that the test is ok now! 🤞