Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2130083002:
Re-enable DataReductionPromoInfoBar tests and fix flakiness (Closed)
DescriptionRe-enable DataReductionPromoInfoBar tests and fix flakiness
StrictMode violation was from Calendar.getInstance() reading
the /etc/timezone file. Whitelist the StrictMode violation
to be fixed.
Tests were flaky because the UI thread would not always launch
the InfoBar before calling addInfoBarAnimationFinished(). Run
the UI thread blocking so that the InfoBar has always been
launched.
BUG=625038, 577185, 627038
Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23
Committed: https://crrev.com/ca4f3ba8a3522dbaf066b18465cf4cc322cdfb24
Cr-Original-Commit-Position: refs/heads/master@{#404506}
Cr-Commit-Position: refs/heads/master@{#405545}
Patch Set 1 #Patch Set 2 : strict mode fix #Patch Set 3 : remove null check #Patch Set 4 : include other file reads #
Total comments: 5
Patch Set 5 : test changes #Patch Set 6 : add field trial #Patch Set 7 : move calendar instantiation #
Messages
Total messages: 40 (22 generated)
Description was changed from ========== Re-enable tests and fix flakiness BUG=625038 ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 ==========
megjablon@chromium.org changed reviewers: + dfalcantara@chromium.org
PTAL, thanks! I found out using the calendar on the UI thread was causing the StrictMode issue. See https://bugs.chromium.org/p/chromium/issues/detail?id=608223.
lgtm, +wnwen as StrictMode FYI
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2130083002/#ps60001 (title: "remove null check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2135193002/ by picksi@chromium.org. The reason for reverting is: A number of related tests have started failing (see https://bugs.chromium.org/p/chromium/issues/detail?id=627038).
Message was sent while issue was closed.
Patchset #4 (id:80001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ==========
Heads up: when using the same issue, you need to manually "Edit issue" and mark it as not closed.
https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:91: Moving all of this code into this try block won't address whatever flakiness you're encountering. Keep it outside unless you have a strong reason to move it in. https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java (right): https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:204: getInstrumentation().waitForIdleSync(); Judging by how you only put this line here and not the other location that does the exact same thing below, it looks like you're trying to put a band-aid on the problem. Do it in both or do it in neither by refactoring these two tests so they both use the same codepath for the first half (they diverge when clicking a button AFAICT).
Description was changed from ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185, 627038 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ==========
https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:91: On 2016/07/12 23:28:07, dfalcantara wrote: > Moving all of this code into this try block won't address whatever flakiness > you're encountering. Keep it outside unless you have a strong reason to move it > in. The flakiness is addressed in the Infobar tests. This is to fix the strict mode issue. I'm still investigating with Peter how to make strict mode trigger locally so that I can test and target this better. According to his comment on the bug, the line below where I do Date.ValueOf makes a disk read too. From a little investigation I think BitmapFactory.decodeResource might too so that's why I've included it all in this block. https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java (right): https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:204: getInstrumentation().waitForIdleSync(); On 2016/07/12 23:28:07, dfalcantara wrote: > Judging by how you only put this line here and not the other location that does > the exact same thing below, it looks like you're trying to put a band-aid on the > problem. Do it in both or do it in neither by refactoring these two tests so > they both use the same codepath for the first half (they diverge when clicking a > button AFAICT). Sorry I've been jumping back and forth between the broken and possible fix and looks like some code got scrambled. I meant for this to be in both locations. Using waitForIdleSync was still causing flakiness so I'm using a CriteriaHelper instead.
Eh third try's the charm, I suppose. lgtm from a test standpoint. https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java (right): https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:91: On 2016/07/13 01:03:28, megjablon wrote: > On 2016/07/12 23:28:07, dfalcantara wrote: > > Moving all of this code into this try block won't address whatever flakiness > > you're encountering. Keep it outside unless you have a strong reason to move > it > > in. > > The flakiness is addressed in the Infobar tests. This is to fix the strict mode > issue. I'm still investigating with Peter how to make strict mode trigger > locally so that I can test and target this better. According to his comment on > the bug, the line below where I do Date.ValueOf makes a disk read too. From a > little investigation I think BitmapFactory.decodeResource might too so that's > why I've included it all in this block. Acknowledged.
On 2016/07/13 01:09:04, dfalcantara wrote: > Eh third try's the charm, I suppose. lgtm from a test standpoint. > > https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... > File > chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java > (right): > > https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:91: > > On 2016/07/13 01:03:28, megjablon wrote: > > On 2016/07/12 23:28:07, dfalcantara wrote: > > > Moving all of this code into this try block won't address whatever flakiness > > > you're encountering. Keep it outside unless you have a strong reason to > move > > it > > > in. > > > > The flakiness is addressed in the Infobar tests. This is to fix the strict > mode > > issue. I'm still investigating with Peter how to make strict mode trigger > > locally so that I can test and target this better. According to his comment on > > the bug, the line below where I do Date.ValueOf makes a disk read too. From a > > little investigation I think BitmapFactory.decodeResource might too so that's > > why I've included it all in this block. > > Acknowledged. I'm going to make sure I've tried all my options to get the test working locally with wnwen@ tomorrow. If that still doesn't work, I'll land this and keep a close eye on the bots and revert right away if we still have an issue.
On 2016/07/13 01:28:28, megjablon wrote: > On 2016/07/13 01:09:04, dfalcantara wrote: > > Eh third try's the charm, I suppose. lgtm from a test standpoint. > > > > > https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java > > (right): > > > > > https://chromiumcodereview.appspot.com/2130083002/diff/100001/chrome/android/... > > > chrome/android/java/src/org/chromium/chrome/browser/infobar/DataReductionPromoInfoBar.java:91: > > > > On 2016/07/13 01:03:28, megjablon wrote: > > > On 2016/07/12 23:28:07, dfalcantara wrote: > > > > Moving all of this code into this try block won't address whatever > flakiness > > > > you're encountering. Keep it outside unless you have a strong reason to > > move > > > it > > > > in. > > > > > > The flakiness is addressed in the Infobar tests. This is to fix the strict > > mode > > > issue. I'm still investigating with Peter how to make strict mode trigger > > > locally so that I can test and target this better. According to his comment > on > > > the bug, the line below where I do Date.ValueOf makes a disk read too. From > a > > > little investigation I think BitmapFactory.decodeResource might too so > that's > > > why I've included it all in this block. > > > > Acknowledged. > > I'm going to make sure I've tried all my options to get the test working locally > with wnwen@ tomorrow. If that still doesn't work, I'll land this and keep a > close eye on the bots and revert right away if we still have an issue. Can you take one last look? wnwen@ caught a Release test error since it doesn't get the field trial, so I added a command line flag to fix that.
lgtm
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2130083002/#ps160001 (title: "git cl try")
The CQ bit was unchecked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
The CQ bit was unchecked by megjablon@chromium.org
The CQ bit was checked by megjablon@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 megjablon@chromium.org
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185, 627038 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185, 627038 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185, 627038 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Cr-Commit-Position: refs/heads/master@{#404506} ========== to ========== Re-enable DataReductionPromoInfoBar tests and fix flakiness StrictMode violation was from Calendar.getInstance() reading the /etc/timezone file. Whitelist the StrictMode violation to be fixed. Tests were flaky because the UI thread would not always launch the InfoBar before calling addInfoBarAnimationFinished(). Run the UI thread blocking so that the InfoBar has always been launched. BUG=625038, 577185, 627038 Committed: https://crrev.com/8fcd3f03a3a531f12bb291dc8665834d85511c23 Committed: https://crrev.com/ca4f3ba8a3522dbaf066b18465cf4cc322cdfb24 Cr-Original-Commit-Position: refs/heads/master@{#404506} Cr-Commit-Position: refs/heads/master@{#405545} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ca4f3ba8a3522dbaf066b18465cf4cc322cdfb24 Cr-Commit-Position: refs/heads/master@{#405545}
Message was sent while issue was closed.
Patchset #8 (id:180001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
