The bug this is blocking is a P1 (we need our first run tests re-enabled), so bumping the priority. 

I'm wondering if we can do something to fix the FRE issues in the short term while addressing the longer-term issues here in a longer time frame.

We could clear the preferences that test cares about before running the tests.

Yun, can you help follow up with this?

Concretely, here's the code that deletes files:
https://cs.chromium.org/chromium/src/chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationData.java?rcl=96fced3741c7b8833fb0d96eb9a0b5332ca47b85&l=81

We can actually wipe all keys in default SharedPreferences by calling
ContextUtils.getAppSharedPreferences().edit().clear().commit();

To fix https://crbug.com/916717, I've added CommitSharedPreferencesTestRule, which is executed after all other test rules. This rule may be a good place to add the aforementioned call.

Is it okay to clear all keys in SharedPreferences before each test?
It seems we can add that in CommitSharedPreferencesTestRule as bsazonov@ mentioned above.

It seems clearing SharedPreferences between tests is gonna take time due to twellington@'s last comment on https://bugs.chromium.org/p/chromium/issues/detail?id=902774 (not tested)

I made that comment based on earlier discussion in this bug (not any sort of investigation).

Okay np. I tried clear all keys between each test and seems autofill test, org.chromium.chrome.browser.autofill.keyboard_accessory.ManualFillingUiCaptureTest, will fail due to no saved password or account.
Probably need to revert to some initial state other than wiping all out?

Is that the only test that fails? If so, we could look at making updates to the setup for that suite (+fhorschig@).

Yeah it was the only fail yesterday but seems other flaky tests also failed today. Let me double check the result.

Clearing all data shouldn't break any tests since they should be isolated.

Since the start of this week, I am looking into multiple keyboard_accessory.* tests as I suspected they fail due to isolation issues (if they fail on bots, they pretty much always do in unison).

Do you know some steps to reproduce the failures locally?
Clearing data according to #8 didn't do the trick (even when the complete chrome_public_test_apk ran).

Chatted with fhorschig@ offline and seems need to clear SharedPreferences before each test to reproduce that failure, which ManualFillingUiCaptureTest is the only failed test
https://chromium-review.googlesource.com/c/chromium/src/+/1399542/3
I just added clear() into CommitSharedPreferencesTestRule since CommitSharedPreferencesTestRule is the outermost TestRule and will execute first before each test? Or there might be a better way to implement if anyone suggests

That seems reasonable to me. Looks like +bsazonov@ added CommitSharedPreferencesTestRule, so would be good to get his opinion as well.

Adding clear() to CommitSharedPreferencesTestRule sounds good to me to significantly reduce the flakiness.


On a more general note: I'm not actually sure whether wiping the state completely in tests is even possible with our current approach. I've seen many places with code resembling the following snippet:

static class MyAwesomeSingleton {
    private static MyAwesomeSingleton sInstance;

    public MyAwesomeSingleton get() { ... }

    public void doAndReply(Runnable runnable) {
        new AsyncTask<Void>() {
            @Override
            protected Void doInBackground() {
                doSomething();
                return null;
            }

            @Override
            protected void onPostExecute(Void result) {
                runnable.run();
            }
        }.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
    }
}

If doAndReply is invoked during the test, it's quite hard to make sure the state is correctly wiped. Runnable passed to the method may contain pretty much everything - SharedPreferences edits, accessing other singletons that we have already reset, etc. This can be fixed, but it would require substantial changes in many classes. And these changes won't benefit the production environment, as Chrome on Android doesn't have a shutdown phase at all.

I hope that I'm wrong - please correct me.

Ensuring background tasks have stopped and queued UI tasks are flushed should be possible. Seems like a good follow-up to the immediate goal of clearing sharedprefs.

For background tasks, ExecutorService has:
shutdown() and awaitTermination() 

For ui thread, there's MessageQueue.addIdleHandler().

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/937f6250959d6b976b2a52a3d8b5360af1c80957

commit 937f6250959d6b976b2a52a3d8b5360af1c80957
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Thu Jan 10 08:55:59 2019

[Android] Deflake ManualFillingUiCaptureTests

The tests fail while waiting for the home button (which I mistakenly
used as a more stable signal that the UI is done loading).
Apparently, the home button depends on AppSharedPreferences which
are not guaranteed to enable it.
Removing that and increasing the waiting time instead.

Bug: 911177,  913178 , 908174
Change-Id: I3da5a586dbeeb8bad21523a77e02ce8c46d9467e
Reviewed-on: https://chromium-review.googlesource.com/c/1403455
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621510}
[modify] https://crrev.com/937f6250959d6b976b2a52a3d8b5360af1c80957/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingTestHelper.java
[modify] https://crrev.com/937f6250959d6b976b2a52a3d8b5360af1c80957/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingUiCaptureTest.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06acfc5326b0f0dac55b5993444d225e79060eb3

commit 06acfc5326b0f0dac55b5993444d225e79060eb3
Author: Yun Liu <yliuyliu@google.com>
Date: Mon Jan 14 20:15:13 2019

Clear app's SharedPreferences before each test to reduce flakiness.

It should fix FirstRun tests flakiness issue if FirstRun tests were
failed because shared pref being set by a previous test.

Bug: 908174, 902774
Change-Id: I9480750a60da1344a72e105ffc444dd982cb36b1
Reviewed-on: https://chromium-review.googlesource.com/c/1399542
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Yun Liu <yliuyliu@google.com>
Cr-Commit-Position: refs/heads/master@{#622569}
[modify] https://crrev.com/06acfc5326b0f0dac55b5993444d225e79060eb3/base/test/android/javatests/src/org/chromium/base/test/CommitSharedPreferencesTestRule.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c298c962528b108c5916a3f31d0ffb1936bb5cf

commit 2c298c962528b108c5916a3f31d0ffb1936bb5cf
Author: Yun Liu <yliuyliu@google.com>
Date: Wed Jan 16 00:42:38 2019

Re-enable FirstRunIntegration tests.

Re-enable these tests after change to clear SharedPreferences before each test.
Related cl: https://crrev.com/c/1399542

Change-Id: I4fba9c11cbf2168a924ca867965c92a9046ff36e
Bug: 908174, 902774
Reviewed-on: https://chromium-review.googlesource.com/c/1409821
Commit-Queue: Yun Liu <yliuyliu@google.com>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622941}
[modify] https://crrev.com/2c298c962528b108c5916a3f31d0ffb1936bb5cf/chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java