📰 Add tests for Tile and TileGroup

[NTP Client] Add tests for Tile and TileGroup

Adds tests focused around TileGroup#buildTiles and
TileGroup#renderTileViews.

Includes some modifications to the EnableFeatures annotation
to mock the FeatureList as soon as the rule is added, without
requiring to set the annotation on each test.

BUG=694297
Review-Url: https://codereview.chromium.org/2722243002
Cr-Commit-Position: refs/heads/master@{#454846}
Committed: https://chromium.googlesource.com/chromium/src/+/31e02521bed88cdfc15b76b2d24065eeda082faf

[dgn, 2017-03-01 17:41:32]

Description was changed from

==========
[NTP Client] Add tests for Tile and TileGroup

Adds tests focused around TileGroup#buildTiles and
TileGroup#renderTileViews.

Includes some modifications to the EnableFeatures annotation
to mock the FeatureList as soon as the rule is added, without
requiring to set the annotation on each test.

BUG=694297
==========

to

==========
[NTP Client] Add tests for Tile and TileGroup

Adds tests focused around TileGroup#buildTiles and
TileGroup#renderTileViews.

Includes some modifications to the EnableFeatures annotation
to mock the FeatureList as soon as the rule is added, without
requiring to set the annotation on each test.

BUG=694297
==========

[dgn, 2017-03-02 14:57:35]

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

[commit-bot: I haz the power, 2017-03-02 14:57:58]

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

[dgn, 2017-03-02 15:07:51]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgn, 2017-03-02 15:07:52]

PTAL

[dgn, 2017-03-02 15:08:33]

dgn@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2017-03-02 16:13:15]

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

[commit-bot: I haz the power, 2017-03-02 16:13:16]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-03-03 11:54:39]

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
File chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java:45: *
choose which features to enable. All are disabled by default.
Ooh, there is one thing I had been thinking of: would it be possible / worth it
to use a Map<String, Boolean> for feature state? The idea being that tests would
explicitly set any feature to enabled or disabled that is checked by the code
under test, and the code would assert if an unknown feature is checked (as
opposed to now, where all features get disabled as soon as you add the rule). We
could do that in a followup though.

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:429:
mLayout = mock(TileGridLayout.class);
Can you use a custom subclass instead?

