Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1834783002:
Eliminate race condition in CustomTabActivityTest (Closed)
DescriptionEliminate race condition in CustomTabActivityTest
Eliminates a race condition in CustomTabActivityTest#testPrecreatedRenderer
by waiting for the new custom tab to open before asserting
BUG=597741
Committed: https://crrev.com/4c94be22307df94b2da87ce920880ada92c28efe
Cr-Commit-Position: refs/heads/master@{#383385}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed low-end restriction #Patch Set 3 : Rebased on master #Messages
Total messages: 22 (6 generated)
kraush@amazon.com changed reviewers: + pasko@chromium.org
Hi pasko, Can you take a look at this test de-flake? Thanks! :) Holger
https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (left): https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:721: assertEquals(mTestPage, tab.getUrl()); was this assertion failing to match the URL? I could not find any flake on build.chromium.org, do you have any ideas why it is more flaky in your environment? https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:725: return currentTab != previousTab && mTestPage.equals(currentTab.getUrl()); can "currentTab != previousTab" be replaced with "currentTab != null"?
On 2016/03/25 15:31:09, pasko wrote: > https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java > (left): > > https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:721: > assertEquals(mTestPage, tab.getUrl()); > was this assertion failing to match the URL? > > I could not find any flake on http://build.chromium.org, do you have any ideas why it > is more flaky in your environment? > > https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java > (right): > > https://codereview.chromium.org/1834783002/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:725: > return currentTab != previousTab && mTestPage.equals(currentTab.getUrl()); > can "currentTab != previousTab" be replaced with "currentTab != null"? The test failed either on the URL or on the canGoBack - I think it depends on which test was running previously. A null-check will unfortunately not work. I just double-checked, and previousTab is not Null (which would also explain why it was failing an assert rather than dying with a NullPointer before - there was a tab, it was just the incorrect tab) As for why this is more flaky in our environment: We've exlusively seen this failure on devices that are lower budget than a Nexus 4. I assume the async startup on Nexus 4s (and most other devices) is just fast enough to never run into this issue. Presumably this is also why this test might've been disabled on Svelte devices in the past, but I can't say for certain (no access to bug 530225) In case you can access the bug and think it is the same reason please let me know and I'd be happy to remove that @Restriction tag from the test :)
On 2016/03/25 16:30:35, kraush wrote: > The test failed either on the URL or on the canGoBack - I think it depends on > which test was running previously. > A null-check will unfortunately not work. > I just double-checked, and previousTab is not Null (which would also explain why > it was failing an assert rather than dying with a NullPointer before - there was > a tab, it was just the incorrect tab) > > As for why this is more flaky in our environment: We've exlusively seen this > failure on devices that are lower budget than a Nexus 4. I assume the async > startup on Nexus 4s (and most other devices) is just fast enough to never run > into this issue. > Presumably this is also why this test might've been disabled on Svelte devices > in the past, but I can't say for certain (no access to bug 530225) > In case you can access the bug and think it is the same reason please let me > know and I'd be happy to remove that @Restriction tag from the test :) Thank you for the context! The crbug.com/530225 was marked internal because, well, that's what we do with all crashes. The stack trace is essentially the same as you point out in crbug.com/597741, and it was indeed flaky on a low end device. You change looks good, please also remove the @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE) and I will l-g-t-m it. Thank you for pointing this out!
On 2016/03/25 17:28:44, pasko wrote: > On 2016/03/25 16:30:35, kraush wrote: > > The test failed either on the URL or on the canGoBack - I think it depends on > > which test was running previously. > > A null-check will unfortunately not work. > > I just double-checked, and previousTab is not Null (which would also explain > why > > it was failing an assert rather than dying with a NullPointer before - there > was > > a tab, it was just the incorrect tab) > > > > As for why this is more flaky in our environment: We've exlusively seen this > > failure on devices that are lower budget than a Nexus 4. I assume the async > > startup on Nexus 4s (and most other devices) is just fast enough to never run > > into this issue. > > Presumably this is also why this test might've been disabled on Svelte devices > > in the past, but I can't say for certain (no access to bug 530225) > > In case you can access the bug and think it is the same reason please let me > > know and I'd be happy to remove that @Restriction tag from the test :) > > Thank you for the context! The crbug.com/530225 was marked internal because, > well, that's what we do with all crashes. The stack trace is essentially the > same as you point out in crbug.com/597741, and it was indeed flaky on a low end > device. > > You change looks good, please also remove the > @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE) and I will l-g-t-m it. > > Thank you for pointing this out! Will do, thanks! :)
Removed the low-end restriction as suggested. Please take another look :)
lgtm, thanks
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834783002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834783002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
On 2016/03/25 17:55:59, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) Sorry, my workspace wasn't completely up-to-date and it seems I didn't have the rename of CriteriaHelper methods yet! I'll rebase, rename and rebuild. Will post the next diff as soon as that is done. Sorry for that! :(
Rebased on top of master and rebuilt. The only change was renaming the called method as per https://chromium.googlesource.com/chromium/src/+/445d015b38c93a4afea0cfbf1613... Rebuilt locally and it worked. I'll submit this revision.
The CQ bit was checked by kraush@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/1834783002/#ps40001 (title: "Rebased on master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834783002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834783002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Eliminate race condition in CustomTabActivityTest Eliminates a race condition in CustomTabActivityTest#testPrecreatedRenderer by waiting for the new custom tab to open before asserting BUG=597741 ========== to ========== Eliminate race condition in CustomTabActivityTest Eliminates a race condition in CustomTabActivityTest#testPrecreatedRenderer by waiting for the new custom tab to open before asserting BUG=597741 Committed: https://crrev.com/4c94be22307df94b2da87ce920880ada92c28efe Cr-Commit-Position: refs/heads/master@{#383385} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4c94be22307df94b2da87ce920880ada92c28efe Cr-Commit-Position: refs/heads/master@{#383385}
Message was sent while issue was closed.
Tip of the day: there is no need to rebase if the patch is like 1 day old. CQ will quickly notify you if the patch does not auto-merge, or testing fails. I think it is even preferable *not* to rebase in order for reviewers to avoid worrying about what was actually committed.
Message was sent while issue was closed.
On 2016/03/29 13:37:31, pasko wrote: > Tip of the day: there is no need to rebase if the patch is like 1 day old. CQ > will quickly notify you if the patch does not auto-merge, or testing fails. I > think it is even preferable *not* to rebase in order for reviewers to avoid > worrying about what was actually committed. Gotcha, thanks for the pointer! :) |
