Issue 890439  has been merged into this issue.

I've seen several runs where the tests pass, but 'collect_cmd' fails,

A success log: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8933910086538252480/+/steps/unit_tests_on_Android_device_Nexus_5X/0/stdout

A failed log: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8933868774705144496/+/steps/unit_tests_on_Android_device_Nexus_5X/0/stdout

The only obvious differences are the lines:
  WARNING:root:collect_cmd had non-zero return code: 87
  step returned non-zero exit code: 87

I'm seeing something different. It seems like the testing framework sometimes fails to inject events:

Caused by: java.lang.SecurityException: Injecting to another application requires INJECT_EVENTS permission
  	at android.os.Parcel.readException(Parcel.java:1620)
  	at android.os.Parcel.readException(Parcel.java:1573)
  	at android.hardware.input.IInputManager$Stub$Proxy.injectInputEvent(IInputManager.java:434)
  	at android.hardware.input.InputManager.injectInputEvent(InputManager.java:798)
  	at java.lang.reflect.Method.invoke(Method.java)
  	at android.support.test.espresso.base.InputManagerEventInjectionStrategy.innerInjectMotionEvent(InputManagerEventInjectionStrategy.java:146)
  	... 11 more

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8933175565210462256/+/steps/chrome_public_test_apk_on_Android_device_Nexus_5X/0/logs/org.chromium.chrome.browser.preferences.password.SavePasswordsPreferencesTest_testSearchViewCloseIconExistsOnlyToClearQueries/0

tedchoc: Could you help triage?

Looks we have assigned component and owner to this bug. Removing the bug from sheriff's queue

Assigning to vabr@ who owns SavePasswordsPreferencesTest

+jbudorick@ +yzjr - The inject permissions exception is usually a problem with our setup.  In this particular instance, the test is using espresso instead of TouchCommon.  Our touch utils dispatches directly to the view and does not use the instrumentation touch dispatching which is the source of these errors.

For the short term fix, we should likely get rid of espressso in this test to avoid these issues here.

Thanks for the recommendation of TouchCommon, I had no idea it was preferred to Espresso (and never heard of it before). I'll check how to switch the test.

Adding fhorschig@, who co-owns the test and authored significant part of it.

Ah, now I understand a bit more:
TouchCommon is Chromium's utility to interact with the application without injecting events.
In SavePasswordsPreferencesTest, this would mean replacing calls like

  Espresso.onView(withSearchMenuIdOrText()).perform(click());

with

  TouchCommon.singleClickView(withSearchMenuIdOrText());

Happy to give it a try.

Right, and a Matcher<View> is not exactly a View, so I'll need to do some more work for the transition. Looking at mActivityTestRule.getActivity().findViewById at the moment.

It should be quite simple to create a performable ViewAction from TouchCommon functions to reach a notation like this:

Espresso.onView(withSearchMenuIdOrText()).perform(TouchCommon.singleClickView());

This would keep readability and make for a easy transition. Naming is not final ;)
If you have an urgently failing test, you should be able to call the methods on matchers with: 

Espresso.onView(withSearchMenuIdOrText()).check((view, e) -> singleClickView(view));

Btw: Thanks for CC'ing me! There are quite some useful functions that will also improve my tests in different places!

I like your idea, Friedrich!
Among other things, keeping the Espresso matchers will preserve the test acting as an accessibility verification in addition to testing the features.

So far I have this ViewAction:

    private static class TouchCommonClick implements ViewAction {
        @NonNull
        private Matcher<View> anyView() {
            return new IsAnything<>();
        }

        @Override
        public Matcher<View> getConstraints() {
            return anyView();
        }

        @Override
        public String getDescription() {
            return "A single click with TouchCommon";
        }

        @Override
        public void perform(UiController uiController, View view) {
            TouchCommon.singleClickView(view);
        }
    }

However, where

    .perform(click());

causes the click with no problem,

    .perform(new TouchCommonClick());

just silently does nothing.
I'm trying to figure out why. One strange thing is about the view -- the matcher used with the above cited ViewActions in the test is:

    withText(R.string.save_password_preferences_export_action_title)

