Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799103002:
Enable printers sync_integration_tests on chromeos (Closed)
DescriptionEnable printers sync_integration_tests on chromeos
Fix printer flaky bugs by adding another wait.
The bug is flaky because tests only wait for UI tasks to be completed,
but since PrintersManagerFactory uses
content::BrowserThread::GetBlockingPool() to schedule ModelTypeStore
initialization, and then reply result to UI thread, so we need to wait
on content::BrowserThread::GetBlockingPool() as well.
Remove unnecessary changes from
https://codereview.chromium.org/2758643002 since that did not really
fix this waiting issue.
BUG=689662, 701999
Review-Url: https://codereview.chromium.org/2799103002
Cr-Commit-Position: refs/heads/master@{#462716}
Committed: https://chromium.googlesource.com/chromium/src/+/864d4cde3b40817cf6831a4a2e6711a4f94413b9
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : Add comments #
Total comments: 8
Patch Set 4 : update #Patch Set 5 : update comments #
Total comments: 11
Patch Set 6 : more comments #
Total comments: 2
Patch Set 7 : add todo #
Messages
Total messages: 45 (34 generated)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Enable printers sync_integration_tests on chromeos BUG=689662 ========== to ========== Enable printers sync_integration_tests on chromeos revert https://codereview.chromium.org/2758643002 as well. BUG=689662 ==========
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable printers sync_integration_tests on chromeos revert https://codereview.chromium.org/2758643002 as well. BUG=689662 ========== to ========== Enable printers sync_integration_tests on chromeos revert https://codereview.chromium.org/2758643002 as well. BUG=689662, 701999 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gangwu@chromium.org changed reviewers: + skau@chromium.org, skym@chromium.org
Description was changed from ========== Enable printers sync_integration_tests on chromeos revert https://codereview.chromium.org/2758643002 as well. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by add flush. Also revert https://codereview.chromium.org/2758643002 as well. BUG=689662, 701999 ==========
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Awesome. Thanks for figuring this out! I reassigned 701999 to you.
I don't think the CL comment > Also revert https://codereview.chromium.org/2758643002 as well. Is exactly appropriate. You didn't revert the CHECK -> DCHECK part of it, because you're not reverting, which is a very specific action. It's fine to link to the previous CL, but would be better to explain why the approach taken in that CL didn't work correctly, and why the approach in this CL will work. https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:115: // Flush here since PrintersManagerFactory::BuildServiceInstanceFor() uses Can you expand on this comment. Why are we flushing? You're explaining why you've chosen this particular BlockingPool, but not why flushing is important/required. https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:117: content::BrowserThread::GetBlockingPool()->FlushForTesting(); I don't like how this is being done. Do we really need to flush/run until idle before every PrintersManager access? Can this just be done once at the start of each test case? Or even better, inside SetupSync()? https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:123: chromeos::PrintersManager* GetPrinterStore(int index) { This function looks really similar to the above one. Maybe some simple refactoring is in order? https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_printers_sync_test.cc (right): https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_printers_sync_test.cc:35: ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait()); Why is this being added back? I know this isn't your fault, but we should not be using UpdatedProgressMarkerChecker here. Part of its checks are directory specific. It seems like we're trying to wait for sync to have potentially sent something something between devices. And then we verify no printers data was sent. Some tests send a bookmark, wait for it, and then verify their data type did not go. I like this approach more, because if UpdatedProgressMarkerChecker(GetSyncService(0)).Wait() doesn't actually wait for long enough/correctly, then this test silently fails. If you feel fixing this is out of scope, add a todo and add a bunch of explanation.
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by add flush. Also revert https://codereview.chromium.org/2758643002 as well. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by add flush. Remove unnecessary changes in https://codereview.chromium.org/2758643002 since that did not really fix. BUG=689662, 701999 ==========
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by add flush. Remove unnecessary changes in https://codereview.chromium.org/2758643002 since that did not really fix. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by add flush for background tasks. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix. BUG=689662, 701999 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
updated https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:115: // Flush here since PrintersManagerFactory::BuildServiceInstanceFor() uses On 2017/04/06 16:48:44, skym wrote: > Can you expand on this comment. Why are we flushing? You're explaining why > you've chosen this particular BlockingPool, but not why flushing is > important/required. Done. https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:117: content::BrowserThread::GetBlockingPool()->FlushForTesting(); On 2017/04/06 16:48:44, skym wrote: > I don't like how this is being done. Do we really need to flush/run until idle > before every PrintersManager access? Can this just be done once at the start of > each test case? Or even better, inside SetupSync()? Done. https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:123: chromeos::PrintersManager* GetPrinterStore(int index) { On 2017/04/06 16:48:44, skym wrote: > This function looks really similar to the above one. Maybe some simple > refactoring is in order? Done. https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_printers_sync_test.cc (right): https://codereview.chromium.org/2799103002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_printers_sync_test.cc:35: ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait()); On 2017/04/06 16:48:44, skym wrote: > Why is this being added back? I know this isn't your fault, but we should not be > using UpdatedProgressMarkerChecker here. Part of its checks are directory > specific. > > It seems like we're trying to wait for sync to have potentially sent something > something between devices. And then we verify no printers data was sent. Some > tests send a bookmark, wait for it, and then verify their data type did not go. > I like this approach more, because if > UpdatedProgressMarkerChecker(GetSyncService(0)).Wait() doesn't actually wait for > long enough/correctly, then this test silently fails. > > If you feel fixing this is out of scope, add a todo and add a bunch of > explanation. Done.
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by add flush for background tasks. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ==========
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ==========
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ==========
update more comments
Thanks for the updates, and sorry to be a bit of a stickler, but I really think it's important that we leave great comments on this code as we're kind of working around leaky interfaces and a whole bunch of context is required to understand why this approach is okay/works. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:72: // content::BrowserThread::GetBlockingPool() to schedule tasks, tests need to So what if the PrintersManagerFactory schedules tasks to BrowserThread::GetBlockingPool()? Why does this integration test care about waiting for them? https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:73: // Wait for tasks running on content::BrowserThread::GetBlockingPool() to be Wait shouldn't be capitalized https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:76: // Wait for UI thread task completion. Can you elaborate what's going on here? What's being posted to the UI thread? Why do we need to wait for it? https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:122: chromeos::PrintersManagerFactory::GetForBrowserContext( It seems like there's more stuff that can be moved into the shared function. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:125: WaitForTaskCompletion(); Does this really need to be called on every GetVerifierPrinterStore()?
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ==========
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
updated. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:72: // content::BrowserThread::GetBlockingPool() to schedule tasks, tests need to On 2017/04/06 21:03:03, skym wrote: > So what if the PrintersManagerFactory schedules tasks to > BrowserThread::GetBlockingPool()? Why does this integration test care about > waiting for them? Done. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:73: // Wait for tasks running on content::BrowserThread::GetBlockingPool() to be On 2017/04/06 21:03:03, skym wrote: > Wait shouldn't be capitalized Done. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:76: // Wait for UI thread task completion. On 2017/04/06 21:03:03, skym wrote: > Can you elaborate what's going on here? What's being posted to the UI thread? > Why do we need to wait for it? Done. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:122: chromeos::PrintersManagerFactory::GetForBrowserContext( On 2017/04/06 21:03:03, skym wrote: > It seems like there's more stuff that can be moved into the shared function. Done. https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:125: WaitForTaskCompletion(); On 2017/04/06 21:03:03, skym wrote: > Does this really need to be called on every GetVerifierPrinterStore()? no, just first one. but I think there is no harm to wait more times.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/printers_helper.cc:125: WaitForTaskCompletion(); On 2017/04/06 21:50:56, Gang Wu wrote: > On 2017/04/06 21:03:03, skym wrote: > > Does this really need to be called on every GetVerifierPrinterStore()? > > no, just first one. but I think there is no harm to wait more times. I don't think I agree there is no harm done, but I've probably bikeshedded long enough on this code review. https://codereview.chromium.org/2799103002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/printers_helper.cc:74: // Must wait for ModelTypeStore initialization. Can you add that we can probably remove all of this once crbug.com/709094 is fixed?
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2799103002/#ps120001 (title: "add todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491522289422700,
"parent_rev": "932a1737ea20898dcab11ee7433dfd969359473c", "commit_rev":
"864d4cde3b40817cf6831a4a2e6711a4f94413b9"}
Message was sent while issue was closed.
Description was changed from ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 ========== to ========== Enable printers sync_integration_tests on chromeos Fix printer flaky bugs by adding another wait. The bug is flaky because tests only wait for UI tasks to be completed, but since PrintersManagerFactory uses content::BrowserThread::GetBlockingPool() to schedule ModelTypeStore initialization, and then reply result to UI thread, so we need to wait on content::BrowserThread::GetBlockingPool() as well. Remove unnecessary changes from https://codereview.chromium.org/2758643002 since that did not really fix this waiting issue. BUG=689662, 701999 Review-Url: https://codereview.chromium.org/2799103002 Cr-Commit-Position: refs/heads/master@{#462716} Committed: https://chromium.googlesource.com/chromium/src/+/864d4cde3b40817cf6831a4a2e67... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/864d4cde3b40817cf6831a4a2e67...
Message was sent while issue was closed.
https://codereview.chromium.org/2799103002/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/printers_helper.cc (right): https://codereview.chromium.org/2799103002/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/printers_helper.cc:74: // Must wait for ModelTypeStore initialization. On 2017/04/06 22:52:11, skym wrote: > Can you add that we can probably remove all of this once crbug.com/709094 is > fixed? Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
