Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1258563005:
Added Signin Utilities (Closed)
|
Created:
5 years, 5 months ago by Sean Kirmani Modified:
5 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTools for being able to sign into fake OS accounts and fake app accounts
when running tests. Real OS accounts are also supported, but take
significantly longer because accounts must be authenticated. It is
recommended to use fake OS accounts unless real ones are necessary.
CQ-DEPEND=CL:1259253007
Committed: https://crrev.com/6832765af577d32c774763e5abe8486aa8db7478
Cr-Commit-Position: refs/heads/master@{#342190}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Changed GaiaOS tests to Enormous #Patch Set 3 : #Patch Set 4 : Added Sync Feature to Gaia Tests #Patch Set 5 : Added Wifi Restriction to Gaia Tests #Patch Set 6 : Removed static import #Patch Set 7 : Renamed Restriction from WIFI to INTERNET #Patch Set 8 : Removed Explicit AccountManager calls #Patch Set 9 : Bug Fixes #Patch Set 10 : Bug Fixes #2 #
Total comments: 10
Patch Set 11 : Addressed John's comments #
Total comments: 10
Patch Set 12 : Addressed John's Comments #2 #
Total comments: 7
Patch Set 13 : Addressed John's Comments #3 #
Total comments: 5
Patch Set 14 : Used MockAccountManager #Patch Set 15 : Test fixes #
Total comments: 5
Patch Set 16 : Addressed John's Comments #4 #
Total comments: 4
Patch Set 17 : Addressed John's Comments #5 #
Total comments: 8
Patch Set 18 : Removed Nulls #
Total comments: 6
Patch Set 19 : Made ChromeSigninUtilsTest extend InstrumentationTestCase #Patch Set 20 : Replaced Context with Target Context #Patch Set 21 : #
Messages
Total messages: 33 (5 generated)
skirmani@google.com changed reviewers: + jbudorick@chromium.org
SigninUtils now ready for separate review than the parameters.
skirmani@google.com changed reviewers: + mikecase@chromium.org
Made tests that require Wi-Fi EnormousTest's.
Ready for review!
https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java (right): https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:45: Intent intent = new Intent(mContext, null /* AddAccountActivity.class */); AddAccountActivity.class? https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:70: String authToken = null; nit: just declare authToken below. https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:72: Bundle result = new Bundle(); nit: similarly, just declare this down where you start using it. https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountConstants.java (right): https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountConstants.java:8: * Holds constants that are needed by the AccountAuthenticator. Why aren't these just in SigninUtils? https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java:24: * A tool used for running tests as signed in. Currently this only works for instrumentation tests Not sure this comment is entirely accurate any more. https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java:27: public class SigninUtils { nit: maybe "ChromeSigninUtils" if this is chrome-specific. https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java:45: ChromeSigninController.get(mTargetContext).clearSignedInUser(); I don't really like these two functions that just forward a call to ChromeSigninController. https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java:136: private static final Object AUTHENTICATION_COMPLETION_LOCK = new Object(); This should not be static. https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java:146: public Bundle waitForAuthCompletion() { We're using a future down in run. Could we do this auth flow completion wait with a future? https://codereview.chromium.org/1258563005/diff/180001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/SigninUtils.java:217: // If the account already exists, return false uh...
Addressed your comments! Please take another PTAL.
https://codereview.chromium.org/1258563005/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java (right): https://codereview.chromium.org/1258563005/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:74: public void testIsSignedInOnGaiaOS() { These should still have a size annotation. https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java (right): https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:19: * Authenticators {@link Service} for test accounts. Allows adding fake accounts without password nit: Authenticator https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:36: public AccountAuthenticator(Context mContext) { nit: mContext -> context https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:74: authToken = "myToken"; Make this a constant at class scope. https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/services/accountauthenticator/AccountAuthenticatorService.java:111: return "AUTH_FAILS".equals(accountManager.getPassword(account)); constant at class scope https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:45: public ChromeSigninUtils(BaseActivityInstrumentationTestCase testCase) { Does this need to be a BaseActivityInstrumentationTestCase if you're just getting the Instrumentation? Couldn't it be an InstrumentationTestCase? https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:46: mTargetContext = testCase.getInstrumentation().getTargetContext(); I'm not sure this is the right context. ChromeSigninController.get says it wants the application context: https://code.google.com/p/chromium/codesearch#chromium/src/sync/android/java/... https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:102: * Adds a Gaia account to the OS. @params https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:104: public void addGaiaAccountToOs(String username, String password, String type) { What type is this? The other functions in this class with a |type| param take either FAKE_ACCOUNT_TYPE or GOOGLE_ACCOUNT_TYPE, but that doesn't appear to be the case here. https://codereview.chromium.org/1258563005/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:208: public void addAccountToOs(String username, String password, String type) { Should this be private?
Addressed your comments! Here are my changes: - Added missing test size to SigninUtilsTest - Fixed nits in AccountAuthenticatorService - Made strings class constants in AccountAuthenticatorService - Made addAccountToOs in ChromeSigninUtils private - Added params documentation to addGaiaAccountToOs - Renamed type param in addGaiaAccountToOs to gaiaAccountType for clarification - Made BaseActivityInstrumentationTestCase an InstrumentationTestCase - Used getContext() instead of getTargetContext() in ChromeSigninUtils PTAL!
https://codereview.chromium.org/1258563005/diff/220001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1258563005/diff/220001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:17: android:sharedUserId="org.chromium.chrome" > What's up with this? https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. I would remove references to "gaia" and just replace them with "google" (with corrections for phrasing) https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:47: mAccountManager = AccountManager.get(testCase.getInstrumentation().getContext()); nit: AccountManager.get(mContext) as long as it stays the same. https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:54: ChromeSigninController.get(mContext).clearSignedInUser(); Again, I'm not sure this is the right context. I think you want this: http://developer.android.com/reference/android/content/Context.html#getApplic... https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:62: public boolean isExistingAccountOnApp() { Still not crazy about the two functions that just forward to ChromeSigninController. https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:226: if (TextUtils.isEmpty(username)) { Why check here and not when adding an account? Be consistent. https://codereview.chromium.org/1258563005/diff/220001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:231: if (acct.name.equals(username) && acct.type.equals(type)) { Does AccountManager.getAccountsByType return some accounts that _aren't_ the given type...? Otherwise, we shouldn't need the second half of this if condition.
- Replaced all instances of Gaia with Google - Removed forward methods to ChromeSigninController - Removed unnecessary account type check - Fixed other various nits PTAL
https://codereview.chromium.org/1258563005/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java (right): https://codereview.chromium.org/1258563005/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:25: private static final String REAL_OS_ACCOUNT_USERNAME = "chromiumforandroid01@gmail.com"; I'm not sure about handling a real account this way, but I'm also not sure how else to do it. https://codereview.chromium.org/1258563005/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:44: assertEquals("Should not be signed into app.", false, Instead of assertEquals(..., boolean, condition), use assertFalse and assertTrue https://codereview.chromium.org/1258563005/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:89: mSigninUtil.removeAllGoogleAccountsFromOs(); The account reset logic should all be in tearDown, not in the test. https://codereview.chromium.org/1258563005/diff/240001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/240001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:85: public boolean isExistingFakeAccountOnOs() { Why are we with checking for just _an_ account instead of a specific account? We allow a client to add a specific account; let's allow them to query a specific account, too.
Implemented Fake OS Signin with MockAccountManager
- Removed redundant private methods in ChromeSigninUtils
- Removed AccountAuthenticatorService
- Removed javatest resources
- Reverted both AndroidManifests
PTAL
- Moved tear down logic to tearDown. - Changed assertEquals to assertTrue and assertFalse where needed. PTAL
https://codereview.chromium.org/1258563005/diff/240001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/240001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:85: public boolean isExistingFakeAccountOnOs() { On 2015/08/04 at 21:43:37, jbudorick wrote: > Why are we with checking for just _an_ account instead of a specific account? We allow a client to add a specific account; let's allow them to query a specific account, too. still this https://codereview.chromium.org/1258563005/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java (right): https://codereview.chromium.org/1258563005/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: This should be in a different directory... probably chrome/android/javatests/src/org/chromium/chrome/test/util https://codereview.chromium.org/1258563005/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:17: * Tester class for {@link ChromeSigninUtils}. nit: "Tester" -> "Tests" https://codereview.chromium.org/1258563005/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:19: * Internet is needed for running the Google account tests. Nix this. https://codereview.chromium.org/1258563005/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java:29: private Context mContext; nit: line break before this https://codereview.chromium.org/1258563005/diff/280001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/280001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:50: mMockAccountManager = null; Why are you delaying construction of the MockAccountManager? It makes the code below more convoluted for very little gain.
Patch Set #16: Addressed John's Comments #4
- Moved SigninUtilsTest from
chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninUtilsTest.java
->
chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java
- No longer delaying construction of MockAccountManager
- isExistingAccountOnOs now queries specific account
- Nit fix: Changed Tester to Tests
- Nit fix: Removed Internet is needed warning
- Nit fix: Added space between static and other global variables in
ChromeSigninUtilsTest
- Renamed p.s.f. variables in ChromeSigninUtilsTest
PTAL
https://codereview.chromium.org/1258563005/diff/300001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/300001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:103: public boolean isExistingFakeAccountOnOs(String username) { nit: @param https://codereview.chromium.org/1258563005/diff/300001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:147: public boolean isExistingGoogleAccountOnOs() { Again, this should be a specific account check. Make sure you include the @param.
https://codereview.chromium.org/1258563005/diff/300001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/300001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:103: public boolean isExistingFakeAccountOnOs(String username) { On 2015/08/05 17:54:02, jbudorick wrote: > nit: @param Done. https://codereview.chromium.org/1258563005/diff/300001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:147: public boolean isExistingGoogleAccountOnOs() { On 2015/08/05 17:54:02, jbudorick wrote: > Again, this should be a specific account check. Make sure you include the > @param. Done.
https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:89: if (mMockAccountManager != null) { This null check is no longer necessary. https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:94: mMockAccountManager = null; Don't release mMockAccountManager here. https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:105: if (mMockAccountManager == null Again, null check no longer necessary. https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:150: if (mAccountManager == null null check
https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:89: if (mMockAccountManager != null) { On 2015/08/05 19:54:09, jbudorick wrote: > This null check is no longer necessary. Done. https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:94: mMockAccountManager = null; On 2015/08/05 19:54:09, jbudorick wrote: > Don't release mMockAccountManager here. Done. https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:105: if (mMockAccountManager == null On 2015/08/05 19:54:09, jbudorick wrote: > Again, null check no longer necessary. Done. https://codereview.chromium.org/1258563005/diff/320001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:150: if (mAccountManager == null On 2015/08/05 19:54:09, jbudorick wrote: > null check Done.
jbudorick@chromium.org changed reviewers: + yfriedman@chromium.org
lgtm w/ nits +yfriedman for owners https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:92: new AccountHolder.Builder().account(acct).build(), true); nit: indentation is off https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:136: authResult.get(AccountManager.KEY_ERROR_MESSAGE))); nit: indentation is off.
https://codereview.chromium.org/1258563005/diff/340001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java (right): https://codereview.chromium.org/1258563005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java:19: public class ChromeSigninUtilsTest extends ChromeActivityTestCaseBase<ChromeActivity> { Is InstrumentationTestCase not sufficient? It looks like this code only depends on ChromeSigninController which is pure java/android dependencies and no native / app
https://codereview.chromium.org/1258563005/diff/340001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java (right): https://codereview.chromium.org/1258563005/diff/340001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java:19: public class ChromeSigninUtilsTest extends ChromeActivityTestCaseBase<ChromeActivity> { Yup, it can! Fixed. https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:92: new AccountHolder.Builder().account(acct).build(), true); On 2015/08/05 21:11:30, jbudorick wrote: > nit: indentation is off Done. https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:136: authResult.get(AccountManager.KEY_ERROR_MESSAGE))); On 2015/08/05 21:11:30, jbudorick wrote: > nit: indentation is off. Done.
On 2015/08/05 22:13:58, Sean Kirmani wrote: > https://codereview.chromium.org/1258563005/diff/340001/chrome/android/javates... > File > chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java > (right): > > https://codereview.chromium.org/1258563005/diff/340001/chrome/android/javates... > chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java:19: > public class ChromeSigninUtilsTest extends > ChromeActivityTestCaseBase<ChromeActivity> { > Yup, it can! Fixed. > > https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... > File > chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java > (right): > > https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:92: > new AccountHolder.Builder().account(acct).build(), true); > On 2015/08/05 21:11:30, jbudorick wrote: > > nit: indentation is off > > Done. > > https://codereview.chromium.org/1258563005/diff/340001/chrome/test/android/ja... > chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:136: > authResult.get(AccountManager.KEY_ERROR_MESSAGE))); > On 2015/08/05 21:11:30, jbudorick wrote: > > nit: indentation is off. > > Done. lgtm
lgtm https://codereview.chromium.org/1258563005/diff/1/chrome/android/javatests/re... File chrome/android/javatests/res/values/strings.xml (right): https://codereview.chromium.org/1258563005/diff/1/chrome/android/javatests/re... chrome/android/javatests/res/values/strings.xml:4: </resources> nit: Looks like there is an extra space at the beginning of this line.
The CQ bit was checked by skirmani@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1258563005/#ps390001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258563005/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258563005/390001
Message was sent while issue was closed.
Committed patchset #21 (id:390001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/6832765af577d32c774763e5abe8486aa8db7478 Cr-Commit-Position: refs/heads/master@{#342190}
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:390001) has been created in https://codereview.chromium.org/1270423006/ by mvanouwerkerk@chromium.org. The reason for reverting is: Tests are failing: https://code.google.com/p/chromium/issues/detail?id=517849. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