This is in a context of displaying the exporting menu item. The item is described by chrome/android/java/res/menu/save_password_preferences_action_bar_menu.xml as having the id "export_passwords". However, switching the matcher to

    Espresso.onView(withId(R.id.export_passwords))

and doing .check((view, e) -> TouchCommon.singleClickView(view)); at the end, an exception is thrown for view being null.

So I'm not clear on what view is actually returned.

So at least to explain the no match on the id -- the id is that of the MenuItem, which is not a View at all. There is a corresponding View which has the text defined by the MenuItem, but a different id.

Still no idea why click() works where new TouchCommonClick() does not, even if the view is matched right, though.

Looking around in code, I saw a lots of uiController.loopMainThreadUntilIdle(); in the perform() method, so I added one at the beginning and one at the end. Still no success with clicking, though.

Placing an assert in the menu handler, I could verify that the TouchCommon click results in the handler run, so perhaps I need more waiting afterwards. Will explore that...

It seems like it is not an issue of waiting:
The test fails in the next step after TouchCommonClick -- when a view with the text R.string.settings_passwords_export_description is expected to be visible, and it is not.
So I added this before the failing check:
        Espresso.onView(isRoot()).check(
                (root, e)
                        -> waitForView((ViewGroup) root,
                                withText(R.string.settings_passwords_export_description)));


This reports no matching view found, and the test noticeably pauses after the click, so apparently there is another issue preventing the next view from appearing. Will try to see what that might be.

My time for this today is running out, so I'll need to continue tomorrow. If the tests are making troubles on the bot, we can disable them, but in that case we need to disable likely all of SavePasswordsPreferencesTest, because all tests from there use Espresso's event injection.

Any hints, advice or observations highly appreciated! :)

The draft changes we used to debug are here: https://crrev.com/c/1280671

And some more details missing in my previous posts or new since debugging with Friedrich:

* TouchCommon only breaks for tests connected to password export, not to password search; these test the flow when the real Chrome passes control to the system reauthentication screen, and then resume after the screen finishes. In tests this is stubbed out, but still onResume is called on the settings preference screen.

* A specific test which we were observing is testExportMenuItem. This does the following steps:
  1. Click the three dots menu.
  2. Click the "Export password" menu item.
  3. Check that a warning alert is displayed.
In step 2., an internal state (ExportFlow.mExportState) is set to 1. Step 3. should be a result of the onResume call (made explicitly inside the test) and the ExportFlow.mExportState being 1. While debugging showed that the place when ExportFlow.mExportState is set to 1 is reached, the onResume call finds it being 0 and hence step 3. fails.

This could be either due ExportFlow.mExportState being set to 1 first, then reset to 0 before onResume, or due to onResume being called before ExportFlow.mExportState is actually set to 1. The former seems less probable, just looking at the code logic and at the fact that the test works reliably with Espresso's click() ViewAction.

Given that at least two threads participate in the test (UI and main, I understand), it seems plausible that TouchCommon's click emulation and Espresso's click() use different synchronisation patterns and as a result, the order of setting ExportFlow.mExportState to 1 and calling onResume ends up different.

My next step was to understand what Espresso's click() does in terms of synchronisation.

This is how Espresso's click() is defined [1]:

  public static ViewAction click() {
    return actionWithAssertions(
        new GeneralClickAction(Tap.SINGLE, GeneralLocation.VISIBLE_CENTER, Press.FINGER));
  }

There seems to be no synchronising in actionWithAssertions. Looking at GeneralClickAction [2], for Views which are not WebViews (our case is not a WebView), I only found the following waiting:

      int duration = ViewConfiguration.getPressedStateDuration();
      // ensures that all work enqueued to process the tap has been run.
      if (duration > 0) {
        uiController.loopMainThreadForAtLeast(duration);
      }

which itself is wrapped in a 3-retries loop. The |duration| seems to be in the order of tens of milliseconds [3].

So I think plugging loopMainThreadForAtLeast(200) into TouchCommonClick.perform is my next step tomorrow.

