Mark all chrome sync tests with @RetryOnFailure

Mark all chrome sync tests with @RetryOnFailure

Current theory is all chrome sync tests are flaky due to
crbug.com/637448. So mark all of them as requiring retry.

BUG=619055, 637448
Committed: https://crrev.com/d7532546c368710e3b5c8b935151d3e60599ca6e
Cr-Commit-Position: refs/heads/master@{#417974}

[boliu, 2016-09-08 20:53:27]

Description was changed from

==========
Mark flaky chrome sync tests with @RetryOnFailure

BUG=619055
==========

to

==========
Mark flaky chrome sync tests with @RetryOnFailure

Also update @RetryOnFailure to apply to an entire class.

BUG=619055
==========

[boliu, 2016-09-08 20:54:07]

boliu@chromium.org changed reviewers:
+ jbudorick@chromium.org, nyquist@chromium.org

[boliu, 2016-09-08 20:54:07]

ptal

[jbudorick, 2016-09-09 19:58:53]

lgtm w/ nit

https://codereview.chromium.org/2319863004/diff/1/base/test/android/javatests...
File
base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java
(right):

https://codereview.chromium.org/2319863004/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/util/RetryOnFailure.java:21:
@Target({ElementType.METHOD, ElementType.TYPE})
nit: remove from this CL

[boliu, 2016-09-09 20:27:49]

boliu@chromium.org changed reviewers:
+ zea@chromium.org

[boliu, 2016-09-09 20:27:49]

+zea since nyquist is out

[boliu, 2016-09-09 20:29:14]

Description was changed from

==========
Mark flaky chrome sync tests with @RetryOnFailure

Also update @RetryOnFailure to apply to an entire class.

BUG=619055
==========

to

==========
Mark flaky chrome sync tests with @RetryOnFailure

BUG=619055
==========

[Nicolas Zea, 2016-09-09 22:12:03]

I'm a bit concerned about leaving these annotations throughout tests is a way
that doesn't make it clear why some have it and others don't.

It raises the question: why don't we mark all tests @RetryOnFailure? Also,
should we be adding a comment that explains the tests are flaky, and mention a
bug number (similar to how we do when we disable tests)?

[boliu, 2016-09-09 22:18:04]

See the bug, and in particular, the design doc linked from the bug.

On 2016/09/09 22:12:03, Nicolas Zea wrote:
> I'm a bit concerned about leaving these annotations throughout tests is a way
> that doesn't make it clear why some have it and others don't.

I think this needs a PSA whe we flip the switch to make the annotation actually
do something. But right now it's just ignored.

> 
> It raises the question: why don't we mark all tests @RetryOnFailure?

That would imply no change.

> Also,
> should we be adding a comment that explains the tests are flaky, and mention a
> bug number (similar to how we do when we disable tests)?

I'm most definitely *not* taking the responsibility of triaging the flakes.

[boliu, 2016-09-09 22:34:29]

On 2016/09/09 22:18:04, boliu wrote:
> See the bug, and in particular, the design doc linked from the bug.
> 
> On 2016/09/09 22:12:03, Nicolas Zea wrote:
> > I'm a bit concerned about leaving these annotations throughout tests is a
way
> > that doesn't make it clear why some have it and others don't.
> 
> I think this needs a PSA whe we flip the switch to make the annotation
actually
> do something. But right now it's just ignored.
> 
> > 
> > It raises the question: why don't we mark all tests @RetryOnFailure?
> 
> That would imply no change.

Actually, given sync tests are 90% flaky, maybe it actually make sense to mark
all of them as such, and all sync tests will still be retried on failure even
after we flip the switch.

But the original goal at least for other test suites is that the tests that are
not currently flaky do not get retried, so they don't silently become flaky.

> 
> > Also,
> > should we be adding a comment that explains the tests are flaky, and mention
a
> > bug number (similar to how we do when we disable tests)?
> 
> I'm most definitely *not* taking the responsibility of triaging the flakes.

[Nicolas Zea, 2016-09-09 22:39:01]

Ah, missed that we're moving away from automatic retries. Makes sense. I'm still
concerned about how we're picking and choosing tests here.

Agreed that given so much of the sync tests are flaking, it might make more
sense to mark them all. The current issues all do seem to stem from
crbug.com/637448. How about mentioning that bug for each flaky annotation (and
having it be part of the commit message).

Then, fixing that bug (on our side/the signin side) can involve removing the
flaky annotation and possibly doing some proper triaging of whatever test then
have their own issues.

[boliu, 2016-09-12 17:10:37]

Description was changed from

==========
Mark flaky chrome sync tests with @RetryOnFailure

BUG=619055
==========

to

==========
Mark all chrome sync tests with @RetryOnFailure

Current theory is all chrome sync tests are flaky due to
crbug.com/637448. So mark all of them as requiring retry.

BUG=619055, 637448
==========

[boliu, 2016-09-12 17:11:16]

ptal

marked all tests as needing retry, and added that bug

[Nicolas Zea, 2016-09-12 17:23:47]

lgtm

[boliu, 2016-09-12 17:33:00]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-09-12 17:33:01]

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/2319863004/#ps40001
(title: "mark all flaky")

[commit-bot: I haz the power, 2016-09-12 17:33:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 18:16:29]

Description was changed from

==========
Mark all chrome sync tests with @RetryOnFailure

Current theory is all chrome sync tests are flaky due to
crbug.com/637448. So mark all of them as requiring retry.

BUG=619055, 637448
==========

to

==========
Mark all chrome sync tests with @RetryOnFailure

Current theory is all chrome sync tests are flaky due to
crbug.com/637448. So mark all of them as requiring retry.

BUG=619055, 637448
==========

[commit-bot: I haz the power, 2016-09-12 18:16:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-12 18:18:06]

Description was changed from

==========
Mark all chrome sync tests with @RetryOnFailure

Current theory is all chrome sync tests are flaky due to
crbug.com/637448. So mark all of them as requiring retry.

BUG=619055, 637448
==========

to

==========
Mark all chrome sync tests with @RetryOnFailure

Current theory is all chrome sync tests are flaky due to
crbug.com/637448. So mark all of them as requiring retry.

BUG=619055, 637448

Committed: https://crrev.com/d7532546c368710e3b5c8b935151d3e60599ca6e
Cr-Commit-Position: refs/heads/master@{#417974}
==========

[commit-bot: I haz the power, 2016-09-12 18:18:07]

Patchset 3 (id:??) landed as
https://crrev.com/d7532546c368710e3b5c8b935151d3e60599ca6e
Cr-Commit-Position: refs/heads/master@{#417974}