Assigning to gchatz to see if they can help evaluate if these tests are needed. Reach out to me if you have any questions!

Overall, even though there are unit tests, the TabUsageRecorder eg tests would catch integration bugs. Most of the tests verify that a metric is collected after certain user actions. Additionally, a few verify that the tab reloads after an eviction.



One issue with the tests is that they exercise code paths that cannot be reached by the user. Specifically, OpenNewIncognitoTabUsingUIAndEvictMainTabs() is used throughout to evict tabs and open tabs in incognito. This method was created originally because switching modes used to cause tab eviction. This is no longer the case, and the only ways that tabs can be evicted is by termination of the renderer process (and the purging of Chrome in the background, implicitly). To make the tests more accurate, they instead could use something like SimulateWKWebViewCrash(WKWebView* webView) in wk_web_view_crash_utils.h to cause eviction.



There are a few tests that can be removed: testing that opening new tab does not cause eviction, and opening tab from app does not cause eviction. 

Thanks Gregory!

So that would we using a different private API, since SimulateWKWebViewCrash is in ios/web/test.

Eugene, switching the the method that Gregory suggested would match more the intent of the test, but it may be leaking too much private stuff. Here are a few questions guiding the next steps... Would we not want to expose SimulatWKWebViewCrash in ios/web/public/test? Without exposing something, we can't do any tests with evicted tabs, are we okay with that? Or make a method that evicts tabs, which can do it by whatever means works?

This is blocking some ARC conversions, so I think we should do something quickly (DEPS exclusion, move method to ios/web/public/test, delete the tests), but I also want to make sure we don't do anything terrible.

Eugene, no need to respond now. I need to investigate if the cause of this bug is necessary before we make a decision. I'll ping the bug then.

From various discussions, we decided the tests are necessary, and a DEPS exception was added to allow ARC conversion to go forward.