Convert WebappActivityTestBase and direct children to JUnit4.

Convert WebappActivityTestBase and direct children to JUnit4.

The difference I found is that with previous setup test device was woken up by
tests, while it has to be configured to keep the screen on (as recommended in
android_test_instructions.md). Is this known regression? Or maybe it's
intended?

BUG=640116
Review-Url: https://codereview.chromium.org/2863583002
Cr-Commit-Position: refs/heads/master@{#470809}
Committed: https://chromium.googlesource.com/chromium/src/+/34f6d311a968d33e65faa33312b6298d1460e626

[piotrs, 2017-05-04 07:08:18]

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

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

Dry run: CQ is trying da patch.

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

[piotrs, 2017-05-04 07:12:46]

Patchset #1 (id:1) has been deleted

[piotrs, 2017-05-04 07:13:12]

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

[commit-bot: I haz the power, 2017-05-04 07:13:18]

Dry run: CQ is trying da patch.

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

[piotrs, 2017-05-04 07:33:12]

piotrs@chromium.org changed reviewers:
+ dominickn@chromium.org, yolandyan@chromium.org

[piotrs, 2017-05-04 07:33:13]

Hi folks,

Can you please take a look? This will allow me for some code reuse later and is
a right thing to do.

[commit-bot: I haz the power, 2017-05-04 09:07:49]

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

[commit-bot: I haz the power, 2017-05-04 09:07:50]

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

[the real yoland, 2017-05-04 19:04:43]

Again, thanks a lot for doing this!! I ve been all over these dull conversions
recently, so I am really grate someone else is helping out!

One thing out of curiosity, did you by any chance use this script to run an auto
convert? (https://github.com/yoland68/chromium-junit-auto-migrate/), if not,
feel free to try it out, it does a bunch of tedious work (e.g. add `Assert.` to
every assert call) for you

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java
(right):

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java:114:
startActivityCompletely(intent);
The issue you are running in the try job is that the intent lacks a
FLAG_ACTIVITY_NEW_TASK flag. This is because in JUnit3 styled instrumentation
test, the activities are launched with this flag by default. Yet, this CL is
using the ChromeActivityTestRule/TestBase's special startActivityCompletely,
which doesn't have the flag. This should work if you use launchActivity(Intent
intent). 

Sorry that this difference is very ambiguous right now, but I do intent to
refactor this.

https://android.googlesource.com/platform/frameworks/base/+/master/legacy-tes...

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java
(right):

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java:46:
private Intent createIntent() {
I would recommend just change the createIntent() method in
WebappActivityTestRule to createIntent(String name, String value) and not create
a new method here

[piotrs, 2017-05-04 23:55:17]

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

[commit-bot: I haz the power, 2017-05-04 23:55:54]

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-05 01:10:09]

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

[commit-bot: I haz the power, 2017-05-05 01:10:10]

Dry run: This issue passed the CQ dry run.

[piotrs, 2017-05-05 01:15:53]

Thanks Yoland! Thanks for the pointer with launchActivity()!

Dom, can you take a look, CQ has passed.

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java
(right):

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java:114:
startActivityCompletely(intent);
On 2017/05/04 19:04:43, the real yoland wrote:
> The issue you are running in the try job is that the intent lacks a
> FLAG_ACTIVITY_NEW_TASK flag. This is because in JUnit3 styled instrumentation
> test, the activities are launched with this flag by default. Yet, this CL is
> using the ChromeActivityTestRule/TestBase's special startActivityCompletely,
> which doesn't have the flag. This should work if you use launchActivity(Intent
> intent). 
> 
> Sorry that this difference is very ambiguous right now, but I do intent to
> refactor this.
> 
>
https://android.googlesource.com/platform/frameworks/base/+/master/legacy-tes...

It indeed fixed it! Thanks a lot for that comment :)

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java
(right):

https://codereview.chromium.org/2863583002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java:46:
private Intent createIntent() {
On 2017/05/04 19:04:43, the real yoland wrote:
> I would recommend just change the createIntent() method in
> WebappActivityTestRule to createIntent(String name, String value) and not
create
> a new method here

I got around it just inlining createIntent() with putExtra(), shorter and nicer.
creating createIntent(String name, String value) would be fine as well, but one
tests requires to additional extras, so it's less flexible.

[dominickn, 2017-05-05 02:42:49]

Looks good, just the one question.

https://codereview.chromium.org/2863583002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDeferredStartupTest.java
(right):

https://codereview.chromium.org/2863583002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappDeferredStartupTest.java:31:
ChromeActivityTestRule.DISABLE_NETWORK_PREDICTION_FLAG})
Question: will these tests run without these flags?

[piotrs, 2017-05-05 03:43:26]

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

