Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
335473004:
[SyncFS] Make SyncEngineTest multi-threaded test (Closed)
|
Created:
6 years, 6 months ago by peria Modified:
6 years, 6 months ago CC:
chromium-reviews, kinuko+fileapi, nhiroki, tzik Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[SyncFS] Make SyncEngineTest multi-threaded test.
Main target of this test is communication between
SyncEngine in UI thread and SyncWorker in a worker thread.
BUG=378621
TEST=./unit_tests --gtest_filter="SyncEngineTest.*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278680
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Remove local RunLoop instances #
Total comments: 8
Patch Set 3 : Work for nits #Patch Set 4 : arraysize does not support structures defined in a function #Messages
Total messages: 16 (0 generated)
PTL.
https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc (right): https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:41: public RemoteFileSyncService::Observer, Although you ignore the result in OnRemoteXXX(), do you need to observe the sync engine? If not, can you remove this and WaitAsyncCall()?
https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc (right): https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:41: public RemoteFileSyncService::Observer, On 2014/06/18 06:58:10, nhiroki wrote: > Although you ignore the result in OnRemoteXXX(), do you need to observe the sync > engine? If not, can you remove this and WaitAsyncCall()? Ah, I just got what you mean. You check the status update in the test (i.e. UpdateServiceState). Let me recall this comment.
https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc (right): https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:70: base::SequencedWorkerPool::SKIP_ON_SHUTDOWN); I think we can pass NULL to file_task_runner and drive_task_runner.
Thanks to Tsuiki-san's suggestion, many wasteful lines are removed!! https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc (right): https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:41: public RemoteFileSyncService::Observer, Due to a discussion with Tsuiki-san, yes, we can remove this inheritance. However, Observing SyncEngine is required to check all communications. So I'd like to add it again in future CLs. https://codereview.chromium.org/335473004/diff/120001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:70: base::SequencedWorkerPool::SKIP_ON_SHUTDOWN); On 2014/06/18 07:39:55, tzik wrote: > I think we can pass NULL to file_task_runner and drive_task_runner. Done.
Looks good! https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc (right): https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:67: base::RunLoop().RunUntilIdle(); Can we drop RunUntilIdle()? This will not work well for multi-threaded situation. https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:72: base::RunLoop().RunUntilIdle(); Could you keep |worker_pool| and call its Shutdown() here? https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:102: void WaitWorkerTaskRunner() { WaitForWorkerTaskRunner or FlushWorkerTaskRunner? https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:215: for (int i = 0; test_data[i].state != REMOTE_SERVICE_STATE_MAX; ++i) { i < arraysize(test_data)?
Updated. PTAL. https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc (right): https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:67: base::RunLoop().RunUntilIdle(); On 2014/06/18 12:08:58, tzik wrote: > Can we drop RunUntilIdle()? > This will not work well for multi-threaded situation. Done. https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:72: base::RunLoop().RunUntilIdle(); On 2014/06/18 12:08:58, tzik wrote: > Could you keep |worker_pool| and call its Shutdown() here? Done. https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:102: void WaitWorkerTaskRunner() { On 2014/06/18 12:08:58, tzik wrote: > WaitForWorkerTaskRunner or FlushWorkerTaskRunner? Done. https://codereview.chromium.org/335473004/diff/160001/chrome/browser/sync_fil... chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:215: for (int i = 0; test_data[i].state != REMOTE_SERVICE_STATE_MAX; ++i) { On 2014/06/18 12:08:58, tzik wrote: > i < arraysize(test_data)? Done.
lgtm
lgtm
The CQ bit was checked by peria@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/peria@chromium.org/335473004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
The CQ bit was checked by peria@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/peria@chromium.org/335473004/240001
Message was sent while issue was closed.
Change committed as 278680 |
