Generalize page load metrics test infrastructure |
||||
Issue descriptionAs we are getting ready to add more PLMObservers which integrate with UKM, we should improve our UKM test infra in the test harness.
,
Jul 7 2017
,
Jul 10 2017
Reopening and assigning to myself. I would like to move the test infrastructure to TestUkmRecorder to share it even wider.
,
Jul 10 2017
Sure, that'd be great, thanks! We may want to keep this class separate from TestUkmRecorder, so test code has access to the convenience functions, without having direct access to the full TestUkmRecorder API (which includes e.g. EnableRecording/DisableRecording etc). See what you think.
,
Jul 10 2017
(to clarify, it may make sense to keep a UkmTester class that is a peer to TestUkmRecorder, rather than merging it with TestUkmRecorder, and provide only the UkmTester API to test code)
,
Jul 10 2017
My personal feeling is that the convenience of having to deal with a single instance outweighs the risk of exposing EnableRecording/DisableRecording. I think that both classes would typically be instantiated right next to each other.
,
Jul 10 2017
That seems ok to me. Will defer to holte and rkaplow if they have preferences, but I'm fine with having a single class (TestUkmRecorder).
,
Jul 10 2017
Since this is a test only class, I'm not too worried about anything misusing the Enable/Disable calls, I'd rather it be one class. Alternatively, you could change it to use private inheritance and expose a getter for the recorder e.g.
TestUkmRecorder : private UkmRecorderImpl {
UkmRecorder* recorder() { return this; }
,
Jul 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9aea53be83e8a2b33e1f3229ddf2de71abf60cee commit 9aea53be83e8a2b33e1f3229ddf2de71abf60cee Author: Dominic Battre <battre@chromium.org> Date: Tue Jul 11 10:56:11 2017 Enhance testing utility of TestUkmRecorder This CL moves the functionality of chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.* into the TestUkmRecorder to provide the utility to other code that needs to test URL keyed metrics. NOTRY=true Bug: 738921 Change-Id: I8c4e9101fc817e265415f4c45c174c5a1ef8ccc0 Reviewed-on: https://chromium-review.googlesource.com/565293 Commit-Queue: Dominic Battré <battre@chromium.org> Reviewed-by: Bryan McQuade <bmcquade@chromium.org> Reviewed-by: Steven Holte <holte@chromium.org> Cr-Commit-Position: refs/heads/master@{#485580} [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/chrome/browser/page_load_metrics/observers/local_network_requests_page_load_metrics_observer_unittest.cc [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc [delete] https://crrev.com/9dfa3c95d67ac0348f208a8cb631c606b5cfa54f/chrome/browser/page_load_metrics/test/ukm_tester.cc [delete] https://crrev.com/9dfa3c95d67ac0348f208a8cb631c606b5cfa54f/chrome/browser/page_load_metrics/test/ukm_tester.h [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/chrome/test/BUILD.gn [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/components/ukm/BUILD.gn [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/components/ukm/test_ukm_recorder.cc [modify] https://crrev.com/9aea53be83e8a2b33e1f3229ddf2de71abf60cee/components/ukm/test_ukm_recorder.h |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Jul 7 2017