Fix test crashes related to AccountManagerHelper.

Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901
Review-Url: https://codereview.chromium.org/2954393003
Cr-Commit-Position: refs/heads/master@{#484308}
Committed: https://chromium.googlesource.com/chromium/src/+/3c5f49bcc2f5a51c0272ad33bfb38edf10f24fa4

[troyhildebrandt, 2017-06-26 22:55:42]

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

[commit-bot: I haz the power, 2017-06-26 22:56:12]

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-06-26 23:43:56]

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

[commit-bot: I haz the power, 2017-06-26 23:44:00]

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

[troyhildebrandt, 2017-06-29 15:43:58]

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

[commit-bot: I haz the power, 2017-06-29 15:44:13]

Dry run: CQ is trying da patch.

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

[troyhildebrandt, 2017-06-29 15:44:18]

Description was changed from

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread.

BUG=710901
==========

to

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread.

BUG=710901
==========

[troyhildebrandt, 2017-06-29 15:44:18]

thildebr@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mariakhomenko@chromium.org, tedchoc@chromium.org

[troyhildebrandt, 2017-06-29 15:45:11]

Description was changed from

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread.

BUG=710901
==========

to

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901
==========

[troyhildebrandt, 2017-06-29 15:45:27]

Description was changed from

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901
==========

to

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before
initializing the browser process, fixing various flaky tests.

BUG=710901
==========

[troyhildebrandt, 2017-06-29 15:46:06]

Description was changed from

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before
initializing the browser process, fixing various flaky tests.

BUG=710901
==========

to

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901
==========

[troyhildebrandt, 2017-06-29 15:46:06]

Description was changed from

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before
initializing the browser process, fixing various flaky tests.

BUG=710901
==========

to

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901
==========

[Maria, 2017-06-29 16:25:38]

mariakhomenko@chromium.org changed reviewers:
+ yolandyan@chromium.org

[Maria, 2017-06-29 16:25:39]

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java
(left):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:50:
resetSharedPrefs();
Will removing this cause an issue for the next test suite?

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java
(right):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java:62:
ThreadUtils.runOnUiThread(new Runnable() {
Should this be an annotation instead?

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java
(right):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java:40:
ThreadUtils.runOnUiThread(new Runnable() {
@UiThreadTest?

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassinTest.java
(right):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassinTest.java:405:
// now fails because the tab model metadata file is actually incorrect.
This is still passing on bots though -- Dan, can you help figure this out?

[commit-bot: I haz the power, 2017-06-29 16:33:54]

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

[commit-bot: I haz the power, 2017-06-29 16:33:55]

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

[Ted C, 2017-06-29 16:35:34]

Oh hai, that's a nice change, how about we fix something completely unrelated to
it while we're at it :-P

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

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java:41:
new ChromeBrowserTestRule(true /* initBrowserProcess */);
Are there any places that pass false?  The true /* comment */ boilerplate is sad
to see repeated so often.

A couple questions:
1.) Do we need to support false at all for ChromeBrowserTestRule?  Should there
just be a single no-argument constructor that inits the browser process?
2.) If we need to support both, should we make the false case the exception? 
Either by having a setter on the rule (setDoNotInitBrowserProcess), or having a
static function like ChromeBrowserTestRule.createWithoutBrowserProcess init?

In general, I'd like to reduce the number of options that people need to
consider when writing tests and force them into the "right" one.

[troyhildebrandt, 2017-06-29 20:11:53]

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

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java:41:
new ChromeBrowserTestRule(true /* initBrowserProcess */);
On 2017/06/29 16:35:34, Ted C wrote:
> Are there any places that pass false?  The true /* comment */ boilerplate is
sad
> to see repeated so often.
> 
> A couple questions:
> 1.) Do we need to support false at all for ChromeBrowserTestRule?  Should
there
> just be a single no-argument constructor that inits the browser process?
> 2.) If we need to support both, should we make the false case the exception? 
> Either by having a setter on the rule (setDoNotInitBrowserProcess), or having
a
> static function like ChromeBrowserTestRule.createWithoutBrowserProcess init?
> 
> In general, I'd like to reduce the number of options that people need to
> consider when writing tests and force them into the "right" one.

