New issue
Advanced search Search tips

Issue 903657 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Flaky-Test: org.chromium.chrome.browser.banners.AppBannerManagerTest#testAppInstalledModalNativeAppBannerCustomTab



Sign in to add a comment

org.chromium.chrome.browser.banners.AppBannerManagerTest#testAppInstalledModalNativeAppBannerCustomTab is flaky

Project Member Reported by Findit, Nov 9

Issue description

Labels: -Sheriff-Chromium
Owner: mdjones@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 9

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

commit 496cd36004b1f6d845e66930bbbd939de4e4f5bb
Author: Patrik Höglund <phoglund@chromium.org>
Date: Fri Nov 09 13:41:49 2018

DisabletestModalNativeAppBannerCanBeTriggeredMultipleTimesCustomTab.

It is flaky. See for instance
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/121816.

Bug:  903658 ,  903657 
Change-Id: Ic682ac60329923a35745d135d101d0eb8b61b08b
Tbr: mdjones@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/1329161
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606818}
[modify] https://crrev.com/496cd36004b1f6d845e66930bbbd939de4e4f5bb/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java

Cc: dominickn@chromium.org
Dominick, can I assign this to you? I'm unfamiliar with the app banner infobar code and it looks like you may have worked on it some.
I think the test should be re-enabled.

The failure in the test log is as follows:

C  883.947s Main  [FAILURE] org.chromium.chrome.browser.banners.AppBannerManagerTest#testAppInstalledModalNativeAppBannerCustomTab:
C  883.947s Main  java.lang.AssertionError
C  883.947s Main  	at org.chromium.chrome.browser.signin.SigninManager.signOut(SigninManager.java:545)
C  883.947s Main  	at org.chromium.chrome.browser.signin.SigninManager.signOut(SigninManager.java:528)
C  883.947s Main  	at org.chromium.chrome.browser.signin.SigninHelper.handleAccountRename(SigninHelper.java:200)
C  883.947s Main  	at org.chromium.chrome.browser.signin.SigninHelper.validateAccountSettings(SigninHelper.java:136)
C  883.947s Main  	at org.chromium.chrome.browser.signin.SigninHelper$1.onPostExecute$a83c79c(SigninHelper.java:158)
C  883.947s Main  	at org.chromium.chrome.browser.signin.SigninHelper$1.onPostExecute(SigninHelper.java:145)
C  883.947s Main  	at org.chromium.base.task.AsyncTask.finish(AsyncTask.java:340)
C  883.947s Main  	at org.chromium.base.task.AsyncTask.lambda$postResult$0$AsyncTask(AsyncTask.java:122)
C  883.947s Main  	at org.chromium.base.task.AsyncTask$$Lambda$0.run(AsyncTask.java)
C  883.947s Main  	at android.os.Handler.handleCallback(Handler.java:733)
C  883.947s Main  	at android.os.Handler.dispatchMessage(Handler.java:95)
C  883.947s Main  	at android.os.Looper.loop(Looper.java:136)
C  883.947s Main  	at android.app.ActivityThread.main(ActivityThread.java:5001)
C  883.948s Main  	at java.lang.reflect.Method.invokeNative(Method.java)
C  883.948s Main  	at java.lang.reflect.Method.invoke(Method.java:515)
C  883.948s Main  	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785)
C  883.948s Main  	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601)
C  883.948s Main  	at dalvik.system.NativeStart.main(NativeStart.java)
C  883.948s Main  


That pretty clearly looks like something else broke signin manager and caused the failure in multiple tests, and this test should not have been disabled. WDYT?
Cc: mdjones@chromium.org
Owner: chcunningham@chromium.org
+chcunningham for signin manager.
Owner: bsazonov@chromium.org
See my comment on https://bugs.chromium.org/p/chromium/issues/detail?id=906188#c5
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 23

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

commit f86778e89d9d0ab4284a700e9abe28fb5f82f22e
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Fri Nov 23 16:35:39 2018

[Signin][Android] Wait for ongoing SigninManager operations in SigninHelper

