Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088093003:
android: Add RetryOnFailure test annotation (Closed)
Descriptionandroid: Add RetryOnFailure test annotation
Note this annotation does not do anything yet without script changes.
BUG=619055
Committed: https://crrev.com/1dd26c8c1c6ffc149f46483b56d20ff71cd35732
Cr-Commit-Position: refs/heads/master@{#402030}
Patch Set 1 #
Total comments: 10
Patch Set 2 : remove bugid #Patch Set 3 : remove tolerance #
Messages
Total messages: 34 (5 generated)
boliu@chromium.org changed reviewers: + jbudorick@chromium.org
ptal stole tolerance from the sdk one, seems reasonable thing to have I came up with bugId and making it required so wdyt? any of those easy to parse out from the script?
https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java (right): https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: * Long term, this should be merged with @FlakyTest. But @FlakyTest means I'm not convinced that we necessarily want to do this. https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:30: int bugId(); We shouldn't have message and bugId; they're essentially redundant. Given that we've used message elsewhere, I'd strongly prefer we stay consistent with that. https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:36: int tolerance() default 3; nit: tolerance isn't a great name for this. I'd prefer retries w/ default 2.
https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java (right): https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: * Long term, this should be merged with @FlakyTest. But @FlakyTest means On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > I'm not convinced that we necessarily want to do this. why not? once FlakyTest doesn't have other meanings?
https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java (right): https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: * Long term, this should be merged with @FlakyTest. But @FlakyTest means On 2016/06/23 13:53:53, boliu wrote: > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > I'm not convinced that we necessarily want to do this. > > why not? once FlakyTest doesn't have other meanings? My hesitation is based on the functional conflict with android.test.FlakyTest / android.support.test.FlakyTest. Our current chromium version of FlakyTest is functionally identical (we just added a message field). RetryOnFailure will behave differently.
https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java (right): https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: * Long term, this should be merged with @FlakyTest. But @FlakyTest means On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > On 2016/06/23 13:53:53, boliu wrote: > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > I'm not convinced that we necessarily want to do this. > > > > why not? once FlakyTest doesn't have other meanings? > > My hesitation is based on the functional conflict with android.test.FlakyTest / > android.support.test.FlakyTest. Our current chromium version of FlakyTest is > functionally identical (we just added a message field). RetryOnFailure will > behave differently. Hmm? I mean after FlakyTest no longer means what it means downstream.. I assume we want to unify that with this eventually?
https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java (right): https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: * Long term, this should be merged with @FlakyTest. But @FlakyTest means On 2016/06/23 14:02:59, boliu wrote: > On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > > On 2016/06/23 13:53:53, boliu wrote: > > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > > I'm not convinced that we necessarily want to do this. > > > > > > why not? once FlakyTest doesn't have other meanings? > > > > My hesitation is based on the functional conflict with android.test.FlakyTest > / > > android.support.test.FlakyTest. Our current chromium version of FlakyTest is > > functionally identical (we just added a message field). RetryOnFailure will > > behave differently. > > Hmm? I mean after FlakyTest no longer means what it means downstream.. I assume > we want to unify that with this eventually? @FlakyTest will still have meaning in an android context, and adding functionality to that would feel like a strange override. I guess I'm assuming that by "merged with @FlakyTest," you meant that this functionality should be moved into our chromium version of @FlakyTest. If that's a misreading of what you meant, let me know.
On 2016/06/23 14:06:18, jbudorick (EMEA til June 30) wrote: > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > File > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java > (right): > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: > * Long term, this should be merged with @FlakyTest. But @FlakyTest means > On 2016/06/23 14:02:59, boliu wrote: > > On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > > > On 2016/06/23 13:53:53, boliu wrote: > > > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > > > I'm not convinced that we necessarily want to do this. > > > > > > > > why not? once FlakyTest doesn't have other meanings? > > > > > > My hesitation is based on the functional conflict with > android.test.FlakyTest > > / > > > android.support.test.FlakyTest. Our current chromium version of FlakyTest is > > > functionally identical (we just added a message field). RetryOnFailure will > > > behave differently. > > > > Hmm? I mean after FlakyTest no longer means what it means downstream.. I > assume > > we want to unify that with this eventually? > > @FlakyTest will still have meaning in an android context, and adding > functionality to that would feel like a strange override. > > I guess I'm assuming that by "merged with @FlakyTest," you meant that this > functionality should be moved into our chromium version of @FlakyTest. If that's > a misreading of what you meant, let me know. Yeah that's what I meant. But if you look at the @FlakyTest from the sdk, it's has the exact same meaning as @RetryOnFailure here, ie a "tolerance" field for retries... we've already made FlakyTest mean different things *already*, no?
On 2016/06/23 14:08:21, boliu wrote: > On 2016/06/23 14:06:18, jbudorick (EMEA til June 30) wrote: > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > File > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java > > (right): > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: > > * Long term, this should be merged with @FlakyTest. But @FlakyTest means > > On 2016/06/23 14:02:59, boliu wrote: > > > On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > > > > On 2016/06/23 13:53:53, boliu wrote: > > > > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > > > > I'm not convinced that we necessarily want to do this. > > > > > > > > > > why not? once FlakyTest doesn't have other meanings? > > > > > > > > My hesitation is based on the functional conflict with > > android.test.FlakyTest > > > / > > > > android.support.test.FlakyTest. Our current chromium version of FlakyTest > is > > > > functionally identical (we just added a message field). RetryOnFailure > will > > > > behave differently. > > > > > > Hmm? I mean after FlakyTest no longer means what it means downstream.. I > > assume > > > we want to unify that with this eventually? > > > > @FlakyTest will still have meaning in an android context, and adding > > functionality to that would feel like a strange override. > > > > I guess I'm assuming that by "merged with @FlakyTest," you meant that this > > functionality should be moved into our chromium version of @FlakyTest. If > that's > > a misreading of what you meant, let me know. > > Yeah that's what I meant. But if you look at the @FlakyTest from the sdk, it's > has the exact same meaning as @RetryOnFailure here, ie a "tolerance" field for > retries... we've already made FlakyTest mean different things *already*, no? Which FlakyTest are you referring to? I don't see that in either: - https://developer.android.com/reference/android/test/FlakyTest.html - https://developer.android.com/reference/android/support/test/filters/FlakyTes... The current difference between those and ours is the message field, which is not a functional addition.
On 2016/06/23 14:10:25, jbudorick (EMEA til June 30) wrote: > On 2016/06/23 14:08:21, boliu wrote: > > On 2016/06/23 14:06:18, jbudorick (EMEA til June 30) wrote: > > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > File > > > > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: > > > * Long term, this should be merged with @FlakyTest. But @FlakyTest means > > > On 2016/06/23 14:02:59, boliu wrote: > > > > On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > > > > > On 2016/06/23 13:53:53, boliu wrote: > > > > > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > > > > > I'm not convinced that we necessarily want to do this. > > > > > > > > > > > > why not? once FlakyTest doesn't have other meanings? > > > > > > > > > > My hesitation is based on the functional conflict with > > > android.test.FlakyTest > > > > / > > > > > android.support.test.FlakyTest. Our current chromium version of > FlakyTest > > is > > > > > functionally identical (we just added a message field). RetryOnFailure > > will > > > > > behave differently. > > > > > > > > Hmm? I mean after FlakyTest no longer means what it means downstream.. I > > > assume > > > > we want to unify that with this eventually? > > > > > > @FlakyTest will still have meaning in an android context, and adding > > > functionality to that would feel like a strange override. > > > > > > I guess I'm assuming that by "merged with @FlakyTest," you meant that this > > > functionality should be moved into our chromium version of @FlakyTest. If > > that's > > > a misreading of what you meant, let me know. > > > > Yeah that's what I meant. But if you look at the @FlakyTest from the sdk, it's > > has the exact same meaning as @RetryOnFailure here, ie a "tolerance" field for > > retries... we've already made FlakyTest mean different things *already*, no? > > Which FlakyTest are you referring to? I don't see that in either: > - https://developer.android.com/reference/android/test/FlakyTest.html That one. It says re-executes and tolerance. But looks like the actual tolerance field isn't public and so not part of the sdk? Not sure how annotations works. Fwiw I was looking at the source code third_party/android_tools/sdk/sources/android-23/android/test/FlakyTest.java > - > https://developer.android.com/reference/android/support/test/filters/FlakyTes... > > The current difference between those and ours is the message field, which is not > a functional addition.
On 2016/06/23 14:14:18, boliu wrote: > On 2016/06/23 14:10:25, jbudorick (EMEA til June 30) wrote: > > On 2016/06/23 14:08:21, boliu wrote: > > > On 2016/06/23 14:06:18, jbudorick (EMEA til June 30) wrote: > > > > > > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > > File > > > > > > > > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > > > > > > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: > > > > * Long term, this should be merged with @FlakyTest. But @FlakyTest means > > > > On 2016/06/23 14:02:59, boliu wrote: > > > > > On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > > > > > > On 2016/06/23 13:53:53, boliu wrote: > > > > > > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > > > > > > I'm not convinced that we necessarily want to do this. > > > > > > > > > > > > > > why not? once FlakyTest doesn't have other meanings? > > > > > > > > > > > > My hesitation is based on the functional conflict with > > > > android.test.FlakyTest > > > > > / > > > > > > android.support.test.FlakyTest. Our current chromium version of > > FlakyTest > > > is > > > > > > functionally identical (we just added a message field). RetryOnFailure > > > will > > > > > > behave differently. > > > > > > > > > > Hmm? I mean after FlakyTest no longer means what it means downstream.. I > > > > assume > > > > > we want to unify that with this eventually? > > > > > > > > @FlakyTest will still have meaning in an android context, and adding > > > > functionality to that would feel like a strange override. > > > > > > > > I guess I'm assuming that by "merged with @FlakyTest," you meant that this > > > > functionality should be moved into our chromium version of @FlakyTest. If > > > that's > > > > a misreading of what you meant, let me know. > > > > > > Yeah that's what I meant. But if you look at the @FlakyTest from the sdk, > it's > > > has the exact same meaning as @RetryOnFailure here, ie a "tolerance" field > for > > > retries... we've already made FlakyTest mean different things *already*, no? > > > > Which FlakyTest are you referring to? I don't see that in either: > > - https://developer.android.com/reference/android/test/FlakyTest.html > > That one. It says re-executes and tolerance. But looks like the actual tolerance > field isn't public and so not part of the sdk? Not sure how annotations works. > Fwiw I was looking at the source code > third_party/android_tools/sdk/sources/android-23/android/test/FlakyTest.java Huh, I didn't see tolerance there initially. Yeah, I guess I'm ok with this, and I see why you added bugId. Perhaps we should revisit our current message="crbug.com/XYZ" w/ bugId, too. > > > - > > > https://developer.android.com/reference/android/support/test/filters/FlakyTes... > > > > The current difference between those and ours is the message field, which is > not > > a functional addition.
https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... File base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java (right): https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:24: String message() default ""; nit: Add a comment for this. https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:30: int bugId(); On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > We shouldn't have message and bugId; they're essentially redundant. Given that > we've used message elsewhere, I'd strongly prefer we stay consistent with that. disregard https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:36: int tolerance() default 3; On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > nit: tolerance isn't a great name for this. I'd prefer retries w/ default 2. Still not crazy about the name or the default; in Android's version, tolerance=0 means the same thing as tolerance=1.
On 2016/06/23 14:18:41, jbudorick (EMEA til June 30) wrote: > On 2016/06/23 14:14:18, boliu wrote: > > On 2016/06/23 14:10:25, jbudorick (EMEA til June 30) wrote: > > > On 2016/06/23 14:08:21, boliu wrote: > > > > On 2016/06/23 14:06:18, jbudorick (EMEA til June 30) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > > > File > > > > > > > > > > > > > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2088093003/diff/1/base/test/android/javatests... > > > > > > > > > > > > > > > base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:18: > > > > > * Long term, this should be merged with @FlakyTest. But @FlakyTest means > > > > > On 2016/06/23 14:02:59, boliu wrote: > > > > > > On 2016/06/23 13:58:24, jbudorick (EMEA til June 30) wrote: > > > > > > > On 2016/06/23 13:53:53, boliu wrote: > > > > > > > > On 2016/06/23 09:22:42, jbudorick (EMEA til June 30) wrote: > > > > > > > > > I'm not convinced that we necessarily want to do this. > > > > > > > > > > > > > > > > why not? once FlakyTest doesn't have other meanings? > > > > > > > > > > > > > > My hesitation is based on the functional conflict with > > > > > android.test.FlakyTest > > > > > > / > > > > > > > android.support.test.FlakyTest. Our current chromium version of > > > FlakyTest > > > > is > > > > > > > functionally identical (we just added a message field). > RetryOnFailure > > > > will > > > > > > > behave differently. > > > > > > > > > > > > Hmm? I mean after FlakyTest no longer means what it means downstream.. > I > > > > > assume > > > > > > we want to unify that with this eventually? > > > > > > > > > > @FlakyTest will still have meaning in an android context, and adding > > > > > functionality to that would feel like a strange override. > > > > > > > > > > I guess I'm assuming that by "merged with @FlakyTest," you meant that > this > > > > > functionality should be moved into our chromium version of @FlakyTest. > If > > > > that's > > > > > a misreading of what you meant, let me know. > > > > > > > > Yeah that's what I meant. But if you look at the @FlakyTest from the sdk, > > it's > > > > has the exact same meaning as @RetryOnFailure here, ie a "tolerance" field > > for > > > > retries... we've already made FlakyTest mean different things *already*, > no? > > > > > > Which FlakyTest are you referring to? I don't see that in either: > > > - https://developer.android.com/reference/android/test/FlakyTest.html > > > > That one. It says re-executes and tolerance. But looks like the actual > tolerance > > field isn't public and so not part of the sdk? Not sure how annotations works. > > Fwiw I was looking at the source code > > third_party/android_tools/sdk/sources/android-23/android/test/FlakyTest.java > > Huh, I didn't see tolerance there initially. > > Yeah, I guess I'm ok with this, and I see why you added bugId. Perhaps we should > revisit our current message="crbug.com/XYZ" w/ bugId, too. I'm ok with not bugId adding it for consistency. So are you ok with the "tolerance" wording? Or you still want to replace it with "maxRetries"? I feel "retries" isn't clear enough.. > > > > > > - > > > > > > https://developer.android.com/reference/android/support/test/filters/FlakyTes... > > > > > > The current difference between those and ours is the message field, which is > > not > > > a functional addition.
new patch set only removed bugid
lgtm
boliu@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng to stamp the gn part
Didn't we go to fairly great lengths to remove flaky tests in the past? IIRC the conclusion is we should just disable / remove flaky tests, since no one ever fixes them. Why do we want an explicit test annotation for this behavior?
On 2016/06/23 18:18:40, dcheng wrote: > Didn't we go to fairly great lengths to remove flaky tests in the past? IIRC the > conclusion is we should just disable / remove flaky tests, since no one ever > fixes them. Why do we want an explicit test annotation for this behavior? Want an explicit annotation so that the default can be changed to *not* retry failed tests. I have a design doc linked from the bug: https://docs.google.com/a/chromium.org/document/d/1u3PIlYZ8Op4k9jsVG-WNZ5UvBp... fwiw I agree with you that flaky tests are worse than disabled tests, but others don't, and this is like a compromise
On 2016/06/23 18:23:40, boliu wrote: > On 2016/06/23 18:18:40, dcheng wrote: > > Didn't we go to fairly great lengths to remove flaky tests in the past? IIRC > the > > conclusion is we should just disable / remove flaky tests, since no one ever > > fixes them. Why do we want an explicit test annotation for this behavior? > > Want an explicit annotation so that the default can be changed to *not* retry > failed tests. I have a design doc linked from the bug: > https://docs.google.com/a/chromium.org/document/d/1u3PIlYZ8Op4k9jsVG-WNZ5UvBp... > > fwiw I agree with you that flaky tests are worse than disabled tests, but others > don't, and this is like a compromise Hmm... I think this would be sending a mixed message about tests... Could you start a discussion on chromium-dev? Or if you're OK with bumping a several-year old thread, https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_OwJ_GWfOaY. If we want to have different policies for java tests vs everything else, it should be documented somewhere, at the very least.
On 2016/06/23 19:19:14, dcheng wrote: > On 2016/06/23 18:23:40, boliu wrote: > > On 2016/06/23 18:18:40, dcheng wrote: > > > Didn't we go to fairly great lengths to remove flaky tests in the past? IIRC > > the > > > conclusion is we should just disable / remove flaky tests, since no one ever > > > fixes them. Why do we want an explicit test annotation for this behavior? > > > > Want an explicit annotation so that the default can be changed to *not* retry > > failed tests. I have a design doc linked from the bug: > > > https://docs.google.com/a/chromium.org/document/d/1u3PIlYZ8Op4k9jsVG-WNZ5UvBp... > > > > fwiw I agree with you that flaky tests are worse than disabled tests, but > others > > don't, and this is like a compromise > > Hmm... I think this would be sending a mixed message about tests... > > Could you start a discussion on chromium-dev? Or if you're OK with bumping a > several-year old thread, > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_OwJ_GWfOaY. > If we want to have different policies for java tests vs everything else, it > should be documented somewhere, at the very least. We implicitly have a different policy for android tests right now, that all tests are assumed to be flaky. And proposal this makes that explicit, and not applied on all tests by default. Let me talk to the person who objected to disabling flaky tests and get back to you.. this disagreement was touched on a bit in the internal thread on clank-team "Proposal to remove instrumentation test retry on failure" if you are interested.
On 2016/06/23 19:27:22, boliu wrote: > On 2016/06/23 19:19:14, dcheng wrote: > > On 2016/06/23 18:23:40, boliu wrote: > > > On 2016/06/23 18:18:40, dcheng wrote: > > > > Didn't we go to fairly great lengths to remove flaky tests in the past? > IIRC > > > the > > > > conclusion is we should just disable / remove flaky tests, since no one > ever > > > > fixes them. Why do we want an explicit test annotation for this behavior? > > > > > > Want an explicit annotation so that the default can be changed to *not* > retry > > > failed tests. I have a design doc linked from the bug: > > > > > > https://docs.google.com/a/chromium.org/document/d/1u3PIlYZ8Op4k9jsVG-WNZ5UvBp... > > > > > > fwiw I agree with you that flaky tests are worse than disabled tests, but > > others > > > don't, and this is like a compromise > > > > Hmm... I think this would be sending a mixed message about tests... > > > > Could you start a discussion on chromium-dev? Or if you're OK with bumping a > > several-year old thread, > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_OwJ_GWfOaY. > > If we want to have different policies for java tests vs everything else, it > > should be documented somewhere, at the very least. > > We implicitly have a different policy for android tests right now, that all > tests are assumed to be flaky. Fun fact: this isn't a different policy for android; it's the default policy for chromium: https://codesearch.chromium.org/chromium/src/base/test/launcher/test_launcher... > > And proposal this makes that explicit, and not applied on all tests by default. > > Let me talk to the person who objected to disabling flaky tests and get back to > you.. this disagreement was touched on a bit in the internal thread on > clank-team "Proposal to remove instrumentation test retry on failure" if you are > interested.
On 2016/06/24 09:31:22, jbudorick (EMEA til June 30) wrote: > On 2016/06/23 19:27:22, boliu wrote: > > On 2016/06/23 19:19:14, dcheng wrote: > > > On 2016/06/23 18:23:40, boliu wrote: > > > > On 2016/06/23 18:18:40, dcheng wrote: > > > > > Didn't we go to fairly great lengths to remove flaky tests in the past? > > IIRC > > > > the > > > > > conclusion is we should just disable / remove flaky tests, since no one > > ever > > > > > fixes them. Why do we want an explicit test annotation for this > behavior? > > > > > > > > Want an explicit annotation so that the default can be changed to *not* > > retry > > > > failed tests. I have a design doc linked from the bug: > > > > > > > > > > https://docs.google.com/a/chromium.org/document/d/1u3PIlYZ8Op4k9jsVG-WNZ5UvBp... > > > > > > > > fwiw I agree with you that flaky tests are worse than disabled tests, but > > > others > > > > don't, and this is like a compromise > > > > > > Hmm... I think this would be sending a mixed message about tests... > > > > > > Could you start a discussion on chromium-dev? Or if you're OK with bumping a > > > several-year old thread, > > > > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_OwJ_GWfOaY. > > > If we want to have different policies for java tests vs everything else, it > > > should be documented somewhere, at the very least. > > > > We implicitly have a different policy for android tests right now, that all > > tests are assumed to be flaky. > > Fun fact: this isn't a different policy for android; it's the default policy for > chromium: > https://codesearch.chromium.org/chromium/src/base/test/launcher/test_launcher... I always thought desktop is the goal post for android for things like this. Turns out it's a lot worse than android.. Resetting expectations :( > > > > > And proposal this makes that explicit, and not applied on all tests by > default. > > > > Let me talk to the person who objected to disabling flaky tests and get back > to > > you.. this disagreement was touched on a bit in the internal thread on > > clank-team "Proposal to remove instrumentation test retry on failure" if you > are > > interested.
On 2016/06/24 15:25:20, boliu wrote: > On 2016/06/24 09:31:22, jbudorick (EMEA til June 30) wrote: > > On 2016/06/23 19:27:22, boliu wrote: > > > On 2016/06/23 19:19:14, dcheng wrote: > > > > On 2016/06/23 18:23:40, boliu wrote: > > > > > On 2016/06/23 18:18:40, dcheng wrote: > > > > > > Didn't we go to fairly great lengths to remove flaky tests in the > past? > > > IIRC > > > > > the > > > > > > conclusion is we should just disable / remove flaky tests, since no > one > > > ever > > > > > > fixes them. Why do we want an explicit test annotation for this > > behavior? > > > > > > > > > > Want an explicit annotation so that the default can be changed to *not* > > > retry > > > > > failed tests. I have a design doc linked from the bug: > > > > > > > > > > > > > > > https://docs.google.com/a/chromium.org/document/d/1u3PIlYZ8Op4k9jsVG-WNZ5UvBp... > > > > > > > > > > fwiw I agree with you that flaky tests are worse than disabled tests, > but > > > > others > > > > > don't, and this is like a compromise > > > > > > > > Hmm... I think this would be sending a mixed message about tests... > > > > > > > > Could you start a discussion on chromium-dev? Or if you're OK with bumping > a > > > > several-year old thread, > > > > > > > > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_OwJ_GWfOaY. Just read through that thread. There is one key difference between now and at the start of that thread: Back in 2012, flaky tests were running, but the result were completely ignored (at least according to the thread) Now, we run them, and if they fail 3 times in a row, then we report it as a failure. So today, flaky test does provide some test coverage, whereas it did not back in 2012. > > > > If we want to have different policies for java tests vs everything else, > it > > > > should be documented somewhere, at the very least. > > > > > > We implicitly have a different policy for android tests right now, that all > > > tests are assumed to be flaky. > > > > Fun fact: this isn't a different policy for android; it's the default policy > for > > chromium: > > > https://codesearch.chromium.org/chromium/src/base/test/launcher/test_launcher... > > I always thought desktop is the goal post for android for things like this. > Turns out it's a lot worse than android.. > > Resetting expectations :( > > > > > > > > > And proposal this makes that explicit, and not applied on all tests by > > default. > > > > > > Let me talk to the person who objected to disabling flaky tests and get back > > to > > > you.. this disagreement was touched on a bit in the internal thread on > > > clank-team "Proposal to remove instrumentation test retry on failure" if you > > are > > > interested.
dcheng: so do you think this still needs to block on a chromium-dev discussion/consensus, given desktop is just as bad as android?
On 2016/06/24 23:13:47, boliu wrote: > dcheng: so do you think this still needs to block on a chromium-dev > discussion/consensus, given desktop is just as bad as android? Meh. LGTM =) Since the goal of this is to move towards labeling more tests as non-flaky, I'm supportive. While part of me is curious how often the retry on failure callback gets hit for the C++ test runner, I don't have time to investigate =/
On 2016/06/24 23:37:14, dcheng wrote: > On 2016/06/24 23:13:47, boliu wrote: > > dcheng: so do you think this still needs to block on a chromium-dev > > discussion/consensus, given desktop is just as bad as android? > > Meh. LGTM =) > > Since the goal of this is to move towards labeling more tests as non-flaky, I'm > supportive. While part of me is curious how often the retry on failure callback > gets hit for the C++ test runner, I don't have time to investigate =/ Quite bad imo: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=... Each light blue square means test passed after retry. You can sort by flakiness.
On 2016/06/24 23:52:14, boliu wrote: > On 2016/06/24 23:37:14, dcheng wrote: > > On 2016/06/24 23:13:47, boliu wrote: > > > dcheng: so do you think this still needs to block on a chromium-dev > > > discussion/consensus, given desktop is just as bad as android? > > > > Meh. LGTM =) > > > > Since the goal of this is to move towards labeling more tests as non-flaky, > I'm > > supportive. While part of me is curious how often the retry on failure > callback > > gets hit for the C++ test runner, I don't have time to investigate =/ > > Quite bad imo: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=... ^^ link courtesy of jbudorick > > Each light blue square means test passed after retry. You can sort by flakiness.
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2088093003/#ps40001 (title: "remove tolerance")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== android: Add RetryOnFailure test annotation Note this annotation does not do anything yet without script changes. BUG=619055 ========== to ========== android: Add RetryOnFailure test annotation Note this annotation does not do anything yet without script changes. BUG=619055 Committed: https://crrev.com/1dd26c8c1c6ffc149f46483b56d20ff71cd35732 Cr-Commit-Position: refs/heads/master@{#402030} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1dd26c8c1c6ffc149f46483b56d20ff71cd35732 Cr-Commit-Position: refs/heads/master@{#402030} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
