Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2027503002:
⛎ Fix downstream tab state migration test. (Closed)
DescriptionFix downstream tab state migration test.
Since the SharedPreferences is kept in memory, it is not cleared and
reloaded even when the underlying file is deleted. Using an
AdvancedMockContext should ensure that we use a fresh in-memory
SharedPreference every time, this may speed up the test too.
BUG=615877
Committed: https://crrev.com/eed074ef476f639f864dfb0511eb56b7693abc6a
Cr-Commit-Position: refs/heads/master@{#396872}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix migration #Patch Set 3 : Try out mock context. #Patch Set 4 : Clean up CL. #Messages
Total messages: 19 (9 generated)
wnwen@chromium.org changed reviewers: + agrieve@chromium.org
⛳
https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java (right): https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java:66: mAppContext = getInstrumentation().getTargetContext().getApplicationContext(); Looks like the bots are not yet happy. Maybe you need to wrap this in a AdvancedMockContext so that an InMemorySharedPreferences is used?
https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java (right): https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java:66: mAppContext = getInstrumentation().getTargetContext().getApplicationContext(); On 2016/05/31 00:43:43, agrieve wrote: > Looks like the bots are not yet happy. Maybe you need to wrap this in a > AdvancedMockContext so that an InMemorySharedPreferences is used? Hmm... that should work, feels a little heavy for just one specific preference though. What do you think about clearing that preference alone like in the next patch?
lgtm https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java (right): https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java:66: mAppContext = getInstrumentation().getTargetContext().getApplicationContext(); On 2016/05/31 13:41:35, Peter Wen wrote: > On 2016/05/31 00:43:43, agrieve wrote: > > Looks like the bots are not yet happy. Maybe you need to wrap this in a > > AdvancedMockContext so that an InMemorySharedPreferences is used? > > Hmm... that should work, feels a little heavy for just one specific preference > though. What do you think about clearing that preference alone like in the next > patch? I'm of the opinion that if a test can run without accessing disk, then it should (aka, in-memory would be better). That said, if the bot is happy, lgtm!
wnwen@chromium.org changed reviewers: + dfalcantara@chromium.org
+dfalcantara@ for OWNERS RestoreMigrateTest.java https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java (right): https://codereview.chromium.org/2027503002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/RestoreMigrateTest.java:66: mAppContext = getInstrumentation().getTargetContext().getApplicationContext(); On 2016/05/31 13:48:46, agrieve wrote: > On 2016/05/31 13:41:35, Peter Wen wrote: > > On 2016/05/31 00:43:43, agrieve wrote: > > > Looks like the bots are not yet happy. Maybe you need to wrap this in a > > > AdvancedMockContext so that an InMemorySharedPreferences is used? > > > > Hmm... that should work, feels a little heavy for just one specific preference > > though. What do you think about clearing that preference alone like in the > next > > patch? > > I'm of the opinion that if a test can run without accessing disk, then it should > (aka, in-memory would be better). That said, if the bot is happy, lgtm! Good point! I've changed it to AdvancedMockContext and tests still seem fine. Thanks for the reminder. :)
Description was changed from ========== Tentative fix for downstream migration test. It is possible that since the SharedPreferences is kept in memory, it is not cleared/reloaded even though the underlying file is deleted. In that case, setUp reloading should fix it. BUG=615877 ========== to ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 ==========
Description was changed from ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 ========== to ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 ==========
Isn't this thing an upstream test? lgtm
On 2016/05/31 17:08:30, dfalcantara wrote: > Isn't this thing an upstream test? > > lgtm Only fails with regularity on downstream L. Upstream K seems to be mostly okay. :/ Thanks for the review!
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2027503002/#ps60001 (title: "Clean up CL.")
The CQ bit was checked by wnwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2027503002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2027503002/60001
Message was sent while issue was closed.
Description was changed from ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 ========== to ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 ========== to ========== Fix downstream tab state migration test. Since the SharedPreferences is kept in memory, it is not cleared and reloaded even when the underlying file is deleted. Using an AdvancedMockContext should ensure that we use a fresh in-memory SharedPreference every time, this may speed up the test too. BUG=615877 Committed: https://crrev.com/eed074ef476f639f864dfb0511eb56b7693abc6a Cr-Commit-Position: refs/heads/master@{#396872} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/eed074ef476f639f864dfb0511eb56b7693abc6a Cr-Commit-Position: refs/heads/master@{#396872} |
