🎯 Fail if an instrumentation test is missing size annotation

Fail if an instrumentation test is missing size annotation

Right now we just silently never run such tests.

BUG=601464
Committed: https://crrev.com/70d776ed260e4c48a4f4a92903030f1e578ef291
Cr-Commit-Position: refs/heads/master@{#391288}

[agrieve, 2016-04-07 02:21:13]

Description was changed from

==========
Fix instrumentation test filtering when no annotations exist

This was causing webrtc's AppRTCDemoTest to report having 0 tests.

BUG=none
==========

to

==========
Fix instrumentation test filtering when no annotations exist

This was causing webrtc's AppRTCDemoTest to report having 0 tests.

BUG=none
==========

[agrieve, 2016-04-07 02:21:13]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org

[agrieve, 2016-04-07 02:21:19]

On 2016/04/07 02:21:13, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:jbudorick@chromium.org

[jbudorick, 2016-04-07 02:30:11]

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/instrumentation_test_instance.py:550: if not
self._annotations or not all_annotations:
I don't think this is logically consistent. A test with no annotations
_shouldn't_ match on a non-empty set of desired annotations.

The appropriate fixes would be to either: 
 - support WebRTC providing a completely empty set of desired annotations, or
 - have WebRTC add size annotations to their tests.

[jbudorick, 2016-04-07 02:31:26]

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/instrumentation_test_instance.py:550: if not
self._annotations or not all_annotations:
On 2016/04/07 02:30:11, jbudorick wrote:
> I don't think this is logically consistent. A test with no annotations
> _shouldn't_ match on a non-empty set of desired annotations.
> 
> The appropriate fixes would be to either: 
>  - support WebRTC providing a completely empty set of desired annotations, or
>  - have WebRTC add size annotations to their tests.

(or 3, remove the default annotations entirely, but that's a change with a large
potential for side effects)

[agrieve, 2016-04-07 02:33:09]

On 2016/04/07 02:21:19, agrieve wrote:
> On 2016/04/07 02:21:13, agrieve wrote:
> > mailto:agrieve@chromium.org changed reviewers:
> > + mailto:jbudorick@chromium.org

btw - anything you know of preventing us from marking 0/0 tests ran as an error?

[jbudorick, 2016-04-07 02:35:39]

On 2016/04/07 02:33:09, agrieve wrote:
> On 2016/04/07 02:21:19, agrieve wrote:
> > On 2016/04/07 02:21:13, agrieve wrote:
> > > mailto:agrieve@chromium.org changed reviewers:
> > > + mailto:jbudorick@chromium.org
> 
> btw - anything you know of preventing us from marking 0/0 tests ran as an
error?

I don't. I'm not sure if I agree with that or not, though. I'd have to think
about it.

[agrieve, 2016-04-07 02:41:12]

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/instrumentation_test_instance.py:550: if not
self._annotations or not all_annotations:
On 2016/04/07 02:31:26, jbudorick wrote:
> On 2016/04/07 02:30:11, jbudorick wrote:
> > I don't think this is logically consistent. A test with no annotations
> > _shouldn't_ match on a non-empty set of desired annotations.
> > 
> > The appropriate fixes would be to either: 
> >  - support WebRTC providing a completely empty set of desired annotations,
or
> >  - have WebRTC add size annotations to their tests.
> 
> (or 3, remove the default annotations entirely, but that's a change with a
large
> potential for side effects)

Should we add an exception when we detect a test method with no annotation?
Seems a bit weird that we can detect test methods, and then have no way for them
to not get filtered out. This change special-cases them so that no annotation
means always included it, but I think if we don't want that logic, we should
throw an error if we find an unannotated test (since there is no way to run it).
wdyt?

[agrieve, 2016-04-07 02:43:24]

On 2016/04/07 02:35:39, jbudorick wrote:
> On 2016/04/07 02:33:09, agrieve wrote:
> > On 2016/04/07 02:21:19, agrieve wrote:
> > > On 2016/04/07 02:21:13, agrieve wrote:
> > > > mailto:agrieve@chromium.org changed reviewers:
> > > > + mailto:jbudorick@chromium.org
> > 
> > btw - anything you know of preventing us from marking 0/0 tests ran as an
> error?
> 
> I don't. I'm not sure if I agree with that or not, though. I'd have to think
> about it.

Yeah, in this case it came up from the filtering removing them all, but usually
i hit it when gtests crash while listing tests. Certainly the latter case is an
error, but your right that the first case is more of a grey area. E.g. running
all LargeTests when there are no LargeTests probably shouldn't be a failure...
I'll think on it as well.

[jbudorick, 2016-04-07 02:45:17]

On 2016/04/07 02:43:24, agrieve wrote:
> On 2016/04/07 02:35:39, jbudorick wrote:
> > On 2016/04/07 02:33:09, agrieve wrote:
> > > On 2016/04/07 02:21:19, agrieve wrote:
> > > > On 2016/04/07 02:21:13, agrieve wrote:
> > > > > mailto:agrieve@chromium.org changed reviewers:
> > > > > + mailto:jbudorick@chromium.org
> > > 
> > > btw - anything you know of preventing us from marking 0/0 tests ran as an
> > error?
> > 
> > I don't. I'm not sure if I agree with that or not, though. I'd have to think
> > about it.
> 
> Yeah, in this case it came up from the filtering removing them all, but
usually
> i hit it when gtests crash while listing tests. Certainly the latter case is
an
> error, but your right that the first case is more of a grey area. E.g. running
> all LargeTests when there are no LargeTests probably shouldn't be a failure...
> I'll think on it as well.

This particular case -- gtests crash during --gtest-list-tests -- *should* have
been fixed by hzl@ in https://codereview.chromium.org/1716863002/

[jbudorick, 2016-04-07 02:50:18]

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/instrumentation_test_instance.py:550: if not
self._annotations or not all_annotations:
On 2016/04/07 02:41:12, agrieve wrote:
> On 2016/04/07 02:31:26, jbudorick wrote:
> > On 2016/04/07 02:30:11, jbudorick wrote:
> > > I don't think this is logically consistent. A test with no annotations
> > > _shouldn't_ match on a non-empty set of desired annotations.
> > > 
> > > The appropriate fixes would be to either: 
> > >  - support WebRTC providing a completely empty set of desired annotations,
> or
> > >  - have WebRTC add size annotations to their tests.
> > 
> > (or 3, remove the default annotations entirely, but that's a change with a
> large
> > potential for side effects)
> 
> Should we add an exception when we detect a test method with no annotation?
> Seems a bit weird that we can detect test methods, and then have no way for
them
> to not get filtered out. This change special-cases them so that no annotation
> means always included it, but I think if we don't want that logic, we should
> throw an error if we find an unannotated test (since there is no way to run
it).
> wdyt?

Honestly, I think the right answer for this is probably either to have WebRTC
add annotations or to remove the default set and only do annotation-based
filtering if a user explicitly passes one, and I'm leaning toward the latter.

In order to confidently do that, we'd have to figure out which tests in each of
the four chromium suites aren't annotated with any of the default annotations or
the default excluded annotations.

[agrieve, 2016-04-07 14:12:15]

Description was changed from

==========
Fix instrumentation test filtering when no annotations exist

This was causing webrtc's AppRTCDemoTest to report having 0 tests.

BUG=none
==========

to

==========
Fix instrumentation test filtering when no annotations exist

This was causing webrtc's AppRTCDemoTest to report having 0 tests.

BUG=601464
==========

[agrieve, 2016-04-07 14:13:47]

On 2016/04/07 02:50:18, jbudorick wrote:
>
https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
> File build/android/pylib/instrumentation/instrumentation_test_instance.py
> (right):
> 
>
https://codereview.chromium.org/1863353002/diff/1/build/android/pylib/instrum...
> build/android/pylib/instrumentation/instrumentation_test_instance.py:550: if
not
> self._annotations or not all_annotations:
> On 2016/04/07 02:41:12, agrieve wrote:
> > On 2016/04/07 02:31:26, jbudorick wrote:
> > > On 2016/04/07 02:30:11, jbudorick wrote:
> > > > I don't think this is logically consistent. A test with no annotations
> > > > _shouldn't_ match on a non-empty set of desired annotations.
> > > > 
> > > > The appropriate fixes would be to either: 
> > > >  - support WebRTC providing a completely empty set of desired
annotations,
> > or
> > > >  - have WebRTC add size annotations to their tests.
> > > 
> > > (or 3, remove the default annotations entirely, but that's a change with a
> > large
> > > potential for side effects)
> > 
> > Should we add an exception when we detect a test method with no annotation?
> > Seems a bit weird that we can detect test methods, and then have no way for
> them
> > to not get filtered out. This change special-cases them so that no
annotation
> > means always included it, but I think if we don't want that logic, we should
> > throw an error if we find an unannotated test (since there is no way to run
> it).
> > wdyt?
> 
> Honestly, I think the right answer for this is probably either to have WebRTC
> add annotations or to remove the default set and only do annotation-based
> filtering if a user explicitly passes one, and I'm leaning toward the latter.
> 
> In order to confidently do that, we'd have to figure out which tests in each
of
> the four chromium suites aren't annotated with any of the default annotations
or
> the default excluded annotations.

This is clearly bug-worthy:
https://bugs.chromium.org/p/chromium/issues/detail?id=601464

I think I'm now agreeing with you that we should just annotate all the tests.
Would probably also make sense to raise an exception if a test is missing an
annotation from the default (complete, right?) set of tests.

[agrieve, 2016-04-27 19:14:11]

Description was changed from

==========
Fix instrumentation test filtering when no annotations exist

This was causing webrtc's AppRTCDemoTest to report having 0 tests.

BUG=601464
==========

to

==========
Fail if an instrumentation test is missing size annotation

Right now we just silently never run such tests.

BUG=601464
==========

[agrieve, 2016-04-27 19:14:27]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 19:14:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863353002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863353002/20001

[commit-bot: I haz the power, 2016-04-27 20:27:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 20:27:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-04-29 00:34:38]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2016-04-29 00:35:11]