[commit-bot: I haz the power, 2017-05-05 03:43:39]

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-05 05:48:10]

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

[commit-bot: I haz the power, 2017-05-05 05:48:11]

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

[dominickn, 2017-05-05 05:49:59]

lgtm for the version keeping the flags

[piotrs, 2017-05-05 05:52:15]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-05-05 05:52:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2863583002/#ps80001
(title: "Adding back the flags")

[commit-bot: I haz the power, 2017-05-05 05:52:28]

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-05 06:01:46]

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

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

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[piotrs, 2017-05-05 06:08:43]

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

[commit-bot: I haz the power, 2017-05-05 06:08:58]

Dry run: CQ is trying da patch.

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

[piotrs, 2017-05-05 06:10:13]

piotrs@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[piotrs, 2017-05-05 06:10:14]

Dan, could you please take a look?

[commit-bot: I haz the power, 2017-05-05 07:12:11]

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

[commit-bot: I haz the power, 2017-05-05 07:12:12]

Dry run: This issue passed the CQ dry run.

[gone, 2017-05-05 17:26:03]

I'd defer to Yoland's review l-g-t-m; he knows it better than I do.

[piotrs, 2017-05-07 23:58:15]

Hi Yoland,

Can you please take a second look? To answer your question I was aware of the
script, but decided to do the initial conversion by hand to understand better
what's the scope of changes etc. I will try to convert the rests of tests if
webapp directory later this week with the script and see how that goes.

[the real yoland, 2017-05-09 00:06:54]

lgtm

[the real yoland, 2017-05-09 00:06:54]

lgtm

[piotrs, 2017-05-09 23:48:44]

piotrs@chromium.org changed reviewers:
+ dtrainor@chromium.org
- dfalcantara@chromium.org

[piotrs, 2017-05-09 23:48:44]

Hi David,

Can you please take a look as an owner? 

Cheers,
Piotr

[David Trainor- moved to gerrit, 2017-05-10 20:10:15]

lgtm

[piotrs, 2017-05-10 20:38:16]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:39:31]

CQ is trying da patch.

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

[piotrs, 2017-05-10 23:00:22]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-05-10 23:00:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dtrainor@chromium.org, yolandyan@chromium.org
Link to the patchset: https://codereview.chromium.org/2863583002/#ps100001
(title: "Merge")

[commit-bot: I haz the power, 2017-05-10 23:01:48]

CQ is trying da patch.

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

[piotrs, 2017-05-11 01:33:07]

The CQ bit was unchecked by piotrs@chromium.org

[piotrs, 2017-05-11 01:33:08]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-05-11 01:33:41]

The CQ bit was unchecked by piotrs@chromium.org

[piotrs, 2017-05-11 01:33:56]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-05-11 01:35:40]

CQ is trying da patch.

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

[piotrs, 2017-05-11 02:28:06]

The CQ bit was unchecked by piotrs@chromium.org

[piotrs, 2017-05-11 02:30:42]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-05-11 02:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dtrainor@chromium.org, yolandyan@chromium.org
Link to the patchset: https://codereview.chromium.org/2863583002/#ps120001
(title: "Merge")

[piotrs, 2017-05-11 02:31:29]

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

[commit-bot: I haz the power, 2017-05-11 02:31:37]

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-11 04:34:23]

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

[commit-bot: I haz the power, 2017-05-11 04:34:25]

Dry run: This issue passed the CQ dry run.

[piotrs, 2017-05-11 04:35:31]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:37:02]

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-11 04:47:33]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494477331534280,
"parent_rev": "aed44111a1f008907b532b9232b6075f44e598f2", "commit_rev":
"34f6d311a968d33e65faa33312b6298d1460e626"}

[commit-bot: I haz the power, 2017-05-11 04:47:41]

Description was changed from

==========
Convert WebappActivityTestBase and direct children to JUnit4.

The difference I found is that with previous setup test device was woken up by
tests, while it has to be configured to keep the screen on (as recommended in
android_test_instructions.md). Is this known regression? Or maybe it's
intended?

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

to

==========
Convert WebappActivityTestBase and direct children to JUnit4.

The difference I found is that with previous setup test device was woken up by
tests, while it has to be configured to keep the screen on (as recommended in
android_test_instructions.md). Is this known regression? Or maybe it's
intended?

BUG=640116

Review-Url: https://codereview.chromium.org/2863583002
Cr-Commit-Position: refs/heads/master@{#470809}
Committed:
https://chromium.googlesource.com/chromium/src/+/34f6d311a968d33e65faa33312b6...
==========

[commit-bot: I haz the power, 2017-05-11 04:47:42]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/34f6d311a968d33e65faa33312b6...