New issue
Advanced search Search tips

Issue 778436 link

Starred by 1 user
Status: Fixed
Owner:
jsaul@google.com
Closed: Jan 2018
Cc:
ma...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task


Show other hotlists

Hotlists containing this issue:
Autofill-Fixit


Sign in to add a comment

Refactor usage of Test Autofill-based classes so that only one version exists

Project Member Reported by jsaul@google.com, Oct 25 2017 
Every unit test class needs to make several fake "Test~~" classes to suit its needs, but it becomes unwieldly and unknown how much code is necessary and what might just be plumbing to get the fake to work.

Goal: Coalesce all uses of Test Autofill-based classes (such as TestPersonalDataManager) into a reusable Fake class.

See context:
https://chromium-review.googlesource.com/c/chromium/src/+/705774/2/components/autofill/core/browser/credit_card_save_manager_unittest.cc#171

Comment 1 by jsaul@google.com, Dec 6 2017

Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1faef906ffa2ac90f5beafd27660152418137d36

commit 1faef906ffa2ac90f5beafd27660152418137d36
Author: Jared Saul <jsaul@google.com>
Date: Wed Dec 06 22:02:32 2017

Make AutofillManagerTest and CreditCardSaveManagerTest use a central TestPersonalDataManager

Bug:  778436 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I8ba2a695880a8594a145ecfa9724907253f5ad7f
Reviewed-on: https://chromium-review.googlesource.com/752074
Commit-Queue: Jared Saul <jsaul@google.com>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522218}
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/autofill/core/browser/address_combobox_model_unittest.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/autofill/core/browser/country_combobox_model_unittest.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/autofill/core/browser/credit_card_save_manager_unittest.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/autofill/core/browser/test_personal_data_manager.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/autofill/core/browser/test_personal_data_manager.h
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/payments/content/payment_request_state_unittest.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/components/payments/content/payment_response_helper_unittest.cc
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/payments/payment_request_unittest.mm
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/payments/payment_request_unittest_base.h
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/payments/payment_request_unittest_base.mm
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/payments/payment_response_helper_unittest.mm
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/ui/payments/address_edit_coordinator_unittest.mm
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/ui/payments/contact_info_edit_coordinator_unittest.mm
[modify] https://crrev.com/1faef906ffa2ac90f5beafd27660152418137d36/ios/chrome/browser/ui/payments/payment_request_coordinator_unittest.mm
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b746d5789c59f09e2a6cd2bb79480fef5de35cd

commit 6b746d5789c59f09e2a6cd2bb79480fef5de35cd
Author: Jared Saul <jsaul@google.com>
Date: Tue Dec 12 14:09:06 2017

Use existing TestPersonalDataManager in AutofillSaveCardInfoBarDelegateMobileTest

Bug:  778436 
Change-Id: I494ed476d5d00be5ad7ad36e24b45d934201d575
Reviewed-on: https://chromium-review.googlesource.com/821713
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523426}
[modify] https://crrev.com/6b746d5789c59f09e2a6cd2bb79480fef5de35cd/chrome/browser/autofill/autofill_save_card_infobar_delegate_mobile_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e28cfc7674edef7a95782243f679bec2610cf654

commit e28cfc7674edef7a95782243f679bec2610cf654
Author: Jared Saul <jsaul@google.com>
Date: Thu Dec 21 14:42:40 2017

Split TestFormDataImporter and TestPaymentsClient out of CreditCardSaveManagerTest

Even though these are the only usages of their respective test classes,
the goal is to have a single test implementation so that when needed,
other test files can extend them instead of re-implementing them all
over again.

Bug:  778436 
Change-Id: I6f0baa424bfecc4990fc1a6354bd8d781036f530
Reviewed-on: https://chromium-review.googlesource.com/838363
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525689}
[modify] https://crrev.com/e28cfc7674edef7a95782243f679bec2610cf654/components/autofill/core/browser/BUILD.gn
[modify] https://crrev.com/e28cfc7674edef7a95782243f679bec2610cf654/components/autofill/core/browser/credit_card_save_manager_unittest.cc
[add] https://crrev.com/e28cfc7674edef7a95782243f679bec2610cf654/components/autofill/core/browser/payments/test_payments_client.cc
[add] https://crrev.com/e28cfc7674edef7a95782243f679bec2610cf654/components/autofill/core/browser/payments/test_payments_client.h
[add] https://crrev.com/e28cfc7674edef7a95782243f679bec2610cf654/components/autofill/core/browser/test_form_data_importer.cc
[add] https://crrev.com/e28cfc7674edef7a95782243f679bec2610cf654/components/autofill/core/browser/test_form_data_importer.h
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d7dcfe72f073ca83053df71d531460549cd84f8

commit 9d7dcfe72f073ca83053df71d531460549cd84f8
Author: Jared Saul <jsaul@google.com>
Date: Sat Dec 23 06:21:16 2017

