Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288643003:
Added Signin Parameters (Closed)
|
Created:
5 years, 4 months ago by Sean Kirmani Modified:
5 years, 3 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. |
DescriptionAdded Signin Parameters
Added three parameters that can be used with ParameterizedTests:
- AddFakeAccountToAppParameter
- AddFakeAccountToOsParameter
- AddGoogleAccountToOsParameter
Other fixes:
- Made ChromeSigninUtils construct with Instrumentation instead of InstrumentationTestCase
- Added test size to restricted tests in ChromeSigninUtilsTest
BUG=
Committed: https://crrev.com/3ede545b2d23ba5caf89f741f001b02fb5952312
Cr-Commit-Position: refs/heads/master@{#344672}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Used EnormousTest and other nits #
Total comments: 6
Patch Set 3 : Moved getAvailableParameter to BaseActivityInstrumentationTestCase and other nits #
Total comments: 6
Patch Set 4 : Nits #
Total comments: 10
Patch Set 5 : rebase and nits #
Total comments: 4
Patch Set 6 : Sami's LGTM Nits #
Total comments: 2
Messages
Total messages: 44 (20 generated)
skirmani@google.com changed reviewers: + jbudorick@chromium.org
Ready for review!
https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java:71: @MediumTest Switch to @EnormousTest for now. We need to rework the bot configurations :( https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/test/util/parameters/SigninParametersTest.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/test/util/parameters/SigninParametersTest.java:117: @Parameter(tag = AddFakeAccountToAppParameter.PARAMETER_TAG), Do these work if the arguments are positional instead of keyworded? https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java:20: static final class ARGUMENT { package-private doesn't seem like the right visibility here. Maybe public? https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java:17: /** Adds a Gaia account to OS to run test as signed into the OS. */ nit: google account https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java:20: static final class ARGUMENT { same thing re visibility
PTAL! https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtilsTest.java:71: @MediumTest On 2015/08/13 19:45:24, jbudorick wrote: > Switch to @EnormousTest for now. We need to rework the bot configurations :( Done. https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/test/util/parameters/SigninParametersTest.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/test/util/parameters/SigninParametersTest.java:117: @Parameter(tag = AddFakeAccountToAppParameter.PARAMETER_TAG), On 2015/08/13 19:45:24, jbudorick wrote: > Do these work if the arguments are positional instead of keyworded? Sadly, no. :( https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java:20: static final class ARGUMENT { On 2015/08/13 19:45:24, jbudorick wrote: > package-private doesn't seem like the right visibility here. Maybe public? Done. https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java:17: /** Adds a Gaia account to OS to run test as signed into the OS. */ On 2015/08/13 19:45:24, jbudorick wrote: > nit: google account Done. https://codereview.chromium.org/1288643003/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java:20: static final class ARGUMENT { On 2015/08/13 19:45:24, jbudorick wrote: > same thing re visibility Done.
https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:123: private HashMap<String, BaseParameter> mParameters; Why are we storing these here? This seems inconsistent with how we handle available parameters in BaseActivityInstrumentationTestCase. https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:44: * @param instrumentation the {@link Instrumentation} to perform signin operations with. nit: I think this needs to be @link android.app.Instrumentation.
https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:857: protected <T extends BaseParameter> T getAvailableParameter(String parameterTag) { Something else to consider: if this is useful here, should this be part of the Parameterizable interface & implemented in BaseActivityInstrumentationTestCase instead?
PTAL very very soon! https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:123: private HashMap<String, BaseParameter> mParameters; On 2015/08/17 18:32:07, jbudorick wrote: > Why are we storing these here? This seems inconsistent with how we handle > available parameters in BaseActivityInstrumentationTestCase. Moved to Base. https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:857: protected <T extends BaseParameter> T getAvailableParameter(String parameterTag) { On 2015/08/17 18:53:58, jbudorick wrote: > Something else to consider: if this is useful here, should this be part of the > Parameterizable interface & implemented in BaseActivityInstrumentationTestCase > instead? Done. https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java (right): https://codereview.chromium.org/1288643003/diff/20001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeSigninUtils.java:44: * @param instrumentation the {@link Instrumentation} to perform signin operations with. On 2015/08/17 18:32:07, jbudorick wrote: > nit: I think this needs to be @link android.app.Instrumentation. Done.
https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:58: public <T extends BaseParameter> T getAvailableParameter(String parameterTag) { javadoc https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:15: <T extends BaseParameter> T getAvailableParameter(String parameterTag); Can/should ParameterizedTestAnnotationTest use this? https://codereview.chromium.org/1288643003/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java (right): https://codereview.chromium.org/1288643003/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java:36: .get(instrumentation.getTargetContext()); nit: this should fit on one line
PTAL! https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:58: public <T extends BaseParameter> T getAvailableParameter(String parameterTag) { On 2015/08/18 22:16:18, jbudorick wrote: > javadoc Done. https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1288643003/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:15: <T extends BaseParameter> T getAvailableParameter(String parameterTag); On 2015/08/18 22:16:18, jbudorick wrote: > Can/should ParameterizedTestAnnotationTest use this? There's not anything in ParameterizedTestAnnotationTest that needs this because that's mostly testing the ParameterReader for reading the MethodParameter. Technically, the ParameterReader can read any parameter, but MethodParameter exists so we can add arguments to read without having any extra setUp or tearDown. https://codereview.chromium.org/1288643003/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java (right): https://codereview.chromium.org/1288643003/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java:36: .get(instrumentation.getTargetContext()); On 2015/08/18 22:16:18, jbudorick wrote: > nit: this should fit on one line Done.
lgtm w/ nits https://codereview.chromium.org/1288643003/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1288643003/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:26: private Map<String, BaseParameter> mParameters; nit: mAvailableParameters https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:843: Map<String, BaseParameter> parameters = super.createAvailableParameters(); nit: availableParameters https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java:21: /** The {@Parameter.Argument.name()} value. */ nit: should this be @link Parameter.Argument.name()? https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java:20: /** The {@Parameter.Argument.name()} value. */ nit: same https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java:20: /** The {@Parameter.Argument.name()} value. */ nit: same
https://codereview.chromium.org/1288643003/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1288643003/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:26: private Map<String, BaseParameter> mParameters; On 2015/08/20 00:55:17, jbudorick wrote: > nit: mAvailableParameters Done. https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:843: Map<String, BaseParameter> parameters = super.createAvailableParameters(); On 2015/08/20 00:55:17, jbudorick wrote: > nit: availableParameters Done. https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java:21: /** The {@Parameter.Argument.name()} value. */ On 2015/08/20 00:55:17, jbudorick wrote: > nit: should this be @link Parameter.Argument.name()? Done. https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToOsParameter.java:20: /** The {@Parameter.Argument.name()} value. */ On 2015/08/20 00:55:17, jbudorick wrote: > nit: same Done. https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java (right): https://codereview.chromium.org/1288643003/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddGoogleAccountToOsParameter.java:20: /** The {@Parameter.Argument.name()} value. */ On 2015/08/20 00:55:17, jbudorick wrote: > nit: same Done.
skirmani@google.com changed reviewers: + nyquist@chromium.org
+nyquist for owner's review!
skirmani@google.com changed reviewers: + feng@chromium.org, skyostil@chromium.org - jbudorick@chromium.org, nyquist@chromium.org
+ Added skyostil, feng for OWNERS review since nyquist is OOO until I'm gone
skirmani@google.com changed reviewers: + nyquist@chromium.org, thestig@chromium.org - feng@chromium.org
skirmani@google.com changed reviewers: - thestig@chromium.org
skirmani@google.com changed reviewers: + thestig@chromium.org
skirmani@google.com changed reviewers: - nyquist@chromium.org, skyostil@chromium.org
+ Added thestig for owners review!
skirmani@google.com changed reviewers: + nyquist@chromium.org
skirmani@google.com changed reviewers: + skyostil@chromium.org
The CQ bit was checked by skirmani@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288643003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288643003/80001
The CQ bit was unchecked by skirmani@google.com
lgtm with a couple of nits. https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:50: * Creates the {@link Map} of available parameters that inherited classes can use. nit: s/Creates/Gets/? https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:22: * Creates the {@link Map} of available parameters that inherited classes can use. This comment looks wrong too.
All addressed. Thanks! https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java (right): https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseActivityInstrumentationTestCase.java:50: * Creates the {@link Map} of available parameters that inherited classes can use. On 2015/08/20 16:56:10, Sami wrote: > nit: s/Creates/Gets/? Done. https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java (right): https://codereview.chromium.org/1288643003/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/util/parameter/Parameterizable.java:22: * Creates the {@link Map} of available parameters that inherited classes can use. On 2015/08/20 16:56:10, Sami wrote: > This comment looks wrong too. Done.
The CQ bit was checked by skirmani@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288643003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288643003/100001
skirmani@google.com changed reviewers: + mikecase@chromium.org
The CQ bit was unchecked by skirmani@google.com
lgtm https://codereview.chromium.org/1288643003/diff/100001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java (right): https://codereview.chromium.org/1288643003/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java:43: mSigninController.clearSignedInUser(); This only affects Java tests I believe? Maybe add a comment about that?
On 2015/08/20 16:22:18, Sean Kirmani wrote: > + Added thestig for owners review! I think you have all the owners reviews you need already. Click the commit box!
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, skyostil@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1288643003/#ps120001 (title: "Tommy's lgtm nit (Sean's Final Patch as an Intern)")
http://i.ytimg.com/vi/YBCcEtdiWUM/hqdefault.jpg https://codereview.chromium.org/1288643003/diff/100001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java (right): https://codereview.chromium.org/1288643003/diff/100001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/parameters/AddFakeAccountToAppParameter.java:43: mSigninController.clearSignedInUser(); On 2015/08/20 18:37:54, nyquist (OOO - back 8-24) wrote: > This only affects Java tests I believe? Maybe add a comment about that? Done.
The CQ bit was unchecked by skirmani@google.com
The CQ bit was checked by skirmani@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288643003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288643003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3ede545b2d23ba5caf89f741f001b02fb5952312 Cr-Commit-Position: refs/heads/master@{#344672}
Message was sent while issue was closed.
boliu@chromium.org changed reviewers: + boliu@chromium.org
Message was sent while issue was closed.
SigninParametersTest#testIsSignedInOnApp/testIsSignedInOnFakeOSandApp have never passed in a bunch of bots downstream :( Considering everything else is ok, guess should just diable them
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted |
