Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
177923004:
Moved CCInfobarDelegate tests out of AutofillMetricsTest (Closed)
|
Created:
6 years, 9 months ago by blundell Modified:
6 years, 9 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMoved CCInfobarDelegate tests out of AutofillMetricsTest
The AutofillMetrics unittests will be componentized. The infobar-related tests,
however, cannot be componentized, as infobars are currently a //chrome-level
concept. This CL splits out the infobar-related tests from AutofillMetricsTest
into a new AutofillCCInfobarDelegateTest.
BUG=303083
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255842
Patch Set 1 #
Total comments: 26
Patch Set 2 : Response to review #
Total comments: 4
Patch Set 3 : Response to review #
Messages
Total messages: 20 (0 generated)
Hi Ilya, Would you like to split out the MockAutofillMetrics and TestPersonalDataManager classes used by both the old and new tests? If so, where to? Thanks, Colin
On 2014/03/06 17:40:21, blundell wrote: > Would you like to split out the MockAutofillMetrics and > TestPersonalDataManager classes used by both the old and new tests? > If so, where to? Nah, I think we can instead dramatically simplify the test code supporting the credit card infobar metrics. See inline :) https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:19: #include "components/autofill/core/browser/form_structure.h" nit: This include is no longer needed. I bet that there are others -- especially the base/ includes -- that are no longer needed as well. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:26: using base::TimeTicks; nit: I don't think these three are needed. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:29: using testing::Mock; nit: Are these two still needed? https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:67: }; I've been meaning to kill this class entirely, actually. There's a handy class named StatisticsDeltaReader [1] which is a much better way to test metrics code. Would you be up for converting this test to use that code instead? If not, we can at least drop most of the mocked methods above. The only one that's really needed is the credit card infobar metric one. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/test/statisti... https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:73: set_metric_logger(new testing::NiceMock<MockAutofillMetrics>()); This shouldn't be needed anymore. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:74: CreateTestAutofillProfiles(&web_profiles_); Nor should this. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:87: ReceiveLoadedProfiles(0, &result); It should be fine to empty this out. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:96: } This shouldn't be needed anymore. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:102: virtual bool IsAutofillEnabled() const OVERRIDE { return autofill_enabled_; } Ditto. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:141: } Ditto. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:143: bool autofill_enabled_; Ditto. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:170: }; nit: This class doesn't seem to be used anywhere. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:229: testing::NiceMock<MockAutofillMetrics> metric_logger; nit: Shouldn't be a need for a NiceMock anymore once you remove the unrelated methods.
Thanks for the detailed review! Much cleaner now. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:19: #include "components/autofill/core/browser/form_structure.h" On 2014/03/06 23:12:02, Ilya Sherman wrote: > nit: This include is no longer needed. I bet that there are others -- > especially the base/ includes -- that are no longer needed as well. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:26: using base::TimeTicks; On 2014/03/06 23:12:02, Ilya Sherman wrote: > nit: I don't think these three are needed. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:29: using testing::Mock; On 2014/03/06 23:12:02, Ilya Sherman wrote: > nit: Are these two still needed? Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:67: }; I kept the usage of AutofillMetrics but trimmed it down to the minimum possible. On 2014/03/06 23:12:02, Ilya Sherman wrote: > I've been meaning to kill this class entirely, actually. There's a handy class > named StatisticsDeltaReader [1] which is a much better way to test metrics code. > Would you be up for converting this test to use that code instead? > > If not, we can at least drop most of the mocked methods above. The only one > that's really needed is the credit card infobar metric one. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/statisti... https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:73: set_metric_logger(new testing::NiceMock<MockAutofillMetrics>()); On 2014/03/06 23:12:02, Ilya Sherman wrote: > This shouldn't be needed anymore. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:74: CreateTestAutofillProfiles(&web_profiles_); On 2014/03/06 23:12:02, Ilya Sherman wrote: > Nor should this. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:87: ReceiveLoadedProfiles(0, &result); On 2014/03/06 23:12:02, Ilya Sherman wrote: > It should be fine to empty this out. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:96: } On 2014/03/06 23:12:02, Ilya Sherman wrote: > This shouldn't be needed anymore. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:102: virtual bool IsAutofillEnabled() const OVERRIDE { return autofill_enabled_; } On 2014/03/06 23:12:02, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:141: } On 2014/03/06 23:12:02, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:143: bool autofill_enabled_; On 2014/03/06 23:12:02, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:170: }; On 2014/03/06 23:12:02, Ilya Sherman wrote: > nit: This class doesn't seem to be used anywhere. Done. https://codereview.chromium.org/177923004/diff/1/chrome/browser/autofill/auto... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:229: testing::NiceMock<MockAutofillMetrics> metric_logger; On 2014/03/06 23:12:02, Ilya Sherman wrote: > nit: Shouldn't be a need for a NiceMock anymore once you remove the unrelated > methods. Done.
LGTM w/ a couple of nits. Thanks, Colin! https://codereview.chromium.org/177923004/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/177923004/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:7: #include <vector> nit: Doesn't seem like this is needed. https://codereview.chromium.org/177923004/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:45: // Overridden to avoid a trip to the database. nit: Maybe drop this comment as well as the blank line 44?
The CQ bit was checked by blundell@chromium.org
https://codereview.chromium.org/177923004/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc (right): https://codereview.chromium.org/177923004/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:7: #include <vector> On 2014/03/07 23:35:45, Ilya Sherman wrote: > nit: Doesn't seem like this is needed. Done. https://codereview.chromium.org/177923004/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_cc_infobar_delegate_unittest.cc:45: // Overridden to avoid a trip to the database. On 2014/03/07 23:35:45, Ilya Sherman wrote: > nit: Maybe drop this comment as well as the blank line 44? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/177923004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/177923004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/177923004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/177923004/40001
Message was sent while issue was closed.
Change committed as 255842 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