I just removed the option for now since we're not using it. It's not tough to
add again if we need it.

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java
(left):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/accessibility/FontSizePrefsTest.java:50:
resetSharedPrefs();
On 2017/06/29 16:25:39, Maria wrote:
> Will removing this cause an issue for the next test suite?

It shouldn't, it's unclear to me why we were doing it in both the @Before and
@After.

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java
(right):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java:62:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/06/29 16:25:39, Maria wrote:
> Should this be an annotation instead?

Done.

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java
(right):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java:40:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/06/29 16:25:39, Maria wrote:
> @UiThreadTest?

Done.

[gone, 2017-06-29 20:14:12]

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassinTest.java
(right):

https://codereview.chromium.org/2954393003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/DocumentModeAssassinTest.java:405:
// now fails because the tab model metadata file is actually incorrect.
On 2017/06/29 16:25:39, Maria wrote:
> This is still passing on bots though -- Dan, can you help figure this out?

It's failing for me when I run it locally on a KK Nexus 5.

[Ted C, 2017-06-29 20:20:55]

lgtm

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

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java:46:
.around(mBrowserTestRule)
can this just be "new ChromeBrowserTestRule()" here now?

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java
(right):

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java:44:
RuleChain.outerRule(mBrowserTestRule).around(new UiThreadTestRule());
same comment as before...I would be consistent with either creating them both
outside the chain or both inside (applies to the rest of the CL).

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
(right):

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:59:
/**R
STUVWXYandZ!

[troyhildebrandt, 2017-06-29 20:43:48]

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

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java:46:
.around(mBrowserTestRule)
On 2017/06/29 20:20:55, Ted C wrote:
> can this just be "new ChromeBrowserTestRule()" here now?

Done.

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java
(right):

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java:44:
RuleChain.outerRule(mBrowserTestRule).around(new UiThreadTestRule());
On 2017/06/29 20:20:55, Ted C wrote:
> same comment as before...I would be consistent with either creating them both
> outside the chain or both inside (applies to the rest of the CL).
Done.

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
(right):

https://codereview.chromium.org/2954393003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:59:
/**R
On 2017/06/29 20:20:55, Ted C wrote:
> STUVWXYandZ!

Congratulations on finding the easter egg...? Done.

[Maria, 2017-06-29 21:05:30]

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java:260:
public void testRefererUrl_extraHeadersInclRefererMultiple() throws Throwable {
@UiThreadTest?

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:67:
ThreadUtils.runOnUiThread(new Runnable() {
@UiThreadTest for changes in this file?

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java:68:
ThreadUtils.runOnUiThread(new Runnable() {
@UiThreadTest in this file?

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTestRule.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTestRule.java:31:
public ConnectivityCheckerTestRule() {
We shouldn't need this constructor anymore

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/OmniboxUrlEmphasizerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/OmniboxUrlEmphasizerTest.java:225:
ThreadUtils.runOnUiThread(new Runnable() {
You shouldn't need ThreadUtils anymore, right?

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java:42:
public void testSyncUsageAndCrashReporting() throws Throwable {
@UiThreadTest?

[troyhildebrandt, 2017-06-29 21:24:41]

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/IntentHandlerTest.java:260:
public void testRefererUrl_extraHeadersInclRefererMultiple() throws Throwable {
On 2017/06/29 21:05:29, Maria wrote:
> @UiThreadTest?

Missed this one, done.

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:67:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/06/29 21:05:29, Maria wrote:
> @UiThreadTest for changes in this file?

Done.

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkBridgeTest.java:68:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/06/29 21:05:29, Maria wrote:
> @UiThreadTest in this file?

Done.

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTestRule.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/feedback/ConnectivityCheckerTestRule.java:31:
public ConnectivityCheckerTestRule() {
On 2017/06/29 21:05:29, Maria wrote:
> We shouldn't need this constructor anymore

Done.

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/OmniboxUrlEmphasizerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/OmniboxUrlEmphasizerTest.java:225:
ThreadUtils.runOnUiThread(new Runnable() {
On 2017/06/29 21:05:29, Maria wrote:
> You shouldn't need ThreadUtils anymore, right?

Good catch, removed.

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java
(right):

https://codereview.chromium.org/2954393003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/PrivacyPreferencesManagerNativeTest.java:42:
public void testSyncUsageAndCrashReporting() throws Throwable {
On 2017/06/29 21:05:29, Maria wrote:
> @UiThreadTest?

Done.

[troyhildebrandt, 2017-06-29 21:25:08]

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

[commit-bot: I haz the power, 2017-06-29 21:25:29]

Dry run: CQ is trying da patch.

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

[Maria, 2017-06-29 22:28:50]

lgtm

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:70:
public void testNoSpareRendererOnLowEndDevices() throws Throwable {
@UiThreadTest

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestRule.java
(right):

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestRule.java:51:
public TabModelSelectorObserverTestRule() {
empty constructor

[troyhildebrandt, 2017-06-29 22:35:31]

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java
(right):

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:70:
public void testNoSpareRendererOnLowEndDevices() throws Throwable {
On 2017/06/29 22:28:50, Maria wrote:
> @UiThreadTest

Done.

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestRule.java
(right):

https://codereview.chromium.org/2954393003/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestRule.java:51:
public TabModelSelectorObserverTestRule() {
On 2017/06/29 22:28:50, Maria wrote:
> empty constructor

Done.

[troyhildebrandt, 2017-06-29 22:37:31]

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

[commit-bot: I haz the power, 2017-06-29 22:37:57]

Dry run: CQ is trying da patch.

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

[the real yoland, 2017-06-29 23:29:56]

I have a warning on using @UiThreadTest, UiThreadTestRule/@UiThreadTest is kind
of a mess in J4, the problem with using it is that rules will alway run before
@Before and after @After, which means calls in @Before and @After are run on Ui
thread as well with @UiThreadTest, which means calls like startActivity() in
@Before and @After would fail, since they require to be called from
instrumentation thread.
That's why I generally avoided using UiThreadTestRule and @UiThreadTest in
JUnit4.

I do plan on doing a refactor in the future to clear up some of these
@UiThreadTest mess. (e.g. providing an additional annotation like
@MethodOnlyUiThreadTest).

https://codereview.chromium.org/2954393003/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java
(right):

https://codereview.chromium.org/2954393003/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:40:
private final ClearAppDataTestRule mClearAppDataTestRule =
I would advocate creating ClearAppDataTestRule as such, unless some kind of mock
Context object would be used for it
```
public class ClearAppDataTestRule implements TestRule {
    @Override
    public Statement apply(final Statement base, Description description) {
        return new Statement() {
            @Override
            public void evaluate() throws Throwable {
               
ApplicationData.clearAppData(InstrumentationRegistry.getContext());
            }
        };
    }
}
```

https://codereview.chromium.org/2954393003/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:45:
RuleChain.outerRule(mClearAppDataTestRule).around(new ChromeBrowserTestRule());
if ClearAppDataTestRule takes no input arguments, this part can be:
```
public final RuleChain mChain = RuleChain
    .outerRule(new ClearAppDataTestRule())
    .around(new ChromeBrowserTestRule());
```

since it is a bit confusing when some TestRules are annotated with @Rule, others
are not.

[commit-bot: I haz the power, 2017-06-30 00:35:28]

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

[commit-bot: I haz the power, 2017-06-30 00:35:30]

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

[troyhildebrandt, 2017-07-05 16:05:11]

On 2017/06/29 23:29:56, the real yoland wrote:
> I have a warning on using @UiThreadTest, UiThreadTestRule/@UiThreadTest is
kind
> of a mess in J4, the problem with using it is that rules will alway run before
> @Before and after @After, which means calls in @Before and @After are run on
Ui
> thread as well with @UiThreadTest, which means calls like startActivity() in
> @Before and @After would fail, since they require to be called from
> instrumentation thread.
> That's why I generally avoided using UiThreadTestRule and @UiThreadTest in
> JUnit4.
> 
> I do plan on doing a refactor in the future to clear up some of these
> @UiThreadTest mess. (e.g. providing an additional annotation like
> @MethodOnlyUiThreadTest).
> 

Good point, thanks for the heads up.

I'll gladly switch the tests over to something more appropriate in the future,
since the use of @UiThreadTest is hit and miss right now.

[troyhildebrandt, 2017-07-05 16:08:39]

https://codereview.chromium.org/2954393003/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java
(right):

https://codereview.chromium.org/2954393003/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:40:
private final ClearAppDataTestRule mClearAppDataTestRule =
On 2017/06/29 23:29:55, the real yoland wrote:
> I would advocate creating ClearAppDataTestRule as such, unless some kind of
mock
> Context object would be used for it
> ```
> public class ClearAppDataTestRule implements TestRule {
>     @Override
>     public Statement apply(final Statement base, Description description) {
>         return new Statement() {
>             @Override
>             public void evaluate() throws Throwable {
>                
> ApplicationData.clearAppData(InstrumentationRegistry.getContext());
>             }
>         };
>     }
> }
> ```

Done.

https://codereview.chromium.org/2954393003/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:45:
RuleChain.outerRule(mClearAppDataTestRule).around(new ChromeBrowserTestRule());
On 2017/06/29 23:29:55, the real yoland wrote:
> if ClearAppDataTestRule takes no input arguments, this part can be:
> ```
> public final RuleChain mChain = RuleChain
>     .outerRule(new ClearAppDataTestRule())
>     .around(new ChromeBrowserTestRule());
> ```
> 
> since it is a bit confusing when some TestRules are annotated with @Rule,
others
> are not.

Done.

[the real yoland, 2017-07-05 16:31:17]

lgtm

[troyhildebrandt, 2017-07-05 16:32:05]

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

[commit-bot: I haz the power, 2017-07-05 16:32:15]

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-07-05 17:55:42]

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

[commit-bot: I haz the power, 2017-07-05 17:55:43]

Dry run: This issue passed the CQ dry run.

[troyhildebrandt, 2017-07-05 18:02:01]

The CQ bit was checked by thildebr@chromium.org

[troyhildebrandt, 2017-07-05 18:02:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2954393003/#ps140001
(title: ".")

[commit-bot: I haz the power, 2017-07-05 18:02:25]

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-07-05 18:08:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499277721219210,
"parent_rev": "1dc41f2bad55034793426e5af70ef70485a7644b", "commit_rev":
"3c5f49bcc2f5a51c0272ad33bfb38edf10f24fa4"}

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

Description was changed from

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901
==========

to

==========
Fix test crashes related to AccountManagerHelper.

Moves tests from NativeLibraryTestRule to ChromeBrowserTestRule where
appropriate to stop tests from crashing locally with the AccountManagerHelper
not initialized error.

Also uses RuleChain with UiThreadTestRule to clean up tests that require
running on the UI thread, and with the new ClearAppDataRule to ensure app data
is cleared before initializing the browser process, fixing various flaky tests.

BUG=710901

Review-Url: https://codereview.chromium.org/2954393003
Cr-Commit-Position: refs/heads/master@{#484308}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c5f49bcc2f5a51c0272ad33bfb3...
==========

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

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3c5f49bcc2f5a51c0272ad33bfb3...