ErrorPageTestCase relies on real networking error |
|||||||||||||||
Issue descriptionThese tests use error_page_response_provider.h, which use real (but not existing) URL to generate DNS error. This test should be either externalURLTest or we should find other way to test this. Possibly Chromium http server.
,
Apr 6 2017
This bug is causing the following test to be flaky on devices: testActivityServiceControllerPrintAfterRedirectionToUnprintablePage testErrorPage testErrorPageRedirect testHistoryForwardToErrorPage So, I'm marking this bug as RBB.
,
Apr 6 2017
We know that flakiness is not caused by a bug in the app. Is there a reason for making this RBB?
,
Apr 6 2017
A number of tests are disabled because of this bug, but I'm not sure if this is the right process. +cmasso, what do you think?
,
Apr 6 2017
Eugene, I believe we do RBB not because of app/test bugs, but because we're losing test coverage that would fail to catch an app bug that comes in the future. The outcome from this could be that the test should be re-written, deleted, or fixed, and based on that we can decide to remove the label.
,
Apr 6 2017
baxley@ said it all!
,
Apr 10 2017
Menglu, are you working on this?
,
Apr 10 2017
Not yet. To fix it instead of moving to external url tests, we need to find a way to get error page without network.
,
Apr 13 2017
I think we should move these tests to external url test suite. We don't have a way to hit DNS error without network. The embeddedTestServer doesn't support it as well.
,
Apr 13 2017
What happens if you use HandleCloseSocket or HandleNoContent from embedded test server?
,
Apr 13 2017
It will show blank pages for both cases (The handler returns a new basic response). According to the response type that the EmbeddedTestServer provides, it doesn't seem to be able to get error pages.
,
Apr 14 2017
Can we make EmbeddedTestServer return the same think as https://null.badssl.com/ ?
,
Apr 18 2017
,
Apr 20 2017
Kindly ping, Is there some new development in this regard? Or should tests be moved to externalURL with a TODO?
,
Apr 20 2017
Yes, Menglu moving these to external URL tests: https://codereview.chromium.org/2835563002/
,
Apr 21 2017
I haven't find a way to trigger error pages yet. I'm moving them to external url tests first while figuring it out.
,
Apr 21 2017
I have no objections!
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24efb2134c61d1a276c082aca21e7b681166396c commit 24efb2134c61d1a276c082aca21e7b681166396c Author: huangml <huangml@chromium.org> Date: Fri Apr 21 19:53:04 2017 Add ErrorPageTestCase to external url test suite. BUG= 694662 Review-Url: https://codereview.chromium.org/2835563002 Cr-Commit-Position: refs/heads/master@{#466421} [modify] https://crrev.com/24efb2134c61d1a276c082aca21e7b681166396c/ios/chrome/browser/ui/BUILD.gn [rename] https://crrev.com/24efb2134c61d1a276c082aca21e7b681166396c/ios/chrome/browser/ui/external_url_error_page_egtest.mm [modify] https://crrev.com/24efb2134c61d1a276c082aca21e7b681166396c/ios/chrome/test/earl_grey/BUILD.gn
,
Apr 21 2017
,
May 2 2017
,
Sep 5 2017
,
Oct 18 2017
There is still at least one test disabled because ErrorPageTestCase relies on real networking error. Looks for TODO( crbug.com/694662 ) in the code.
,
Oct 18 2017
,
Oct 18 2017
,
Apr 27 2018
I'm going to rewrite all tests to use EmbeddedTestServer.
,
Apr 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edb008a80d498fc70a127cc8777209854c851cb1 commit edb008a80d498fc70a127cc8777209854c851cb1 Author: Eugene But <eugenebut@google.com> Date: Mon Apr 30 17:33:19 2018 Replace ExternalURLErrorPageTestCase.testErrorPage with hermetic test. Old test was loading http://mock/redirect and because this URL did not exist the load was failing because of DNS resolution error. New test uses EmbeddedTestServer, which closes the socket to simulate ERR_INTERNET_DISCONNECTED error. Bug: 694662 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Ib8974954403f868aa8b41df49da8e83a7902a61e Reviewed-on: https://chromium-review.googlesource.com/1033952 Reviewed-by: Danyao Wang <danyao@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#554805} [modify] https://crrev.com/edb008a80d498fc70a127cc8777209854c851cb1/ios/chrome/browser/ui/external_url_error_page_egtest.mm [modify] https://crrev.com/edb008a80d498fc70a127cc8777209854c851cb1/ios/chrome/browser/web/BUILD.gn [add] https://crrev.com/edb008a80d498fc70a127cc8777209854c851cb1/ios/chrome/browser/web/error_page_egtest.mm
,
Apr 30 2018
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7408a8454b771876b709be263bb5636ec26c7f5e commit 7408a8454b771876b709be263bb5636ec26c7f5e Author: Eugene But <eugenebut@google.com> Date: Wed May 02 05:39:02 2018 Replace External URL testErrorPageRedirect with hermetic test. Old test was loading http://mock/bad and because this URL did not exist the load was failing because of DNS resolution error. New test uses EmbeddedTestServer, which closes the socket to simulate ERR_INTERNET_DISCONNECTED error. Bug: 694662 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Iaca523e8153a2c4cca0e1f174652ed48f22cabd3 Reviewed-on: https://chromium-review.googlesource.com/1035610 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/heads/master@{#555316} [modify] https://crrev.com/7408a8454b771876b709be263bb5636ec26c7f5e/ios/chrome/browser/ui/external_url_error_page_egtest.mm [modify] https://crrev.com/7408a8454b771876b709be263bb5636ec26c7f5e/ios/chrome/browser/web/error_page_egtest.mm [modify] https://crrev.com/7408a8454b771876b709be263bb5636ec26c7f5e/ios/web/public/test/http_server/error_page_response_provider.h [modify] https://crrev.com/7408a8454b771876b709be263bb5636ec26c7f5e/ios/web/public/test/http_server/error_page_response_provider.mm
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a906972c3b29dba238ecd98008a2ad4c07f89eb1 commit a906972c3b29dba238ecd98008a2ad4c07f89eb1 Author: Eugene But <eugenebut@google.com> Date: Wed May 02 22:15:14 2018 Replace ExternalURLErrorPageTestCase iframe tests with hermetic test. Old tests were loading http://mock/bad and because this URL did not exist the load was failing because of DNS resolution error. There were 3 tests to test frame load with and without timeout: testErrorPageInIFrame testErrorPageInIFrameAfterDelay testErrorPageNoUserInteraction (name did not reflect what the test did) And it was useful to test these 2 cases because UIWebView called load failure callbacks for iframes, and the error page could show up after iframe load failure. WKWebView callbacks are not called for iframes, so it's enough to have one test which verifies that web view loaded an iframe. New test uses EmbeddedTestServer, which closes the socket to simulate ERR_INTERNET_DISCONNECTED error. Bug: 694662 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I7e2021bbfec1539dd8787f40f20bfec6ad784700 Reviewed-on: https://chromium-review.googlesource.com/1038581 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Eric Roman <eroman@chromium.org> Reviewed-by: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/heads/master@{#555574} [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/chrome/browser/ui/BUILD.gn [delete] https://crrev.com/7ce4bde8653b60682450006329116e1d91173b98/ios/chrome/browser/ui/external_url_error_page_egtest.mm [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/chrome/browser/web/BUILD.gn [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/chrome/browser/web/error_page_egtest.mm [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/chrome/test/earl_grey/BUILD.gn [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/chrome/test/earl_grey/chrome_earl_grey.h [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/chrome/test/earl_grey/chrome_earl_grey.mm [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/testing/BUILD.gn [modify] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/testing/DEPS [add] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/testing/embedded_test_server_handlers.cc [add] https://crrev.com/a906972c3b29dba238ecd98008a2ad4c07f89eb1/ios/testing/embedded_test_server_handlers.h
,
May 3 2018
,
Jun 8 2018
Actually there are still 2 tests which rely on DNS resolution error
,
Oct 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2bd13b06dfd2fe72965e80597a58294ba12f811b commit 2bd13b06dfd2fe72965e80597a58294ba12f811b Author: Eugene But <eugenebut@chromium.org> Date: Mon Oct 22 16:17:10 2018 Remove kWebErrorPages flag. Notable changes: - removed redundant tests from ErrorPageTestCase (error_page_inttest cover these cases) - removed ErrorPageGenerator class - removed ErrorPageContent class - removed code which shows alert for non-printable pages Bug: 725241 , 694662 , 840489 , 859910 Change-Id: I0cad718200a3682633c0babf9b57b45cff39a579 Reviewed-on: https://chromium-review.googlesource.com/c/1289995 Reviewed-by: Danyao Wang <danyao@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#601604} [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/app/strings/ios_strings.grd [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/about_flags.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/ios_chrome_flag_descriptions.cc [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/ios_chrome_flag_descriptions.h [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/prerender/preload_controller.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/ui/activity_services/activity_service_controller_egtest.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/ui/browser_view_controller_unittest.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/web/BUILD.gn [delete] https://crrev.com/4676d794a8801cd5ae31b745ee124431d174f688/ios/chrome/browser/web/error_page_content.h [delete] https://crrev.com/4676d794a8801cd5ae31b745ee124431d174f688/ios/chrome/browser/web/error_page_content.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/chrome/browser/web/error_page_egtest.mm [delete] https://crrev.com/4676d794a8801cd5ae31b745ee124431d174f688/ios/chrome/browser/web/error_page_generator.h [delete] https://crrev.com/4676d794a8801cd5ae31b745ee124431d174f688/ios/chrome/browser/web/error_page_generator.mm [delete] https://crrev.com/4676d794a8801cd5ae31b745ee124431d174f688/ios/chrome/browser/web/error_page_generator_unittest.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/features.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/navigation/error_retry_state_machine.h [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/navigation/error_retry_state_machine.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/navigation/error_retry_state_machine_unittest.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/public/features.h [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/public/test/fakes/test_native_content_provider.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/public/web_state/ui/crw_native_content_provider.h [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/web_state/error_page_inttest.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/web_state/ui/crw_web_controller.mm [modify] https://crrev.com/2bd13b06dfd2fe72965e80597a58294ba12f811b/ios/web/web_state/web_state_observer_inttest.mm
,
Oct 22
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by sczs@chromium.org
, Feb 22 2017Status: Assigned (was: Untriaged)