Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871753003:
[Sync] Speculative changes to address flaky AndroidSyncSettings tests (Closed)
Description[Sync] Speculative changes to address flaky AndroidSyncSettings tests
testAccountInitialization fails at 172 with
assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls);
testIsSyncableOnSigninAndNotOnSignout fails at 372 with
assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1);
I didn't find codepath that would explain both failures so I'm speculatively
fixing threading issues that I spotted and enabling one of disabled tests, the
one easier to reason about.
Considering last time it took tests couple of days to appear on flakiness
console I'm going to wait about a week, if test doesn't fail I'll enable
the other one.
Here are the issues I'm addressing:
- counters in CountingMockSyncContentResolverDelegate are a simple ints, while
they are incremented and accessed from different threads. I'm changing them to
AtomicIntegers.
- in AndroidSyncSettings.updateSyncability callback receiving results of
getGoogleAccounts is executed on UI thread, not Instrumentation thread where
all the test code executes. It accesses mAccount. I wrapped this code block in
synchronized.
BUG=717960
R=skym@chromium.org
Review-Url: https://codereview.chromium.org/2871753003
Cr-Commit-Position: refs/heads/master@{#470222}
Committed: https://chromium.googlesource.com/chromium/src/+/f1c5a372002a6a2367c19a3d6100fc69ef5a7ccc
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use assertEquals instead of assertTrue #
Messages
Total messages: 17 (11 generated)
The CQ bit was checked by pavely@chromium.org to run a CQ dry run
Description was changed from ========== [Sync] Speculative chagnes to address flaky AndroidSyncSettings tests testAccountInitialization fails at 172 with assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls); testIsSyncableOnSigninAndNotOnSignout fails at 372 with assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); I didn't find codepath that would explain both failures so I'm speculatively fixing threading issues that I spotted and enabling one of disabled tests, the one easier to reason about. Here are the issues I'm addressing: - counters in CountingMockSyncContentResolverDelegate are a simple ints, while they are incremented and accessed from different threads. I'm changing them to AtomicIntegers. - in AndroidSyncSettings.updateSyncability callback receiving results of getGoogleAccounts is executed on UI thread, not Instrumentation thread where all the test code executes. It accesses mAccount. I wrapped this code block in synchronized. BUG=717960 R=skym@chromium.org ========== to ========== [Sync] Speculative changes to address flaky AndroidSyncSettings tests testAccountInitialization fails at 172 with assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls); testIsSyncableOnSigninAndNotOnSignout fails at 372 with assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); I didn't find codepath that would explain both failures so I'm speculatively fixing threading issues that I spotted and enabling one of disabled tests, the one easier to reason about. Here are the issues I'm addressing: - counters in CountingMockSyncContentResolverDelegate are a simple ints, while they are incremented and accessed from different threads. I'm changing them to AtomicIntegers. - in AndroidSyncSettings.updateSyncability callback receiving results of getGoogleAccounts is executed on UI thread, not Instrumentation thread where all the test code executes. It accesses mAccount. I wrapped this code block in synchronized. BUG=717960 R=skym@chromium.org ==========
Description was changed from ========== [Sync] Speculative changes to address flaky AndroidSyncSettings tests testAccountInitialization fails at 172 with assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls); testIsSyncableOnSigninAndNotOnSignout fails at 372 with assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); I didn't find codepath that would explain both failures so I'm speculatively fixing threading issues that I spotted and enabling one of disabled tests, the one easier to reason about. Here are the issues I'm addressing: - counters in CountingMockSyncContentResolverDelegate are a simple ints, while they are incremented and accessed from different threads. I'm changing them to AtomicIntegers. - in AndroidSyncSettings.updateSyncability callback receiving results of getGoogleAccounts is executed on UI thread, not Instrumentation thread where all the test code executes. It accesses mAccount. I wrapped this code block in synchronized. BUG=717960 R=skym@chromium.org ========== to ========== [Sync] Speculative changes to address flaky AndroidSyncSettings tests testAccountInitialization fails at 172 with assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls); testIsSyncableOnSigninAndNotOnSignout fails at 372 with assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); I didn't find codepath that would explain both failures so I'm speculatively fixing threading issues that I spotted and enabling one of disabled tests, the one easier to reason about. Considering last time it took tests couple of days to appear on flakiness console I'm going to wait about a week, if test doesn't fail I'll enable the other one. Here are the issues I'm addressing: - counters in CountingMockSyncContentResolverDelegate are a simple ints, while they are incremented and accessed from different threads. I'm changing them to AtomicIntegers. - in AndroidSyncSettings.updateSyncability callback receiving results of getGoogleAccounts is executed on UI thread, not Instrumentation thread where all the test code executes. It accesses mAccount. I wrapped this code block in synchronized. BUG=717960 R=skym@chromium.org ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... File components/sync/android/java/src/org/chromium/components/sync/AndroidSyncSettings.java (right): https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/java/src/org/chromium/components/sync/AndroidSyncSettings.java:254: synchronized (mLock) { What is this lock protecting? mSyncContentResolverDelegate? Not all access (103, 234, 245) to it is guarded by mLock. https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... File components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java (right): https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java:31: private static class CountingMockSyncContentResolverDelegate @ThreadSafe? https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java:111: assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); Why not assertEquals(1, ...)?
The CQ bit was checked by pavely@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...
https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... File components/sync/android/java/src/org/chromium/components/sync/AndroidSyncSettings.java (right): https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/java/src/org/chromium/components/sync/AndroidSyncSettings.java:254: synchronized (mLock) { On 2017/05/08 23:26:28, skym wrote: > What is this lock protecting? mSyncContentResolverDelegate? Not all access (103, > 234, 245) to it is guarded by mLock. mAccount. Delegate is final set in ctor while mAccount is updated under lock in updateAccount. https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... File components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java (right): https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java:31: private static class CountingMockSyncContentResolverDelegate On 2017/05/08 23:26:28, skym wrote: > @ThreadSafe? MockSyncContentResolverDelegate is not annotated with @ThreadSafe. What does @ThreadSafe affect anyway? https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java:111: assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); On 2017/05/08 23:26:28, skym wrote: > Why not assertEquals(1, ...)? Done.
The CQ bit was unchecked by pavely@chromium.org
The CQ bit was checked by pavely@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2871753003/#ps20001 (title: "Use assertEquals instead of assertTrue")
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": 20001, "attempt_start_ts": 1494289655135460,
"parent_rev": "8762c915f746100704a494b9dd09075030e77906", "commit_rev":
"f1c5a372002a6a2367c19a3d6100fc69ef5a7ccc"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Speculative changes to address flaky AndroidSyncSettings tests testAccountInitialization fails at 172 with assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls); testIsSyncableOnSigninAndNotOnSignout fails at 372 with assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); I didn't find codepath that would explain both failures so I'm speculatively fixing threading issues that I spotted and enabling one of disabled tests, the one easier to reason about. Considering last time it took tests couple of days to appear on flakiness console I'm going to wait about a week, if test doesn't fail I'll enable the other one. Here are the issues I'm addressing: - counters in CountingMockSyncContentResolverDelegate are a simple ints, while they are incremented and accessed from different threads. I'm changing them to AtomicIntegers. - in AndroidSyncSettings.updateSyncability callback receiving results of getGoogleAccounts is executed on UI thread, not Instrumentation thread where all the test code executes. It accesses mAccount. I wrapped this code block in synchronized. BUG=717960 R=skym@chromium.org ========== to ========== [Sync] Speculative changes to address flaky AndroidSyncSettings tests testAccountInitialization fails at 172 with assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls); testIsSyncableOnSigninAndNotOnSignout fails at 372 with assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); I didn't find codepath that would explain both failures so I'm speculatively fixing threading issues that I spotted and enabling one of disabled tests, the one easier to reason about. Considering last time it took tests couple of days to appear on flakiness console I'm going to wait about a week, if test doesn't fail I'll enable the other one. Here are the issues I'm addressing: - counters in CountingMockSyncContentResolverDelegate are a simple ints, while they are incremented and accessed from different threads. I'm changing them to AtomicIntegers. - in AndroidSyncSettings.updateSyncability callback receiving results of getGoogleAccounts is executed on UI thread, not Instrumentation thread where all the test code executes. It accesses mAccount. I wrapped this code block in synchronized. BUG=717960 R=skym@chromium.org Review-Url: https://codereview.chromium.org/2871753003 Cr-Commit-Position: refs/heads/master@{#470222} Committed: https://chromium.googlesource.com/chromium/src/+/f1c5a372002a6a2367c19a3d6100... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f1c5a372002a6a2367c19a3d6100...
Message was sent while issue was closed.
https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... File components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java (right): https://codereview.chromium.org/2871753003/diff/1/components/sync/android/jav... components/sync/android/javatests/src/org/chromium/components/sync/AndroidSyncSettingsTest.java:31: private static class CountingMockSyncContentResolverDelegate On 2017/05/09 00:27:28, pavely wrote: > On 2017/05/08 23:26:28, skym wrote: > > @ThreadSafe? > > MockSyncContentResolverDelegate is not annotated with @ThreadSafe. What does > @ThreadSafe affect anyway? It doesn't do anything, just marks intention. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