This CL adds isOperationInProgress and runAfterOperationInProgress methods
to SigninManager and uses these methods in SigninHelper.validateAccounts.
SigninHelper has been causing many test failures recently because it
attempts to start resign-in before previous sign-out is finished.
Tests for isOperationInProgress and runAfterOperationInProgress will be
added in subsequent CLs, as they require significant amount of changes
around SigninManager (it's not quite testable at the moment).

Bug:  903657 ,  903658 , 906188, 906193, 906788
Change-Id: I216e16874adef6a050df4796eab4eb55ddf6fa6c
Reviewed-on: https://chromium-review.googlesource.com/c/1345069
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610632}
[modify] https://crrev.com/f86778e89d9d0ab4284a700e9abe28fb5f82f22e/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java
[modify] https://crrev.com/f86778e89d9d0ab4284a700e9abe28fb5f82f22e/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 30

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

commit 489cf13fb7bb36075efff2edc2737557279f8d81
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Fri Nov 30 14:07:18 2018

[Android] Mock AndroidSyncSettings before starting activity in SyncTestRule

This CL reorders calls in SyncTestRule so AndroidSyncSettings instance
is overridden before activity is started. This is done to prevent
classes initialized during activity initialization from grabbing
a reference to the default AndroidSyncSettings.

Bug:  903657 ,  903658 , 906188, 906193, 906788
Change-Id: Ib412d7748a8ec6f48b80436051a932d6b7713940
Reviewed-on: https://chromium-review.googlesource.com/c/1356583
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612637}
[modify] https://crrev.com/489cf13fb7bb36075efff2edc2737557279f8d81/chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTestRule.java

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 3

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

commit 5d23d73c8925b1489e5f2478c60416ee7232edb6
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Mon Dec 03 12:24:37 2018

Enable DisabletestModalNativeAppBannerCanBeTriggeredMultipleTimesCustomTab

This CL reverts half of https://crrev.com/c/1329161 by enabling
testModalNativeAppBannerCanBeTriggeredMultipleTimesCustomTab
that was disabled by the CL. The issue that was causing the failures
should've been fixed by https://crrev.com/c/1345069.
testAppInstalledModalNativeAppBannerCustomTab, the second test disabled
by https://crrev.com/c/1329161 is not enabled by this CL, as it fails
with TimeoutException (doesn't seem to be connected to the failure fixed
by https://crrev.com/c/1345069).

Bug:  903657 
Change-Id: I4ef52c1bfe9d560fecbcfa4e389baffef758fe15
Reviewed-on: https://chromium-review.googlesource.com/c/1350187
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613057}
[modify] https://crrev.com/5d23d73c8925b1489e5f2478c60416ee7232edb6/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 7

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

commit 958e6783640e39cc1f5aa49cd17e21037e4bbb31
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Fri Dec 07 12:00:44 2018

[Signin][Android] Add tests for SigninManager.runAfterOperationInProgress

This CL is a follow-up to https://crrev.com/c/1345069, which added
isOperationInProgress and runAfterOperationInProgress methods to
SigninManager. This CL modifies SigninManager to make it testable (by
making some methods non-static and adding a constructor with explicit
dependencies) and adds tests for the new methods.

Bug:  903657 ,  903658 , 906188, 906193, 906788
Change-Id: Iba91ca038898d7abd015b4ab194bc26edbe34479
Reviewed-on: https://chromium-review.googlesource.com/c/1350908
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614673}
[modify] https://crrev.com/958e6783640e39cc1f5aa49cd17e21037e4bbb31/chrome/android/java/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachine.java
[modify] https://crrev.com/958e6783640e39cc1f5aa49cd17e21037e4bbb31/chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java
[modify] https://crrev.com/958e6783640e39cc1f5aa49cd17e21037e4bbb31/chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerTest.java
[modify] https://crrev.com/958e6783640e39cc1f5aa49cd17e21037e4bbb31/chrome/browser/android/signin/signin_manager_android.cc

Sign in to add a comment