Break page load metrics test dependency on IPC. |
||
Issue descriptionCurrently, PLM unit tests exercise the IPC dispatch path. This achieved slightly increased coverage but coupled unit tests to IPC. We should instead dispatch simulated timing events directly to callbacks, bypassing the IPC dispatch. Additionally, the browsertest has a dependency on IPC. We should add a MetricsWebContentsObserver::TestingObserver, which can observe state changes at the observer level, instead of watching for IPC messages. This both simplifies the logic and more directly verifies expected behavior at the appropriate level.
,
Apr 27 2017
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2cb37fe0beb13e6baa8852662362e9204d1ebfa3 commit 2cb37fe0beb13e6baa8852662362e9204d1ebfa3 Author: zmin <zmin@chromium.org> Date: Thu Apr 27 15:45:22 2017 Revert of Break page load metrics test dependency on IPC. (patchset #2 id:20001 of https://codereview.chromium.org/2847513002/ ) Reason for revert: PageLoadMetricsBrowserTest.DocumentWriteReloada is flaky https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20MSan%20Tests/387 Original issue's description: > Break page load metrics test dependency on IPC. > > This change breaks direct dependencies on IPC for page load metrics > tests, in order to ease the transition to mojo. > > Previously, unit tests exercised the IPC dispatch path. This > achieved slightly increased coverage but coupled unit tests to > IPC. We now dispatch simulated timing events directly to callbacks, > bypassing the IPC dispatch. > > Additionally, we break the browsertest dependency on IPC and add > a MetricsWebContentsObserver::TestingObserver, which can observe > state changes at the observer level, instead of watching for IPC > messages. This both simplifies the logic and more directly verifies > expected behavior at the appropriate level. > > BUG= 715744 > TBR=csharrison > > Review-Url: https://codereview.chromium.org/2847513002 > Cr-Commit-Position: refs/heads/master@{#467687} > Committed: https://chromium.googlesource.com/chromium/src/+/9567f9fa3fcfcaa50fc7b6a6adf66106a2eb6be5 TBR=lpy@chromium.org,bmcquade@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 715744 Review-Url: https://codereview.chromium.org/2847803002 Cr-Commit-Position: refs/heads/master@{#467691} [modify] https://crrev.com/2cb37fe0beb13e6baa8852662362e9204d1ebfa3/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc [modify] https://crrev.com/2cb37fe0beb13e6baa8852662362e9204d1ebfa3/chrome/browser/page_load_metrics/metrics_web_contents_observer.h [modify] https://crrev.com/2cb37fe0beb13e6baa8852662362e9204d1ebfa3/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc [modify] https://crrev.com/2cb37fe0beb13e6baa8852662362e9204d1ebfa3/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc [modify] https://crrev.com/2cb37fe0beb13e6baa8852662362e9204d1ebfa3/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/44d3a319f57f02f92beb9ceb0ad973d7d4d39c82 commit 44d3a319f57f02f92beb9ceb0ad973d7d4d39c82 Author: bmcquade <bmcquade@chromium.org> Date: Thu Apr 27 15:53:08 2017 Reland of Break page load metrics test dependency on IPC. (patchset #1 id:1 of https://codereview.chromium.org/2847803002/ ) Reason for revert: The flake that caused the previous revert by zmin existed before the original patch landed, so the original patch is not the cause of the flake. This refactor should help us to un-flake the test, so I'm re-landing it. Original issue's description: > Revert of Break page load metrics test dependency on IPC. (patchset #2 id:20001 of https://codereview.chromium.org/2847513002/ ) > > Reason for revert: > PageLoadMetricsBrowserTest.DocumentWriteReloada is flaky > > https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20MSan%20Tests/387 > > Original issue's description: > > Break page load metrics test dependency on IPC. > > > > This change breaks direct dependencies on IPC for page load metrics > > tests, in order to ease the transition to mojo. > > > > Previously, unit tests exercised the IPC dispatch path. This > > achieved slightly increased coverage but coupled unit tests to > > IPC. We now dispatch simulated timing events directly to callbacks, > > bypassing the IPC dispatch. > > > > Additionally, we break the browsertest dependency on IPC and add > > a MetricsWebContentsObserver::TestingObserver, which can observe > > state changes at the observer level, instead of watching for IPC > > messages. This both simplifies the logic and more directly verifies > > expected behavior at the appropriate level. > > > > BUG= 715744 > > TBR=csharrison > > > > Review-Url: https://codereview.chromium.org/2847513002 > > Cr-Commit-Position: refs/heads/master@{#467687} > > Committed: https://chromium.googlesource.com/chromium/src/+/9567f9fa3fcfcaa50fc7b6a6adf66106a2eb6be5 > > TBR=lpy@chromium.org,bmcquade@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 715744 > > Review-Url: https://codereview.chromium.org/2847803002 > Cr-Commit-Position: refs/heads/master@{#467691} > Committed: https://chromium.googlesource.com/chromium/src/+/2cb37fe0beb13e6baa8852662362e9204d1ebfa3 TBR=lpy@chromium.org,zmin@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 715744 Review-Url: https://codereview.chromium.org/2849563002 Cr-Commit-Position: refs/heads/master@{#467695} [modify] https://crrev.com/44d3a319f57f02f92beb9ceb0ad973d7d4d39c82/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc [modify] https://crrev.com/44d3a319f57f02f92beb9ceb0ad973d7d4d39c82/chrome/browser/page_load_metrics/metrics_web_contents_observer.h [modify] https://crrev.com/44d3a319f57f02f92beb9ceb0ad973d7d4d39c82/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc [modify] https://crrev.com/44d3a319f57f02f92beb9ceb0ad973d7d4d39c82/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc [modify] https://crrev.com/44d3a319f57f02f92beb9ceb0ad973d7d4d39c82/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Apr 27 2017