On 2016/04/27 20:27:25, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

Updated. ptal

[commit-bot: I haz the power, 2016-04-29 00:35:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863353002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863353002/40001

[jbudorick, 2016-04-29 00:38:34]

lgtm w/ question

https://codereview.chromium.org/1863353002/diff/40001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1863353002/diff/40001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:566: if not
self._annotations or not all_annotations:
Can `not all_annotations` ever be true here?

[commit-bot: I haz the power, 2016-04-29 02:22:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 02:22:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-04-29 19:26:02]

https://codereview.chromium.org/1863353002/diff/40001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1863353002/diff/40001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:566: if not
self._annotations or not all_annotations:
On 2016/04/29 00:38:34, jbudorick wrote:
> Can `not all_annotations` ever be true here?

Whoops, meant to revert that part! Done.

[agrieve, 2016-04-29 19:27:05]

agrieve@chromium.org changed reviewers:
+ sgurun@chromium.org, yfriedman@chromium.org

[agrieve, 2016-04-29 19:27:06]

sgurun@chromium.org: Please review changes in android_webview

yfriedman@chromium.org: Please review changes in base and chrome

[agrieve, 2016-04-29 19:27:40]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 19:28:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863353002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863353002/80001

[sgurun-gerrit only, 2016-04-29 21:17:01]

On 2016/04/29 19:28:08, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1863353002/80001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1863353002/80001

himm, I actually did not know we did not run these tests. LGTM!

[Yaron, 2016-04-29 21:23:26]

lgtm

[commit-bot: I haz the power, 2016-04-29 21:29:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 21:29:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-05-03 16:45:14]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-05-03 16:45:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, sgurun@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1863353002/#ps100001
(title: "add annotation for SiteSettingsPreferencesTest.java")

[commit-bot: I haz the power, 2016-05-03 16:45:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863353002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863353002/100001

[commit-bot: I haz the power, 2016-05-03 17:55:14]

Description was changed from

==========
Fail if an instrumentation test is missing size annotation

Right now we just silently never run such tests.

BUG=601464
==========

to

==========
Fail if an instrumentation test is missing size annotation

Right now we just silently never run such tests.

BUG=601464
==========

[commit-bot: I haz the power, 2016-05-03 17:55:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-03 17:57:02]

Description was changed from

==========
Fail if an instrumentation test is missing size annotation

Right now we just silently never run such tests.

BUG=601464
==========

to

==========
Fail if an instrumentation test is missing size annotation

Right now we just silently never run such tests.

BUG=601464

Committed: https://crrev.com/70d776ed260e4c48a4f4a92903030f1e578ef291
Cr-Commit-Position: refs/heads/master@{#391288}
==========

[commit-bot: I haz the power, 2016-05-03 17:57:03]

Patchset 6 (id:??) landed as
https://crrev.com/70d776ed260e4c48a4f4a92903030f1e578ef291
Cr-Commit-Position: refs/heads/master@{#391288}

[Mathieu, 2016-05-03 20:25:10]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1945913002/ by mathp@chromium.org.

The reason for reverting is: One of the touched tests failed on the bots

https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2....