ios_chrome_unittests fail with slim-navigation-manager |
||||
Issue descriptionThese tests are failing with --enable-features=SlimNavigationManager: TabModelTest.RestoreSessionOn2NtpTest TabModelTest.AddWithLinkTransitionAndIndex TabModelTest.RestoreSessionOnNTPTest TabModelTest.RestoreSessionOnAnyTest TabModelTest.AddWithOrderControllerAndGrouping TabTest.GetSuggestedFilenameFromDefaultName TabTest.GetSuggestedFilenameFromContentDisposition TabTest.GetSuggestedFilenameFromURL TabTest.AddToHistoryWithRedirect VoiceSearchNavigationsTest.NavigationAfterVoiceSearch VoiceSearchNavigationsTest.TransientNavigations VoiceSearchNavigationsTest.CommittedVoiceSearchNavigation TextToSpeechListenerTest.ValidAudioDataTest IOSCaptivePortalBlockingPageTest.PresentAndDismiss
,
Aug 15
TabModelTest and TabTest failures are due to their direct use of NavigationManagerImpl->CommitPendingItem(). The tests are not able to satisfy a precondition for this internal API, which is that the navigation has already committed in the underlying WKWebView and has a corresponding WKBackForwardListItem. Summary from email exchange w/ eugenebut@ and rohitrao@: Three options to fix these tests: 1. Disable these tests for slim nav: brute force, loses significant coverage 2. Add embedded test server to this test: makes tests slower 3. Change tab_model_unittest to use TestWebState and TestNavigationManager: ideal long term solution, but not viable now as Sylvain pointed out in https://chromium-review.googlesource.com/c/chromium/src/+/794849. === Rohit is OK with #2: We will someday remove TabModel in favor of WebStateList, but we're only making incremental progress and it will take a long time to get it all done. === Eugene prefers EG tests that test how TabModel affects the UI: RestoreSessionOnAnyTest EG version would restore the session and check the number of tabs and current tab. AddWithLinkTransitionAndIndex EG version would check the tabs order. I'm not sure if we want to spend time on writing unit test for a class which we plan to remove. The drawback would be slower tests. The benefits are unclear giving that we will unlikely change TabModel implementation. EG test on the other hand will provide end to end coverage. EG test will be useful when we remove TabModel class (having EG tests may help with preventing regressions; unit test will not be useful). Also in the unlikely event of actually changing the implementation of TabModel, EG test should help with catching the regressions too. ===
,
Aug 15
IOSCaptivePortalBlockingPageTest.PresentAndDismiss is passing now.
,
Aug 15
TextToSpeechListenerTest.ValidAudioDataTest can be fixed by the same solution to VoiceSearchNavigationTest.
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ee7db1c740c18c1b5b68343e490f08d571a4ea4e commit ee7db1c740c18c1b5b68343e490f08d571a4ea4e Author: Danyao Wang <danyao@chromium.org> Date: Thu Aug 16 00:02:45 2018 [Nav Experiment] Mock out WKWebView in TabTest. TabTest uses the internal NavigationManagerImpl::CommitPendingItem() API. WKBasedNavigationManager's implementation of this API requires a precondition that a WKBackForwardListItem exists for the to be committed pending item. Mocking out WKWebView allows injection of WKBackForwardList item to satisfy this precondition. Parameterized TabTest to test both navigation manager implementations on trybot. Bug: 863026 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5f6b2faa94c1c63682a333c9f8c7c2600017cdea Reviewed-on: https://chromium-review.googlesource.com/1176376 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#583454} [modify] https://crrev.com/ee7db1c740c18c1b5b68343e490f08d571a4ea4e/ios/chrome/browser/tabs/BUILD.gn [modify] https://crrev.com/ee7db1c740c18c1b5b68343e490f08d571a4ea4e/ios/chrome/browser/tabs/DEPS [modify] https://crrev.com/ee7db1c740c18c1b5b68343e490f08d571a4ea4e/ios/chrome/browser/tabs/tab_unittest.mm
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9f6025aae6b05646b633c010f7d1dad7563f0fd4 commit 9f6025aae6b05646b633c010f7d1dad7563f0fd4 Author: Danyao Wang <danyao@chromium.org> Date: Fri Aug 17 00:18:01 2018 [Nav Experiment] Introduce kLoadingPlaceholder state to error retry. And also skips WebStateObserver::OnNavigationItemCommitted() callback if the navigation has a placeholder URL. These changes are helpful to differentiate a new request that hasn't started error handling state transitions, which means it may still succeed, and a request that has already failed and is in the middle of loading the error view. The differentiation is useful for integrating offline reading list with slim nav (crbug.com/840782). More directly, this change fixes VoiceSearchNavigationTest, which needs a placeholder entry to be loaded into the WebView before calling LoadHtml(), but the placeholder entry must not consume the WillLoadVoiceSearchResult() flag, which is cleared in OnNavigationItemCommitted callback. Parameterized VoiceSearchNavigationTest on both navigation manager implementations. This situation does not arise in production because LoadHtml() is a test only API. In production, placeholder URLs are only triggered for app-specific URL or error view, neither of which triggers OnNavigationItemCommitted on the placeholder load. Bug: 863026 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ic4eda9f46b59886a2d24169b630aa8179373f7fb Reviewed-on: https://chromium-review.googlesource.com/1176240 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#583898} [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/chrome/browser/voice/BUILD.gn [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/chrome/browser/voice/voice_search_navigations_tab_helper_unittest.mm [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/navigation/error_retry_state_machine.h [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/navigation/error_retry_state_machine.mm [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/navigation/error_retry_state_machine_unittest.mm [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/public/test/web_test_with_web_state.mm [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/web_state/ui/crw_web_controller_unittest.mm [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/web_state/web_state_impl.mm [modify] https://crrev.com/9f6025aae6b05646b633c010f7d1dad7563f0fd4/ios/web/web_state/web_state_impl_unittest.mm
,
Sep 7
,
Sep 12
,
Sep 14
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/271a2fecb1d42438099a9359b43db893c8ae13d3 commit 271a2fecb1d42438099a9359b43db893c8ae13d3 Author: Danyao Wang <danyao@chromium.org> Date: Mon Sep 24 22:40:45 2018 [Nav Experiment] Add TabOrderTestCase to replace some TabModelTest cases TabModelTest.AddWithOrderControllerAndGrouping is converted into TabOrderTestCase/testChildTabOrdering. TabModelTest.AddWithLinkTransitionAndIndex is removed because the behavior it tests, i.e. child tabs being inserted before parent tab, is not possible using public interfaces. Child tabs are always inserted after the parent tab. The original tests use NavigationManager::CommitPendingItem() with a test server. This is no longer possible with WKBasedNavigationManagerImpl. Updating the original tests to use embedded test server was rejected because TabModel is a deprecated class. Converting them to EG test allows the behavior to be tested with WKBasedNavigationManager and during a future removal of TabModel. Bug: 863026 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: Ia8f2b604cab93cba38142cac384a356153e95d9b Reviewed-on: https://chromium-review.googlesource.com/1240741 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#593721} [modify] https://crrev.com/271a2fecb1d42438099a9359b43db893c8ae13d3/ios/chrome/browser/tabs/tab_model_unittest.mm [modify] https://crrev.com/271a2fecb1d42438099a9359b43db893c8ae13d3/ios/chrome/browser/web/BUILD.gn [add] https://crrev.com/271a2fecb1d42438099a9359b43db893c8ae13d3/ios/chrome/browser/web/tab_order_egtest.mm
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f34febf2b1e8fcb6ce8796d242bdfee1849d620b commit f34febf2b1e8fcb6ce8796d242bdfee1849d620b Author: Danyao Wang <danyao@chromium.org> Date: Tue Sep 25 15:09:58 2018 [Nav Experiment] Disable TabModelTest.RestoreSession* tests temporarily These tests are broken for WKBasedNavigationManager. They need to be migrated to EG test later. Also parameterize TabModelTest so both navigation manager implementations are tested on trybot. Bug: 863026 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: I871eb635ef820ab97cf2c4cfbe19e30b74d28227 Reviewed-on: https://chromium-review.googlesource.com/1240868 Commit-Queue: Danyao Wang <danyao@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#593936} [modify] https://crrev.com/f34febf2b1e8fcb6ce8796d242bdfee1849d620b/ios/chrome/browser/tabs/tab_model_unittest.mm
,
Sep 25
|
||||
►
Sign in to add a comment |
||||
Comment 1 by danyao@chromium.org
, Aug 15