https://codereview.chromium.org/2722243002/diff/20001/testing/android/junit/j...
File
testing/android/junit/java/src/org/chromium/testing/local/AnnotationProcessor.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/AnnotationProcessor.java:48:
public AnnotationProcessor(Class<T> annotationClass, boolean alwaysApply) {
I find it a bit strange to have AnnotationProcessor not always require an
annotation, but I can't think of a much better way off the top of my head
either.

https://codereview.chromium.org/2722243002/diff/20001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/AnnotationProcessor.java:56:
mAnnotation = description.getAnnotation(mAnnotationClass);
Maybe extract getting the annotation into a helper method, and then do 

  if (mAnnotation != null || mAlwaysApply) return supper.apply(base,
description);

[dgn, 2017-03-03 13:48:31]

PTAL

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
File chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java:45: *
choose which features to enable. All are disabled by default.
On 2017/03/03 11:54:38, Bernhard Bauer wrote:
> Ooh, there is one thing I had been thinking of: would it be possible / worth
it
> to use a Map<String, Boolean> for feature state? The idea being that tests
would
> explicitly set any feature to enabled or disabled that is checked by the code
> under test, and the code would assert if an unknown feature is checked (as
> opposed to now, where all features get disabled as soon as you add the rule).
We
> could do that in a followup though.

Yes we could do that. But I think it becomes interesting only when we define
default states for features. Otherwise turning on only what you need is easy
enough for unit tests, considering that we mock most things. Which features do
you think would benefit most? I'll do it as follow up, yes.

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:429:
mLayout = mock(TileGridLayout.class);
On 2017/03/03 11:54:38, Bernhard Bauer wrote:
> Can you use a custom subclass instead?

Used FrameLayout instead.

https://codereview.chromium.org/2722243002/diff/20001/testing/android/junit/j...
File
testing/android/junit/java/src/org/chromium/testing/local/AnnotationProcessor.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/AnnotationProcessor.java:48:
public AnnotationProcessor(Class<T> annotationClass, boolean alwaysApply) {
On 2017/03/03 11:54:39, Bernhard Bauer wrote:
> I find it a bit strange to have AnnotationProcessor not always require an
> annotation, but I can't think of a much better way off the top of my head
> either.

Currently just adding @EnableFeatures at the class level would mean that the
rule would set a default value for all the tests. So I could remove the
alwaysApply argument. I just thought that adding @Rule should be already
indicating intent, and it feels redundant to still have to set a default. But if
you prefer I can do it that way.

https://codereview.chromium.org/2722243002/diff/20001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/AnnotationProcessor.java:56:
mAnnotation = description.getAnnotation(mAnnotationClass);
On 2017/03/03 11:54:38, Bernhard Bauer wrote:
> Maybe extract getting the annotation into a helper method, and then do 
> 
>   if (mAnnotation != null || mAlwaysApply) return supper.apply(base,
> description);

Done.

[dgn, 2017-03-03 13:48:37]

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

[commit-bot: I haz the power, 2017-03-03 13:48:46]

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

[Bernhard Bauer, 2017-03-03 14:01:23]

lgtm

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
File chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java:45: *
choose which features to enable. All are disabled by default.
On 2017/03/03 13:48:30, dgn wrote:
> On 2017/03/03 11:54:38, Bernhard Bauer wrote:
> > Ooh, there is one thing I had been thinking of: would it be possible / worth
> it
> > to use a Map<String, Boolean> for feature state? The idea being that tests
> would
> > explicitly set any feature to enabled or disabled that is checked by the
code
> > under test, and the code would assert if an unknown feature is checked (as
> > opposed to now, where all features get disabled as soon as you add the
rule).
> We
> > could do that in a followup though.
> 
> Yes we could do that. But I think it becomes interesting only when we define
> default states for features. Otherwise turning on only what you need is easy
> enough for unit tests, considering that we mock most things. Which features do
> you think would benefit most? I'll do it as follow up, yes.

Well, what I'm a bit worried about is if we make a change that makes our code
check a feature it didn't check before. We will just default to off, which may
not match the real-world behavior if the feature in Chrome defaults to on, so we
would avoid catching a hypothetical bug that only happens if the feature is on.
If we require the feature to be set, we would at least have to explicitly define
the feature state we're assuming.

[commit-bot: I haz the power, 2017-03-03 14:59:14]

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

[commit-bot: I haz the power, 2017-03-03 14:59:15]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-03-03 15:53:44]

dgn@chromium.org changed reviewers:
+ dtrainor@chromium.org

[dgn, 2017-03-03 15:53:45]

Thanks!

dtrainor@chromium.org: Please review changes in //testing/android

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
File chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java
(right):

https://codereview.chromium.org/2722243002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/EnableFeatures.java:45: *
choose which features to enable. All are disabled by default.
On 2017/03/03 14:01:22, Bernhard Bauer wrote:
> On 2017/03/03 13:48:30, dgn wrote:
> > On 2017/03/03 11:54:38, Bernhard Bauer wrote:
> > > Ooh, there is one thing I had been thinking of: would it be possible /
worth
> > it
> > > to use a Map<String, Boolean> for feature state? The idea being that tests
> > would
> > > explicitly set any feature to enabled or disabled that is checked by the
> code
> > > under test, and the code would assert if an unknown feature is checked (as
> > > opposed to now, where all features get disabled as soon as you add the
> rule).
> > We
> > > could do that in a followup though.
> > 
> > Yes we could do that. But I think it becomes interesting only when we define
> > default states for features. Otherwise turning on only what you need is easy
> > enough for unit tests, considering that we mock most things. Which features
do
> > you think would benefit most? I'll do it as follow up, yes.
> 
> Well, what I'm a bit worried about is if we make a change that makes our code
> check a feature it didn't check before. We will just default to off, which may
> not match the real-world behavior if the feature in Chrome defaults to on, so
we
> would avoid catching a hypothetical bug that only happens if the feature is
on.
> If we require the feature to be set, we would at least have to explicitly
define
> the feature state we're assuming.

Okay got it. In that case, I reverted the change that makes the rule have some
effect by default, since it would not be useful for that later state. I just
added the current empty annotation at the class level.

[dgn, 2017-03-03 15:53:51]

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

[commit-bot: I haz the power, 2017-03-03 15:54:09]

Dry run: 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, 2017-03-03 16:45:52]

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

[commit-bot: I haz the power, 2017-03-03 16:45:53]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-04 04:09:24]

testing/android lgtm

[dgn, 2017-03-06 09:49:45]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-03-06 09:49:46]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2722243002/#ps60001
(title: "Revert default Rule effect")

[commit-bot: I haz the power, 2017-03-06 09:49:58]

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, 2017-03-06 10:47:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488793785931380,
"parent_rev": "42bd5bf1a4db228077a4b44c70bff956470f0a75", "commit_rev":
"31e02521bed88cdfc15b76b2d24065eeda082faf"}

[commit-bot: I haz the power, 2017-03-06 10:48:14]

Description was changed from

==========
[NTP Client] Add tests for Tile and TileGroup

Adds tests focused around TileGroup#buildTiles and
TileGroup#renderTileViews.

Includes some modifications to the EnableFeatures annotation
to mock the FeatureList as soon as the rule is added, without
requiring to set the annotation on each test.

BUG=694297
==========

to

==========
[NTP Client] Add tests for Tile and TileGroup

Adds tests focused around TileGroup#buildTiles and
TileGroup#renderTileViews.

Includes some modifications to the EnableFeatures annotation
to mock the FeatureList as soon as the rule is added, without
requiring to set the annotation on each test.

BUG=694297

Review-Url: https://codereview.chromium.org/2722243002
Cr-Commit-Position: refs/heads/master@{#454846}
Committed:
https://chromium.googlesource.com/chromium/src/+/31e02521bed88cdfc15b76b2d240...
==========

[commit-bot: I haz the power, 2017-03-06 10:48:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/31e02521bed88cdfc15b76b2d240...

[Michael van Ouwerkerk, 2017-03-09 11:02:59]

post-commit lgtm

Thanks!