Improve FakeSyncService and get rid of subclasses |
|||||||
Issue descriptionFakeSyncService is a minimal implementation that returns default values from (almost?) all methods. Many tests thus define their own subclasses of FakeSyncService (often called TestSyncService) that add some setters and override the corresponding getters. It'd be nice to just offer all those setters directly in FakeSyncService and get rid of all the subclasses.
,
Oct 25
Sounds good. For code consistency, we may want to revisit naming. I believe it's quite common in sync code to have Fake classes do exactly this: implement an abstract interface with empty behavior. In doubt, and if we want to introduce a test-only class with non-trivial logic, I would suggest TestSyncService, currently in use in many places, so it'd make sense if we truly get rid of the 15 subclasses.
,
Nov 5
,
Nov 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b9bf429b1c8443c5cddf54035cf9843669d7208 commit 1b9bf429b1c8443c5cddf54035cf9843669d7208 Author: Marc Treib <treib@chromium.org> Date: Mon Nov 05 15:25:23 2018 Introduce syncer::TestSyncService This is a minimal implementation that lets tests directly set all desired state. It's intended to eventually replace all the various TestSyncService implementations that many individual tests define. So far, it's used in SyncServiceUtilsTest only. Bug: 859874 Change-Id: Ic5aabce48790c6787c678c3aec8a81cb28634e65 Reviewed-on: https://chromium-review.googlesource.com/c/1314637 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#605335} [modify] https://crrev.com/1b9bf429b1c8443c5cddf54035cf9843669d7208/components/sync/BUILD.gn [modify] https://crrev.com/1b9bf429b1c8443c5cddf54035cf9843669d7208/components/sync/driver/sync_service_utils_unittest.cc [add] https://crrev.com/1b9bf429b1c8443c5cddf54035cf9843669d7208/components/sync/driver/test_sync_service.cc [add] https://crrev.com/1b9bf429b1c8443c5cddf54035cf9843669d7208/components/sync/driver/test_sync_service.h
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/633949c3a4cede92dae8d15f38d411858f5ac6d0 commit 633949c3a4cede92dae8d15f38d411858f5ac6d0 Author: Marc Treib <treib@chromium.org> Date: Wed Nov 07 08:08:55 2018 Merge autofill::TestSyncService into syncer::TestSyncService Less duplication == good. Most of the things in autofill::TestSyncService were already supported, a few missing ones are added in this CL. TBRing trivial change in payment_request_browsertest_base.h TBR=mathp Bug: 859874 Change-Id: I51c140c86efb95494f84eb38142d54fc32f74b14 Reviewed-on: https://chromium-review.googlesource.com/c/1319592 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Commit-Position: refs/heads/master@{#605991} [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/chrome/browser/ui/views/payments/payment_request_browsertest_base.h [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/autofill/core/browser/BUILD.gn [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/autofill/core/browser/autofill_experiments_unittest.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/autofill/core/browser/autofill_metrics_unittest.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/autofill/core/browser/credit_card_save_manager_unittest.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/autofill/core/browser/local_card_migration_manager_unittest.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/autofill/core/browser/personal_data_manager_unittest.cc [delete] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/components/autofill/core/browser/test_sync_service.cc [delete] https://crrev.com/1c9309376a15c980e1e0941cc2b545100ca2781e/components/autofill/core/browser/test_sync_service.h [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/sync/driver/fake_sync_service.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/sync/driver/fake_sync_service.h [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/sync/driver/sync_service_utils_unittest.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/sync/driver/test_sync_service.cc [modify] https://crrev.com/633949c3a4cede92dae8d15f38d411858f5ac6d0/components/sync/driver/test_sync_service.h
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01d07ebfa744be2a37cd42d6eeead4d2f4f51e1a commit 01d07ebfa744be2a37cd42d6eeead4d2f4f51e1a Author: Marc Treib <treib@chromium.org> Date: Wed Nov 07 16:41:48 2018 SuggestionsServiceImplTest: Use syncer::TestSyncService instead of defining a custom MockSyncService that isn't used as a mock. Bug: 859874 Change-Id: If1458426ff77a666a2be0dc08513a87b26ad76b0 Reviewed-on: https://chromium-review.googlesource.com/c/1322710 Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#606063} [modify] https://crrev.com/01d07ebfa744be2a37cd42d6eeead4d2f4f51e1a/components/suggestions/suggestions_service_impl_unittest.cc [modify] https://crrev.com/01d07ebfa744be2a37cd42d6eeead4d2f4f51e1a/components/sync/driver/fake_sync_service.h
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74ea7417166a78276e123e2329e56db065d8d4bf commit 74ea7417166a78276e123e2329e56db065d8d4bf Author: Marc Treib <treib@chromium.org> Date: Thu Nov 08 09:45:12 2018 Use syncer::TestSyncService in HistoryNoticeUtilsTest One less subclass of FakeSyncService to maintain :) Bug: 859874 Change-Id: I29d0f367227f59c2ab3b08eefefbd1b21bb35019 Reviewed-on: https://chromium-review.googlesource.com/c/1318922 Reviewed-by: Christian Dullweber <dullweber@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#606397} [modify] https://crrev.com/74ea7417166a78276e123e2329e56db065d8d4bf/components/browsing_data/core/history_notice_utils_unittest.cc
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24ab870c177893569e4559a6c9d495a60f21573b commit 24ab870c177893569e4559a6c9d495a60f21573b Author: Marc Treib <treib@chromium.org> Date: Thu Nov 08 09:57:25 2018 Use syncer::TestSyncService in UserEventServiceImplTest One less custom subclass of FakeSyncService :) Bug: 859874 Change-Id: Idfafda6dcb3ab705a6c6410d27dd4087b0d91699 Reviewed-on: https://chromium-review.googlesource.com/c/1318979 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: vitaliii <vitaliii@chromium.org> Cr-Commit-Position: refs/heads/master@{#606398} [modify] https://crrev.com/24ab870c177893569e4559a6c9d495a60f21573b/components/sync/user_events/user_event_service_impl_unittest.cc
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58a14a284058b304386b20d464684de94084977d commit 58a14a284058b304386b20d464684de94084977d Author: Marc Treib <treib@chromium.org> Date: Thu Nov 08 13:08:09 2018 Use TestSyncService in about_sync_util_unittest.cc Bug: 859874 Change-Id: I7416dee32bd2daf93976f2e63f397a76e9a30b72 Reviewed-on: https://chromium-review.googlesource.com/c/1326006 Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#606436} [modify] https://crrev.com/58a14a284058b304386b20d464684de94084977d/components/sync/driver/about_sync_util_unittest.cc
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66530d6db815436af289fcbfea14d779b36cf73e commit 66530d6db815436af289fcbfea14d779b36cf73e Author: Marc Treib <treib@chromium.org> Date: Thu Nov 08 13:17:01 2018 Cleanup syncer::FakeSyncService This removes a bunch of methods/members that aren't used (anymore). Bug: 859874 Change-Id: Ia3d0c3be3b1bfdaa9230a26e0ad44c9ea359bf43 Reviewed-on: https://chromium-review.googlesource.com/c/1326004 Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#606442} [modify] https://crrev.com/66530d6db815436af289fcbfea14d779b36cf73e/components/sync/driver/fake_sync_service.cc [modify] https://crrev.com/66530d6db815436af289fcbfea14d779b36cf73e/components/sync/driver/fake_sync_service.h
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fc26c5bcef94f9b53fea18207c4a54e0f167d417 commit fc26c5bcef94f9b53fea18207c4a54e0f167d417 Author: Marc Treib <treib@chromium.org> Date: Thu Nov 08 13:35:33 2018 Use syncer::TestSyncService in components/password_manager tests This replaces a custom subclass of FakeSyncService with the standard syncer::TestSyncService. It also removed another custom subclass that's just not used at all. Bug: 859874 Change-Id: I0ef20ca70a1fdc81649c856c287ed6f1432956f8 Reviewed-on: https://chromium-review.googlesource.com/c/1326149 Reviewed-by: Vaclav Brozek <vabr@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#606445} [modify] https://crrev.com/fc26c5bcef94f9b53fea18207c4a54e0f167d417/components/password_manager/core/browser/password_autofill_manager_unittest.cc [modify] https://crrev.com/fc26c5bcef94f9b53fea18207c4a54e0f167d417/components/password_manager/core/browser/password_bubble_experiment_unittest.cc [modify] https://crrev.com/fc26c5bcef94f9b53fea18207c4a54e0f167d417/components/sync/driver/test_sync_service.cc [modify] https://crrev.com/fc26c5bcef94f9b53fea18207c4a54e0f167d417/components/sync/driver/test_sync_service.h
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/739f4a75ae992a0837f2e7841a5f91258268562b commit 739f4a75ae992a0837f2e7841a5f91258268562b Author: Marc Treib <treib@chromium.org> Date: Thu Nov 08 16:38:29 2018 Use syncer::TestSyncService in SyncUsernameTestBase While we're here, also use the default (preferred) constructor of IdentityTestEnvironment. With this, there's no need anymore to create TokenService, SigninManager, SigninClient, etc manually. Bug: 859874 Change-Id: I765e0898ac232c70255a19bd05f2d2e694e6639e Reviewed-on: https://chromium-review.googlesource.com/c/1326506 Reviewed-by: Vaclav Brozek <vabr@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#606494} [modify] https://crrev.com/739f4a75ae992a0837f2e7841a5f91258268562b/components/password_manager/core/browser/sync_username_test_base.cc [modify] https://crrev.com/739f4a75ae992a0837f2e7841a5f91258268562b/components/password_manager/core/browser/sync_username_test_base.h
,
Nov 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/68d1640209e8818469222523d4d09ced6b66899c commit 68d1640209e8818469222523d4d09ced6b66899c Author: Marc Treib <treib@chromium.org> Date: Mon Nov 12 12:40:40 2018 UnifiedConsentServiceTest: Use syncer::TestSyncService instead of FakeSyncService TestSyncService already supports a good portion of the setters that are needed here, so this allows us to remove a bunch of boilerplate. Bug: 859874 Change-Id: I10a3e0cac0114d1df70b1ca72bec98f95aad0cb1 Reviewed-on: https://chromium-review.googlesource.com/c/1329795 Reviewed-by: Thomas Tangl <tangltom@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#607202} [modify] https://crrev.com/68d1640209e8818469222523d4d09ced6b66899c/components/unified_consent/unified_consent_service_unittest.cc
,
Nov 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69a0c405c5129b6b9ec690703988ee40a1af4d88 commit 69a0c405c5129b6b9ec690703988ee40a1af4d88 Author: Marc Treib <treib@chromium.org> Date: Tue Nov 13 09:53:09 2018 Cleanup: Remove unused FakeSyncService implementation in ntp_snippets Bug: 859874 Change-Id: Ib02931e14a22b2454b94b2021b65744441856475 Reviewed-on: https://chromium-review.googlesource.com/c/1326504 Reviewed-by: Filip Gorski <fgorski@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#607538} [modify] https://crrev.com/69a0c405c5129b6b9ec690703988ee40a1af4d88/components/ntp_snippets/remote/test_utils.cc [modify] https://crrev.com/69a0c405c5129b6b9ec690703988ee40a1af4d88/components/ntp_snippets/remote/test_utils.h
,
Nov 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0955e76b373804b6623c1adbc89af246066fa361 commit 0955e76b373804b6623c1adbc89af246066fa361 Author: Marc Treib <treib@chromium.org> Date: Wed Nov 14 14:56:50 2018 UrlKeyedDataCollectionConsentHelperTest: Use syncer::TestSyncService instead of syncer::FakeSyncService. TestSyncService already supports a good portion of the setters that are needed here, so this allows us to remove a bunch of boilerplate. Bug: 859874 Change-Id: I2520cfb526c81de566630dd7f956ddc45eabcee3 Reviewed-on: https://chromium-review.googlesource.com/c/1335593 Reviewed-by: Thomas Tangl <tangltom@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#607977} [modify] https://crrev.com/0955e76b373804b6623c1adbc89af246066fa361/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc
,
Nov 15
I think this is done :)
,
Nov 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/200318d56a2f658c725b104eec7a61d03581be57 commit 200318d56a2f658c725b104eec7a61d03581be57 Author: Marc Treib <treib@chromium.org> Date: Thu Nov 15 21:22:12 2018 SyncDisableObserverTest: Use TestSyncService instead of FakeSyncService syncer::TestSyncService already supports most of the state that's required here, so this lets us remove a bunch of boilerplate. Bug: 859874 Change-Id: I3c45e341f31c199176dd8173b6a858a8c704c15f Reviewed-on: https://chromium-review.googlesource.com/c/1335582 Commit-Queue: Ilya Sherman <isherman@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Cr-Commit-Position: refs/heads/master@{#608506} [modify] https://crrev.com/200318d56a2f658c725b104eec7a61d03581be57/components/ukm/observers/sync_disable_observer_unittest.cc
,
Nov 27
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 74dde08f5ffefc82d3bba924e632b5553e8da9cb was merged to refs/branch-heads/3578 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74dde08f5ffefc82d3bba924e632b5553e8da9cb Commit: 74dde08f5ffefc82d3bba924e632b5553e8da9cb Author: treib@chromium.org Commiter: treib@chromium.org Date: 2018-11-27 17:04:44 +0000 UTC Introduce syncer::TestSyncService This is a minimal implementation that lets tests directly set all desired state. It's intended to eventually replace all the various TestSyncService implementations that many individual tests define. So far, it's used in SyncServiceUtilsTest only. (cherry picked from commit 1b9bf429b1c8443c5cddf54035cf9843669d7208) Bug: 859874 Change-Id: Ic5aabce48790c6787c678c3aec8a81cb28634e65 Reviewed-on: https://chromium-review.googlesource.com/c/1314637 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#605335} Reviewed-on: https://chromium-review.googlesource.com/c/1350987 Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#821} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74dde08f5ffefc82d3bba924e632b5553e8da9cb commit 74dde08f5ffefc82d3bba924e632b5553e8da9cb Author: Marc Treib <treib@chromium.org> Date: Tue Nov 27 17:04:44 2018 Introduce syncer::TestSyncService This is a minimal implementation that lets tests directly set all desired state. It's intended to eventually replace all the various TestSyncService implementations that many individual tests define. So far, it's used in SyncServiceUtilsTest only. (cherry picked from commit 1b9bf429b1c8443c5cddf54035cf9843669d7208) Bug: 859874 Change-Id: Ic5aabce48790c6787c678c3aec8a81cb28634e65 Reviewed-on: https://chromium-review.googlesource.com/c/1314637 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#605335} Reviewed-on: https://chromium-review.googlesource.com/c/1350987 Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#821} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/74dde08f5ffefc82d3bba924e632b5553e8da9cb/components/sync/BUILD.gn [modify] https://crrev.com/74dde08f5ffefc82d3bba924e632b5553e8da9cb/components/sync/driver/sync_service_utils_unittest.cc [add] https://crrev.com/74dde08f5ffefc82d3bba924e632b5553e8da9cb/components/sync/driver/test_sync_service.cc [add] https://crrev.com/74dde08f5ffefc82d3bba924e632b5553e8da9cb/components/sync/driver/test_sync_service.h
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e5e246e95ec44a782404d1efe12c3075b4c7571 commit 2e5e246e95ec44a782404d1efe12c3075b4c7571 Author: Marc Treib <treib@chromium.org> Date: Tue Nov 27 17:05:15 2018 SyncDisableObserverTest: Use TestSyncService instead of FakeSyncService syncer::TestSyncService already supports most of the state that's required here, so this lets us remove a bunch of boilerplate. (cherry picked from commit 200318d56a2f658c725b104eec7a61d03581be57) Bug: 859874 Change-Id: I3c45e341f31c199176dd8173b6a858a8c704c15f Reviewed-on: https://chromium-review.googlesource.com/c/1335582 Commit-Queue: Ilya Sherman <isherman@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#608506} Reviewed-on: https://chromium-review.googlesource.com/c/1350988 Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#822} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/2e5e246e95ec44a782404d1efe12c3075b4c7571/components/ukm/observers/sync_disable_observer_unittest.cc
,
Nov 27
Merge approval for the two above CLs was requested (and granted) on bug 906609.
,
Nov 27
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 2e5e246e95ec44a782404d1efe12c3075b4c7571 was merged to refs/branch-heads/3578 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e5e246e95ec44a782404d1efe12c3075b4c7571 Commit: 2e5e246e95ec44a782404d1efe12c3075b4c7571 Author: treib@chromium.org Commiter: treib@chromium.org Date: 2018-11-27 17:05:15 +0000 UTC SyncDisableObserverTest: Use TestSyncService instead of FakeSyncService syncer::TestSyncService already supports most of the state that's required here, so this lets us remove a bunch of boilerplate. (cherry picked from commit 200318d56a2f658c725b104eec7a61d03581be57) Bug: 859874 Change-Id: I3c45e341f31c199176dd8173b6a858a8c704c15f Reviewed-on: https://chromium-review.googlesource.com/c/1335582 Commit-Queue: Ilya Sherman <isherman@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#608506} Reviewed-on: https://chromium-review.googlesource.com/c/1350988 Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#822} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 27
This merge was approved here - https://bugs.chromium.org/p/chromium/issues/detail?id=906609#c18 |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by treib@chromium.org
, Oct 24