Tests and test helpers for credentials.

Tests and test helpers for credentials.

[beaton, 2014-03-26 00:50:43]

https://codereview.appspot.com/79990043/diff/1/google-api-client-appengine/sr...
File
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java
(right):

https://codereview.appspot.com/79990043/diff/1/google-api-client-appengine/sr...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java:30:
String accessTokenText = null;
this should be private

https://codereview.appspot.com/79990043/diff/1/google-api-client/src/test/jav...
File
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java
(right):

https://codereview.appspot.com/79990043/diff/1/google-api-client/src/test/jav...
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java:46:
Map<String, String> map = new HashMap<String, String>();
Is there a dependency on the guava (google collections) library already in this
code?

There are nicer tools for this in Guava.  But it's not worth adding the
dependency.

[Anthony Moore, 2014-03-26 17:54:53]

Revised. All comments done.

https://codereview.appspot.com/79990043/diff/1/google-api-client-appengine/sr...
File
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java
(right):

https://codereview.appspot.com/79990043/diff/1/google-api-client-appengine/sr...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java:30:
String accessTokenText = null;
On 2014/03/26 00:50:44, beaton wrote:
> this should be private

Done.

https://codereview.appspot.com/79990043/diff/1/google-api-client/src/test/jav...
File
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java
(right):

https://codereview.appspot.com/79990043/diff/1/google-api-client/src/test/jav...
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java:46:
Map<String, String> map = new HashMap<String, String>();
On 2014/03/26 00:50:44, beaton wrote:
> Is there a dependency on the guava (google collections) library already in
this
> code?
> 
> There are nicer tools for this in Guava.  But it's not worth adding the
> dependency.

Done.

[beaton, 2014-03-26 18:34:09]

LGTM

https://codereview.appspot.com/79990043/diff/1/google-api-client/src/test/jav...
File
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java
(right):

https://codereview.appspot.com/79990043/diff/1/google-api-client/src/test/jav...
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java:53:
String key = URLDecoder.decode(sides[0], "UTF-8");
let's put this UTF-8 string in a static constant somewhere

[Orest, 2014-03-26 20:43:06]

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
File
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java:49:

Do we need this empty line here?

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java:67:
assertTrue(headerContainsToken);
I would call assertTrue on 'authHeader.contains(EXPECTED_ACCESS_TOKEN)', thus
avoiding creating the headerContainsToken local variable.

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java:68:

