Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
69583002:
Refactor some ProfileSyncServiceTests (Closed)
|
Created:
7 years, 1 month ago by rlarocque Modified:
7 years, 1 month ago CC:
chromium-reviews, tim+watch_chromium.org, rsimha+watch_chromium.org, haitaol+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor some ProfileSyncServiceTests
Create a new test framework class named ProfileSyncServiceSimpleTest and
use it to replace several ProfileSyncServiceTests. This newer framework
takes advantage of MockSyncBackendHost and allows us to use a real
ProfileSyncService in these tests.
The resulting tests are better at testing the ProfileSyncService, but
they no longer test any part of the SyncBackendHost code. This is a
desirable trade off because these are unit tests.
BUG=312994
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235661
Patch Set 1 #Patch Set 2 : Rever some unnecessary changes #Patch Set 3 : More reversions #
Total comments: 10
Patch Set 4 : Review fixes #Patch Set 5 : Add explicit virtual dtor #Patch Set 6 : Add another suppress tests #
Total comments: 4
Patch Set 7 : Reduce threading #Patch Set 8 : Use UI_MAINLOOP #Patch Set 9 : Rebase? #Messages
Total messages: 15 (0 generated)
Please review. This CL overlaps a bit with Pavel's work on some of the tests in this file (https://codereview.chromium.org/69583002/). I think the conflict won't be too bad, since I've forked my own test framework class and we're mostly not touching the same classes. We should coordinate when it comes time to commit this, though, just in case.
https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_service_unittest.cc (right): https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:57: ProfileSyncServiceTest() nit: this should have a virtual destructor btw https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:201: class ProfileSyncServiceSimpleTest : public ::testing::Test { nit: Comment about how this differs? https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:236: ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()) extract this into a standalone function both classes can use? (are there any other duplicated functions this could be applied to as well?) https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:346: TEST_F(ProfileSyncServiceSimpleTest, NotDisabledByPolicy) { Isn't this just a normal init test (which I agree we should have)? Perhaps it makes sense to set the managed pref after initialization, and ensure the sync service disables itself? https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:389: ExpectSyncBackendHostCreation(); test that a suppressed start doesn't initialize before unsuppresing?
Patch updated. PTAL. https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_service_unittest.cc (right): https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:201: class ProfileSyncServiceSimpleTest : public ::testing::Test { On 2013/11/13 21:06:45, Nicolas Zea wrote: > nit: Comment about how this differs? Added a comment that describes the code. The plan is to remove the ProfileSyncServiceTest test class soon. Pavel is working to move some of the other tests. I think we can refactor the JsController tests, too. Once that's done, there won't be anything left that relies on the ProfileSyncServiceTest. https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:236: ProfileOAuth2TokenServiceFactory::GetForProfile(profile_.get()) On 2013/11/13 21:06:45, Nicolas Zea wrote: > extract this into a standalone function both classes can use? (are there any > other duplicated functions this could be applied to as well?) Since we plan to remove the ProfileSyncServiceTest soon, I don't think that's a good idea. I'm willing to live with a bit of code duplication for now. https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:346: TEST_F(ProfileSyncServiceSimpleTest, NotDisabledByPolicy) { On 2013/11/13 21:06:45, Nicolas Zea wrote: > Isn't this just a normal init test (which I agree we should have)? Perhaps it > makes sense to set the managed pref after initialization, and ensure the sync > service disables itself? Yeah, it's pretty much a normal init test. I added it and named it this way when I realized that the old DisabledByPolicy was working in part because that test didn't issue any auth tokens. The disabled by policy functionality could have been broken and that test would not have noticed. I was hoping the name would encourage people making future modifications to this file to keep it in sync with the DisabledByPolicy test, so that it could act as a "control" for the experiment. Your idea is much better. It's more likely to break if either the enterprise policy doesn't work, or the test framework fails to take into account all the prerequisites for backend init. I've moved this test up a bit and renamed it to SuccessfulInitialization. I've also added a new one named DisabledByPolicyAfterInit. https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:389: ExpectSyncBackendHostCreation(); On 2013/11/13 21:06:45, Nicolas Zea wrote: > test that a suppressed start doesn't initialize before unsuppresing? I don't follow. Are you suggesting there should be more assertions in this test, or a separate test for the case where StopAndSuppress() is called before Initialize()?
https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... File chrome/browser/sync/profile_sync_service_unittest.cc (right): https://codereview.chromium.org/69583002/diff/50001/chrome/browser/sync/profi... chrome/browser/sync/profile_sync_service_unittest.cc:389: ExpectSyncBackendHostCreation(); On 2013/11/13 22:28:25, rlarocque wrote: > On 2013/11/13 21:06:45, Nicolas Zea wrote: > > test that a suppressed start doesn't initialize before unsuppresing? > > I don't follow. Are you suggesting there should be more assertions in this > test, or a separate test for the case where StopAndSuppress() is called before > Initialize()? More assertions. I.e, before you call UnsupressAndStart, verify that supressing actually prevents startup.
I've added the other test we discussed offline. PTAL.
LGTM with two questions https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service_unittest.cc (right): https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_unittest.cc:212: : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD | if these test don't involve threads, is the thread bundle still necessary? https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_unittest.cc:239: content::RunAllPendingInMessageLoop(content::BrowserThread::IO); Similar question about this?
Good points about the threading code. I think we can simplify it a bit. I've made some changes; let's see if the trybots accept them. https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service_unittest.cc (right): https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_unittest.cc:212: : thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD | On 2013/11/13 23:49:00, Nicolas Zea wrote: > if these test don't involve threads, is the thread bundle still necessary? Apparently yes. There seems to be something in the TestingProfile that posted to the IO thread. I tried cutting back to having just the IO_THREAD. That seems to work. https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_unittest.cc:239: content::RunAllPendingInMessageLoop(content::BrowserThread::IO); On 2013/11/13 23:49:00, Nicolas Zea wrote: > Similar question about this? I'm mostly sure this is unnecessary. The ThreadBundle should be RunLoop()ing as it gets destructed. I think this extra code was intended to handle the SBH <-> SBH::Core request and reply sequence which is no longer a part of this test. I tried removing this and it seems to work. We'll have to see if the trybots are OK with it.
On 2013/11/14 00:41:38, rlarocque wrote: > Good points about the threading code. I think we can simplify it a bit. I've > made some changes; let's see if the trybots accept them. > > https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... > File chrome/browser/sync/profile_sync_service_unittest.cc (right): > > https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... > chrome/browser/sync/profile_sync_service_unittest.cc:212: : > thread_bundle_(content::TestBrowserThreadBundle::REAL_DB_THREAD | > On 2013/11/13 23:49:00, Nicolas Zea wrote: > > if these test don't involve threads, is the thread bundle still necessary? > > Apparently yes. There seems to be something in the TestingProfile that posted > to the IO thread. I tried cutting back to having just the IO_THREAD. That > seems to work. > > https://codereview.chromium.org/69583002/diff/180002/chrome/browser/sync/prof... > chrome/browser/sync/profile_sync_service_unittest.cc:239: > content::RunAllPendingInMessageLoop(content::BrowserThread::IO); > On 2013/11/13 23:49:00, Nicolas Zea wrote: > > Similar question about this? > > I'm mostly sure this is unnecessary. The ThreadBundle should be RunLoop()ing as > it gets destructed. I think this extra code was intended to handle the SBH <-> > SBH::Core request and reply sequence which is no longer a part of this test. > > I tried removing this and it seems to work. We'll have to see if the trybots > are OK with it. Regarding the testing profile, I suspect you might be able to get away with just instantiating a MessageLoopForIO, rather than a real thread bundle.
> > Regarding the testing profile, I suspect you might be able to get away with just > instantiating a MessageLoopForIO, rather than a real thread bundle. I don't know about that. It wouldn't surprise me if there were a few DCHECKs to assert that we're on the UI thread. If not, it's certainly conceivable that someone could add them in the future. I think we need the thread bundle to get past those checks. What I can do, though, is use IO_MAINLOOP instead of REAL_IO_THREAD to avoid instantiating a real thread.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/69583002/320001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/69583002/320001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/69583002/320001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/69583002/320001
Message was sent while issue was closed.
Change committed as 235661 |
