Convert Notification Tests to JUnit4

Convert Notification Tests to JUnit4

For more on JUnit4 migration, please check src/testing/android/docs/junit4.md

BUG=640116
Review-Url: https://codereview.chromium.org/2877253002
Cr-Commit-Position: refs/heads/master@{#472317}
Committed: https://chromium.googlesource.com/chromium/src/+/5db525e85eaf72baeecebe4484769cd497eb2ca5

[the real yoland, 2017-05-13 01:06:16]

yolandyan@chromium.org changed reviewers:
+ jbudorick@chromium.org, johnme@chromium.org, mikecase@chromium.org,
tedchoc@chromium.org

[the real yoland, 2017-05-13 01:06:24]

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

[commit-bot: I haz the power, 2017-05-13 01:06:59]

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-05-13 02:09:53]

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

[commit-bot: I haz the power, 2017-05-13 02:09:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbudorick, 2017-05-15 02:59:35]

lgtm w/ nits & q

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:404:
//TODO(yolandyan): remove this after supporing SdkSuppress annotation
What would this require?

also, nits: space after //, supporing -> supporting

[johnme, 2017-05-15 12:21:54]

johnme@chromium.org changed reviewers:
+ awdf@chromium.org
- johnme@chromium.org

[johnme, 2017-05-15 12:21:55]

+awdf, who owns these tests now

[awdf, 2017-05-15 13:23:11]

lgtm :) - you can ignore my 'Asides', unless you have comments!

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:69:
ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG,
I can see it makes sense to disable the first run experience in case it gets in
the way, but I'm curious why we now disable this network prediction flag on
moving to junit 4. Is it because of crbug.com/60035 ?

(Aside - Would be useful if
ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG had a comment about what
this flag does & why a test might want to disable it)

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:93:
Assert.assertEquals(expectedTitle, tab.getTitle());
Aside - In some of my recent junit 4 tests I have been using hamcrest assertThat
assertions for more readable error messages (since the library seemed to be
available already). Is there any guidance in Chromium over what kind of
assertions are preferred?

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:343:
Assert.assertEquals(5.5, getEngagementScoreBlocking(), 0);
Aside: huh, nice that Assert.assertEquals surfaced we are checking for float
equality here, which might cause problems if the numbers/calculation was
changed..

[the real yoland, 2017-05-15 18:19:32]

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:69:
ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG,
On 2017/05/15 at 13:23:11, awdf wrote:
> I can see it makes sense to disable the first run experience in case it gets
in the way, but I'm curious why we now disable this network prediction flag on
moving to junit 4. Is it because of crbug.com/60035 ?
> 
> (Aside - Would be useful if
ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG had a comment about what
this flag does & why a test might want to disable it)

Aha, this test (and all chrome public test apk tests) actually had this flag
enabled long before JUnit4, but it was added in ChromeActivityTestCaseBase and
inherited by NotificationTestBase and this.

Now since the tests no longer inherit from a base class, all the flags are added
individually.

I added a comment here on why disabling it

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:93:
Assert.assertEquals(expectedTitle, tab.getTitle());
On 2017/05/15 at 13:23:10, awdf wrote:
> Aside - In some of my recent junit 4 tests I have been using hamcrest
assertThat assertions for more readable error messages (since the library seemed
to be available already). Is there any guidance in Chromium over what kind of
assertions are preferred?

In my experience, as long as they are not deprecated (which meaning almost
anything from android.test.* should NOT be used)

any of them would work, the JUnit assertThat is almost the same as hamcrest's
version. And Hamcrest's assertions works perfectly fine in JUnit4

I don't think we have a guide about assertions preference, but even with a
guide, people would probably just use of whichever they are
comfortable/knowledgeable :)

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:343:
Assert.assertEquals(5.5, getEngagementScoreBlocking(), 0);
On 2017/05/15 at 13:23:11, awdf wrote:
> Aside: huh, nice that Assert.assertEquals surfaced we are checking for float
equality here, which might cause problems if the numbers/calculation was
changed..

ya, without the delta value, this would actually fail in runtime even if the
values are exactly equal

https://codereview.chromium.org/2877253002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:404:
//TODO(yolandyan): remove this after supporing SdkSuppress annotation
On 2017/05/15 at 02:59:35, jbudorick wrote:
> What would this require?
> 
> also, nits: space after //, supporing -> supporting

This would require test_runner.py side changes, this android instrumentation
output for is weird

Since SdkSuppress is the same as MinSdkLevel, I think we should just replace
MinSdkLevel when we start to support SdkSuppress

Done

[the real yoland, 2017-05-15 18:35:11]

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

[the real yoland, 2017-05-15 18:35:16]

The CQ bit was unchecked by yolandyan@chromium.org

[the real yoland, 2017-05-15 18:35:18]

The CQ bit was checked by yolandyan@chromium.org

[the real yoland, 2017-05-15 18:35:18]

The patchset sent to the CQ was uploaded after l-g-t-m from awdf@chromium.org,
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2877253002/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-15 18:36:09]

CQ is trying da patch.

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

[the real yoland, 2017-05-15 18:55:23]

The CQ bit was unchecked by yolandyan@chromium.org

[the real yoland, 2017-05-16 00:47:51]

+tedchoc for OWNER stamp on
chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java

[Ted C, 2017-05-16 18:31:51]

owners rs lgtm

[the real yoland, 2017-05-16 20:32:21]

The CQ bit was checked by yolandyan@chromium.org

[commit-bot: I haz the power, 2017-05-16 20:35:38]

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-05-17 04:08:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494966741839490,
"parent_rev": "c7e27427380a52010e29cb5e8a69e96a8adaaefe", "commit_rev":
"5db525e85eaf72baeecebe4484769cd497eb2ca5"}

[commit-bot: I haz the power, 2017-05-17 04:08:17]

Description was changed from

==========
Convert Notification Tests to JUnit4

For more on JUnit4 migration, please check src/testing/android/docs/junit4.md

BUG=640116
==========

to

==========
Convert Notification Tests to JUnit4

For more on JUnit4 migration, please check src/testing/android/docs/junit4.md

BUG=640116

Review-Url: https://codereview.chromium.org/2877253002
Cr-Commit-Position: refs/heads/master@{#472317}
Committed:
https://chromium.googlesource.com/chromium/src/+/5db525e85eaf72baeecebe448476...
==========

[commit-bot: I haz the power, 2017-05-17 04:08:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5db525e85eaf72baeecebe448476...