Do we need this empty line here?

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
File
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java:73:
public GetAccessTokenResult getAccessTokenUncached(Iterable<String> arg0) {
s/arg0/scopes/

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java:83:
public ParsedAppId parseFullAppId(String arg0) {
s/arg0/fullAppId/

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java:49:
public static Map<String, String> parseQuery(String query) throws IOException {
The spirit of this class is to do just testing for GoogleUtils class.

I think you may want to put your new method here:
https://code.google.com/p/google-api-java-client/source/browse/google-api-cli...
.

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredentialTest.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredentialTest.java:49:
.setJsonFactory(new JacksonFactory())
Couldn't we use JSON_FACTORY here, instead of creating another object?

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredentialTest.java:71:
credential.setRefreshToken(REFRESH_TOKEN);
Shouldn't the builder figure the refresh token out from the information you've
already provided it via the transport?

[Anthony Moore, 2014-03-26 23:29:15]

Applied all open comments.

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
File
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java:49:

On 2014/03/26 20:43:06, Orest wrote:
> Do we need this empty line here?

Done.

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java:67:
assertTrue(headerContainsToken);
On 2014/03/26 20:43:06, Orest wrote:
> I would call assertTrue on 'authHeader.contains(EXPECTED_ACCESS_TOKEN)', thus
> avoiding creating the headerContainsToken local variable.

Done.

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/AppIdentityCredentialTest.java:68:

On 2014/03/26 20:43:06, Orest wrote:
> Do we need this empty line here?

Done.

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
File
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java:73:
public GetAccessTokenResult getAccessTokenUncached(Iterable<String> arg0) {
On 2014/03/26 20:43:06, Orest wrote:
> s/arg0/scopes/

Done.

https://codereview.appspot.com/79990043/diff/20001/google-api-client-appengin...
google-api-client-appengine/src/test/java/com/google/api/client/googleapis/extensions/appengine/auth/oauth2/MockAppIdentityService.java:83:
public ParsedAppId parseFullAppId(String arg0) {
On 2014/03/26 20:43:06, Orest wrote:
> s/arg0/fullAppId/

Done.

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/GoogleUtilsTest.java:49:
public static Map<String, String> parseQuery(String query) throws IOException {
On 2014/03/26 20:43:06, Orest wrote:
> The spirit of this class is to do just testing for GoogleUtils class.
> 
> I think you may want to put your new method here:
>
https://code.google.com/p/google-api-java-client/source/browse/google-api-cli...
> .

Agreed this is the wrong place. Since it is only currently needed by test code,
we should avoid making it public production code, so I put it in a new class
TestUtils.

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredentialTest.java
(right):

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredentialTest.java:49:
.setJsonFactory(new JacksonFactory())
On 2014/03/26 20:43:06, Orest wrote:
> Couldn't we use JSON_FACTORY here, instead of creating another object?

Done.

https://codereview.appspot.com/79990043/diff/20001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/auth/oauth2/GoogleCredentialTest.java:71:
credential.setRefreshToken(REFRESH_TOKEN);
On 2014/03/26 20:43:06, Orest wrote:
> Shouldn't the builder figure the refresh token out from the information you've
> already provided it via the transport?

The one passed to the transport is to simulate the token server having the
refresh token stored in its tables, while the copy passed to the credential
simulates getting the token from a callback at the end of a 3LO flow. So it
would not really make sense for the builder to pull this out of the mock
transport.

[Orest, 2014-03-26 23:49:58]

LGTM

https://codereview.appspot.com/79990043/diff/40001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/compute/MockMetadataServerTransport.java
(right):

https://codereview.appspot.com/79990043/diff/40001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/compute/MockMetadataServerTransport.java:56:
throw new IOException("Metadata requrest header not found.");
s/requrest/request/

[Anthony Moore, 2014-03-27 00:10:16]

Applied last comment.

https://codereview.appspot.com/79990043/diff/40001/google-api-client/src/test...
File
google-api-client/src/test/java/com/google/api/client/googleapis/compute/MockMetadataServerTransport.java
(right):

https://codereview.appspot.com/79990043/diff/40001/google-api-client/src/test...
google-api-client/src/test/java/com/google/api/client/googleapis/compute/MockMetadataServerTransport.java:56:
throw new IOException("Metadata requrest header not found.");
On 2014/03/26 23:49:58, Orest wrote:
> s/requrest/request/

Done.

[beaton, 2014-03-27 01:59:05]

Please do fix the string references to "UTF-8" to use a constant defined
somewhere.  Other than that, LGTM.

[beaton, 2014-03-27 01:59:05]

Please do fix the string references to "UTF-8" to use a constant defined
somewhere.  Other than that, LGTM.

[Anthony Moore, 2014-03-27 17:53:39]

On 2014/03/27 01:59:05, beaton wrote:
> Please do fix the string references to "UTF-8" to use a constant defined
> somewhere.  Other than that, LGTM.

For this initial test checkin there are no references to UTF-8. The following
checkin has some that I'll make sure we use a constant for. Thanks.

[Anthony Moore, 2014-03-27 17:59:15]

On 2014/03/27 17:53:39, Anthony Moore wrote:
> On 2014/03/27 01:59:05, beaton wrote:
> > Please do fix the string references to "UTF-8" to use a constant defined
> > somewhere.  Other than that, LGTM.
> 
> For this initial test checkin there are no references to UTF-8. The following
> checkin has some that I'll make sure we use a constant for. Thanks.

My mistake. Fixed the remaining ones in TestUtils.

[wonderfly, 2014-04-09 18:57:22]

LGTM