[1] https://android.googlesource.com/platform/frameworks/testing/+/android-support-test/espresso/core/src/main/java/android/support/test/espresso/action/ViewActions.java#158

[2] https://android.googlesource.com/platform/frameworks/testing/+/android-support-test/espresso/core/src/main/java/android/support/test/espresso/action/GeneralClickAction.java#107

[3] https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewConfiguration.java#60

So with uiController.loopMainThreadForAtLeast(200); added to the perform method this worked.

And when I say "worked", I actually mean: less tests failed. In this case the failing ones were:

org.chromium.chrome.browser.preferences.password.SavePasswordsPreferencesTest#testExportMenuItemNoLock
org.chromium.chrome.browser.preferences.password.SavePasswordsPreferencesTest#testExportRequiresReauth

The reauthenticateAndRequestExport method in the test fixture runs on the main thread and does two things:
(1) clicks the menu item
(2) synchronises with the UI thread, runs onResume on it, and verifies that the UI reflects that clicking the menu item was done

Now, for the clicking of the menu item to take effect (including changing the inner state, ExportFlow.mExportState, and dismissing the menu item), some tasks on the main thread need to run. If step (2) is done without allowing the main thread to run, then ExportFlow.mExportState will not be updated when onResume is done, and any amount of waiting for the menu item to disappear will just timeout.

I tried waiting using:
        onView(isRoot()).check(
                (root, e)
                        -> waitForView((ViewGroup) root,
                                withText(R.string.save_password_preferences_export_action_title),
                                VIEW_GONE));
on the main thread, and apparently this does not spin the main thread message loop inside.
Maybe I need to run this on the UI thread?

(Running the waitForFiew on the UI thread was a terrible idea. That got the test stuck completely.)

The plan is to have something which makes the tests run again and then argue how to make it the least ugly. So far I still cannot get all tests passing, will continue on this tomorrow.

Hi everybody, sorry for the slow progress. For various reasons I did not get much time spent on this today. I did have all tests pass already, but not very sure that it is not just flakiness.

Well, actually, I'm sure it is flakiness, because the only thing that works to ensure that menu item click is processed before onResume was calling uiController.loopMainThreadForAtLeast in the perform method. This needs a >0 timeout argument, so inherently flaky (lower number = more failures but faster test, higher = less failures but slower test). I tried also uiController.loopMainThreadUntilIdle and even InstrumentationRegistry.getInstrumentation().waitForIdleSync(), but none of that worked.

Tomorrow I will try to polish that and make sure that it is not flaky, then get that reviewed and landed, because although imperfect, it is probably still better than today's state.

Sorry for the delay, but I think we want to generally avoid the wait for main loop thread idle-ness of espresso.  I think the general "waitForView", "waitForText", "waitForBlah" is more useful than thread idle-ness.

Thanks, Ted!

I understand the desire not to idle-loop the main thread. However, so far none of the "waitFor*" attempts we made seemed to keep the tests running. It looks like we have a cascade of issues:

(1) worst = current situation, where events are flakily injected
(2) a little better = loopMainThreadForAtLeast but no event injection
(3) ideal = waitFor*

I wonder if it makes sense to go for (2) now, land that, and see if we get to (3) with some more effort?

Speaking of landing, I have less than one hour left before I will need to leave the office and I won't be able to use any computer equipped for Android development until 29 November 2018 due to travelling. So I'm unlikely to land even (2), the less (3) before that.

Should we temporarily disable the whole SavePasswordsPreferencesTest suite, or is the flaking bearable enough to keep the coverage?

Another update:
I am currently swamped with tasks I need to finish before I leave the team at the end of November. I am not sure whether and when I will be able to get back to this. Should I disable the whole SavePasswordsPreferencesTest suite?

I'm not seeing this flake in the recent builds, so I don't think we should disable this.  I think it's ok to put this on the back burner.

With the end of my time on the current team approaching, it becomes clear that I won't be able to work on this :(.
Marking as available so that it's part of the password team's TODO list.

vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.