New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 727825 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 701032



Sign in to add a comment

Make UserEventService unit testable

Project Member Reported by s...@chromium.org, May 30 2017

Issue description

This is a follow up to the comments on https://codereview.chromium.org/2897563002/ . Reviewers ask for unit tests and renjieliu@ points out that this is currently very difficult because the translate code is forced to depend directly on the concrete class's definition, which is currently called UserEventService.

We should be able to facilitate unit tests by simply breaking the interface and implementation into two separate classes, and providing a test implementation that, instead of calling into a bridge, simply accumulates changes for later verification against.

It's my understanding that the existing KeyedServiceFactory::SetTestingFactory() should be sufficient once the above split is created.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 1 2017

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

commit c794f9f4c22fe5047a78f5911e2fbcf75d161525
Author: skym <skym@chromium.org>
Date: Thu Jun 01 21:02:16 2017

[Sync] Split UserEventService into interface and impl, add fake impl, add unit tests.

BUG= 727825 

Review-Url: https://codereview.chromium.org/2909283003
Cr-Commit-Position: refs/heads/master@{#476434}

[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/chrome/browser/sync/user_event_service_factory.cc
[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/chrome/browser/sync/user_event_service_factory.h
[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/chrome/browser/ui/webui/sync_internals_message_handler.cc
[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc
[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/BUILD.gn
[add] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/fake_user_event_service.cc
[add] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/fake_user_event_service.h
[add] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/no_op_user_event_service.cc
[add] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/no_op_user_event_service.h
[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/user_event_service.cc
[modify] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/user_event_service.h
[add] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/user_event_service_impl.cc
[add] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/user_event_service_impl.h
[rename] https://crrev.com/c794f9f4c22fe5047a78f5911e2fbcf75d161525/components/sync/user_events/user_event_service_impl_unittest.cc

Comment 2 by s...@chromium.org, Jun 1 2017

Status: Fixed (was: Started)

Sign in to add a comment