Split TestCreditCardSaveManager out of CreditCardSaveManagerTest

Even though this is the only usage of TestCreditCardSaveManager, the
goal is to have a single test implementation of it so that when needed,
other test files can extend it instead of re-implementing a duplicate of
it.

Bug:  778436 
Change-Id: I45da2aebf4990feca414746caff2c31fdf1eb5b6
Reviewed-on: https://chromium-review.googlesource.com/841384
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#526142}
[modify] https://crrev.com/9d7dcfe72f073ca83053df71d531460549cd84f8/components/autofill/core/browser/BUILD.gn
[modify] https://crrev.com/9d7dcfe72f073ca83053df71d531460549cd84f8/components/autofill/core/browser/credit_card_save_manager_unittest.cc
[add] https://crrev.com/9d7dcfe72f073ca83053df71d531460549cd84f8/components/autofill/core/browser/test_credit_card_save_manager.cc
[add] https://crrev.com/9d7dcfe72f073ca83053df71d531460549cd84f8/components/autofill/core/browser/test_credit_card_save_manager.h
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 25 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04351a65fc73c70e36cbab244070e4da61b89f1e

commit 04351a65fc73c70e36cbab244070e4da61b89f1e
Author: Jared Saul <jsaul@google.com>
Date: Mon Dec 25 04:58:21 2017

Refactor duplicated TestAutofillManagers into one centralized version

Bug:  778436 
Change-Id: Ieebb22b80e4d894aa1e54f9661bffa5bd8cf4949
Reviewed-on: https://chromium-review.googlesource.com/841540
Commit-Queue: Jared Saul <jsaul@google.com>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526167}
[modify] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/BUILD.gn
[modify] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/autofill_metrics_unittest.cc
[modify] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/credit_card_save_manager_unittest.cc
[add] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/test_autofill_manager.cc
[add] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/test_autofill_manager.h
[add] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/test_form_structure.cc
[add] https://crrev.com/04351a65fc73c70e36cbab244070e4da61b89f1e/components/autofill/core/browser/test_form_structure.h

Comment 8 by ma...@chromium.org, Jan 3 2018 

Hi Jared, can you list the remaining test things that you've identified need conversion?

Comment 9 by jsaul@google.com, Jan 6 2018 

I see three more I'd like to take care of:

* AutofillManagerTest's MockAutofillDriver extends TestAutofillDriver but adds a small amount of additional logic as well.  The logic it adds should really just go in TestAutofillDriver, so the Mock implementation is purely just mocking the test class.
* Ditto for TestAutofillDownloadManager.  It's used only in AutofillManagerTest, but it mocks some methods despite not being named MockAutofillDownloadManager.  Other instances of this pattern in the area delegate the logic to the test class, then mock the test class.
* TestAutofillExternalDelegate is implemented by both AutofillManagerTest and AutofillPopupControllerBrowserTest, so that's the last one that appears to be duplicated in multiple places.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 8 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b431fa88cbeccf1a602fada60d6b8e54e41ee47

commit 8b431fa88cbeccf1a602fada60d6b8e54e41ee47
Author: Jared Saul <jsaul@google.com>
Date: Mon Jan 08 14:07:57 2018

[Autofill] Move TestAutofillDriver logic out of MockAutofillDriver

Bug:  778436 
Change-Id: I76e3d9fe33ffbdf985bec3ae00500aa735a595dd
Reviewed-on: https://chromium-review.googlesource.com/852983
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527630}
[modify] https://crrev.com/8b431fa88cbeccf1a602fada60d6b8e54e41ee47/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/8b431fa88cbeccf1a602fada60d6b8e54e41ee47/components/autofill/core/browser/test_autofill_driver.cc
[modify] https://crrev.com/8b431fa88cbeccf1a602fada60d6b8e54e41ee47/components/autofill/core/browser/test_autofill_driver.h
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df9f6d470e736872ace211ae745319cbe8c72723

commit df9f6d470e736872ace211ae745319cbe8c72723
Author: Jared Saul <jsaul@google.com>
Date: Fri Jan 12 02:45:18 2018

[Autofill] Merge duplicate TestAutofillExternalDelegate implementations

Bug:  778436 
Change-Id: Ib15f9521344adf52423703053514bebf12ee1900
Reviewed-on: https://chromium-review.googlesource.com/855819
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Commit-Queue: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#528860}
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/chrome/browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/components/autofill/core/browser/autofill_external_delegate_unittest.cc
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/components/autofill/core/browser/autofill_test_utils.cc
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/components/autofill/core/browser/autofill_test_utils.h
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/components/autofill/core/browser/test_autofill_external_delegate.cc
[modify] https://crrev.com/df9f6d470e736872ace211ae745319cbe8c72723/components/autofill/core/browser/test_autofill_external_delegate.h

Comment 13 by jsaul@google.com, Jan 12 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment