Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2726433004:
Disable 4 unit tests that are failing with PlzNavigate and Site Isolation. (Closed)
DescriptionDisable 4 unit tests that are failing with PlzNavigate and Site Isolation.
These are
CaptivePortalTabHelperTest.HttpsSubframeParallelError
CorePageLoadMetricsObserverTest.FailedProvisionalLoad
CaptivePortalTabHelperTest.HttpToHttpsRedirectTimeout
SBNavigationObserverTest.ServerRedirect
These are the last test failures on desktop and it's suspected they'll be fixed by Camille's upcoming NavigationSimulator.
BUG=674734
Review-Url: https://codereview.chromium.org/2726433004
Cr-Commit-Position: refs/heads/master@{#453820}
Committed: https://chromium.googlesource.com/chromium/src/+/255cffbde1d757fb114a07b648ac72837a9cd7ad
Patch Set 1 #
Total comments: 6
Patch Set 2 : review comments #Patch Set 3 : nit #
Messages
Total messages: 25 (16 generated)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jam@chromium.org changed reviewers: + nasko@chromium.org
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with the comments addressed. https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc (right): https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:386: base::CommandLine::ForCurrentProcess()->HasSwitch( This should be AreAllSitesIsolatedForTesting() instead of checking for the specific command line switch. https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:5: #include "chrome/browser/safe_browsing/safe_browsing_navigation_observer.h" nit: Empty line after this include.
https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc (right): https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:386: base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/03/01 00:24:00, nasko (slow) wrote: > This should be AreAllSitesIsolatedForTesting() instead of checking for the > specific command line switch. Done. https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:5: #include "chrome/browser/safe_browsing/safe_browsing_navigation_observer.h" On 2017/03/01 00:24:00, nasko (slow) wrote: > nit: Empty line after this include. this was changed by git cl format, so if I change it now it'll change the next time the headers are changed..
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2726433004/#ps40001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:5: #include "chrome/browser/safe_browsing/safe_browsing_navigation_observer.h" On 2017/03/01 00:58:31, jam wrote: > On 2017/03/01 00:24:00, nasko (slow) wrote: > > nit: Empty line after this include. > > this was changed by git cl format, so if I change it now it'll change the next > time the headers are changed.. git cl format respects ordering within sets of headers separated by empty lines, therefore it "should just work" or so I am told :).
The CQ bit was unchecked by jam@chromium.org
https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc (right): https://codereview.chromium.org/2726433004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc:5: #include "chrome/browser/safe_browsing/safe_browsing_navigation_observer.h" On 2017/03/01 01:03:32, nasko (slow) wrote: > On 2017/03/01 00:58:31, jam wrote: > > On 2017/03/01 00:24:00, nasko (slow) wrote: > > > nit: Empty line after this include. > > > > this was changed by git cl format, so if I change it now it'll change the next > > time the headers are changed.. > > git cl format respects ordering within sets of headers separated by empty lines, > therefore it "should just work" or so I am told :). Done.
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2726433004/#ps60001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488331356207500,
"parent_rev": "c05f2493ffa65543fae98cfd4ae10dcd11421660", "commit_rev":
"255cffbde1d757fb114a07b648ac72837a9cd7ad"}
Message was sent while issue was closed.
Description was changed from ========== Disable 4 unit tests that are failing with PlzNavigate and Site Isolation. These are CaptivePortalTabHelperTest.HttpsSubframeParallelError CorePageLoadMetricsObserverTest.FailedProvisionalLoad CaptivePortalTabHelperTest.HttpToHttpsRedirectTimeout SBNavigationObserverTest.ServerRedirect These are the last test failures on desktop and it's suspected they'll be fixed by Camille's upcoming NavigationSimulator. BUG=674734 ========== to ========== Disable 4 unit tests that are failing with PlzNavigate and Site Isolation. These are CaptivePortalTabHelperTest.HttpsSubframeParallelError CorePageLoadMetricsObserverTest.FailedProvisionalLoad CaptivePortalTabHelperTest.HttpToHttpsRedirectTimeout SBNavigationObserverTest.ServerRedirect These are the last test failures on desktop and it's suspected they'll be fixed by Camille's upcoming NavigationSimulator. BUG=674734 Review-Url: https://codereview.chromium.org/2726433004 Cr-Commit-Position: refs/heads/master@{#453820} Committed: https://chromium.googlesource.com/chromium/src/+/255cffbde1d757fb114a07b648ac... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/255cffbde1d757fb114a07b648ac... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
