Adding the AccessTokenFetcher and Environment class to the app remoting test

remoting/test/app_remoting_test_driver_environment.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_TEST_APP_REMOTING_TEST_DRIVER_ENVIRONMENT_H_
+#define REMOTING_TEST_APP_REMOTING_TEST_DRIVER_ENVIRONMENT_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "remoting/test/access_token_fetcher.h"
+#include "remoting/test/refresh_token_storage.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+class RunLoop;
+}
+
+namespace remoting {
+namespace test {
+
+// This class is globally accessible to all test fixtures and cases and has its
+// lifetime managed by the GTest framework.  It is responsible for managing
+// access tokens and caching remote host connection information.

[Wez, 2015/02/13 03:01:54]

As with other class comments, start with what the class is for, then describe
properties such as ownership. e.g. "Manages access tokens and remote host
connection details across tests. ..."

I don't see any actual remote host connection information stored in this class,
but perhaps I'm being dopey?

[joedow, 2015/02/14 02:31:27]

Fixed the comment.  The remote host connection info is added in my next change,
I have a TODO at the bottom of the comment to track it : )

+// TODO(joedow) Add remote host connection functionality.
+class AppRemotingTestDriverEnvironment : public testing::Environment {
+ public:
+  AppRemotingTestDriverEnvironment(
+      const std::string& user_name,
+      const std::string& service_environment);
+  ~AppRemotingTestDriverEnvironment() override;
+
+  // Sets up the underlying objects and state for this class.  Returns false if
+  // a valid access token cannot be retrieved.

[Wez, 2015/02/13 03:01:54]

The first part of the comment is pretty redundant; only the statement re return
code is non-obvious

[joedow, 2015/02/14 02:31:28]

Done.

+  bool Initialize(const std::string& auth_code);
+
+  // Uses the existing |refresh_token_| to request a new access token.
+  // This call blocks until either the token is retrieved or the call fails.

[Wez, 2015/02/13 03:01:54]

You can shorten this to e.g. "Synchronously request a new access token using
|refresh_token_|."

[joedow, 2015/02/14 02:31:28]

Done.

+  // Returns true if a valid access token has been retrieved, otherwise false.

[Wez, 2015/02/13 03:01:54]

As per style guide, if a method has a bool return value, document one of the
possible result,  but not the other - the "otherwise false" is implicit!

[joedow, 2015/02/14 02:31:28]

Done.

+  bool RefreshAccessToken();
+
+  // Used for service call authentication.

[Wez, 2015/02/13 03:01:53]

If this and the following accessor are used by tests then suggest bundling them
together with a comment "Accessors for fields used by tests."

[joedow, 2015/02/14 02:31:28]

Done.

+  const std::string& access_token() const { return access_token_; }
+
+  // The user name to use for authentication.
+  const std::string& user_name() const { return user_name_; }
+
+  // Used to set a mock version of the fetcher for testing.  Calling this method
+  // transfers ownership of the fetcher's lifetime to this object.
+  void SetAccessTokenFetcherForTesting(

[Wez, 2015/02/13 03:01:53]

Why is this and the function below "ForTesting"? This entire class is for use by
tests, surely...?

If you really need these tagging for test use only then the suffix is "ForTest"
not "ForTesting".

[joedow, 2015/02/14 02:31:28]

It is true that the class is used in a test tool, but I wanted to call out the
fact that this method is meant for testing the class itself, not for use in a
tool or other framework.  I'd like to keep the distinction so I'll shorten it.

[Wez, 2015/02/19 22:00:22]

Acknowledged.

+      remoting::test::AccessTokenFetcher* mock_fetcher);
+
+  // Used to set a mock version of the refresh token storage object for testing.
+  // Calling this method transfers ownership to this object.
+  void SetRefreshTokenStorageForTesting(
+      remoting::test::RefreshTokenStorageInterface* mock_refresh_token_storage);

[Wez, 2015/02/13 03:01:53]

I'd recommend not mixing accessor methods and ones that done work, e.g. put all
the accessor methods after the work methods.

[joedow, 2015/02/14 02:31:28]

Done.

+
+ private:
+  // Environment interface methods.

[Wez, 2015/02/13 03:01:54]

nit: drop "methods"; that's implicit, since they are both methods. :)

[joedow, 2015/02/14 02:31:28]

Done.

+  void SetUp() override;
+  void TearDown() override;
+
+  // Used to retrieve an access token via either an auth code or refresh token.

[Wez, 2015/02/13 03:01:53]

The parameter is called |auth_token|.. so presumably it only does the former?

[joedow, 2015/02/14 02:31:27]

Done.

+  // Returns true if a new, valid access token has been retrieved, otherwise
+  // false.

[Wez, 2015/02/13 03:01:53]

See above re true/false in comments.

[joedow, 2015/02/14 02:31:28]

Done.

+  bool RetrieveAccessToken(const std::string& auth_code);
+
+  // Called back after the access token fetcher completes its network requests.

[Wez, 2015/02/13 03:01:54]

nit: No need for "back" nor for "its network requests".

[joedow, 2015/02/14 02:31:28]

Done.

+  // On success the tokens will be valid strings, if a failure occurs the
+  // access token will be empty, the refresh token may be valid

[Wez, 2015/02/13 03:01:53]

See elsewhere re it being weird that refresh token may not be empty!

You can express this more concisely as "The token strings will be empty on
failure."

nit: Since you're returning const std::string& these will _always_ be valid
strings; they might be empty ones, though.

[joedow, 2015/02/14 02:31:28]

Done.

+  void OnAccessTokenRetrieved(
+      const std::string& access_token,
+      const std::string& refresh_token);
+
+  // Used for service call authentication.

[Wez, 2015/02/13 03:01:54]

What is "service call authentication"?  Do you mean "Stored for used by tests to
authenticate to the service."?

[joedow, 2015/02/14 02:31:28]

Done.

+  std::string access_token_;
+
+  // This token is used to retrieve a short lived access token.

[Wez, 2015/02/13 03:01:53]

No need for "This token is" nor "short lived"; the way this code is structured
you don't care that the access token is short lived, I don't think?

[joedow, 2015/02/14 02:31:28]

Done.

+  std::string refresh_token_;
+
+  // The user name to use for authentication.

[Wez, 2015/02/13 03:01:54]

nit: Drop "the"

[joedow, 2015/02/14 02:31:28]

Done.

+  std::string user_name_;
+
+  // This string indicates which service API to target when retrieving remote
+  // host connection information.

[Wez, 2015/02/13 03:01:53]

Drop "This string indicates which"

In general parameters like this should be expressed as enums so that it's clear
(and compiler-checked) that they only have one of a specific set of values.

[joedow, 2015/02/14 02:31:28]

I'll update this to an enum in my next CL, this was added in preparation for
that change, but I think the enum should live in one of the remote_host* files. 
Comment fixed, TODO added.

+  std::string service_environment_;
+
+  // Indicates that Initialize() has been called and the underlying objects are
+  // in a valid state.
+  bool initialized_;

[Wez, 2015/02/13 03:01:53]

It looks like the only "underlying objects" that matter here are the
|refresh_token_| or |refresh_token_storage_|? Could you instead of a member just
have an accessor method initialized() that tests whether the relevant field is
valid?

[joedow, 2015/02/14 02:31:28]

Done.

+
+  // Runs a nested MessageLoop while a network request is being made and
+  // blocks until it completes.  This member must be reset for every new network

[Wez, 2015/02/13 03:01:53]

RunLoops don't run a nested MessageLoop, and they don't block - they actually
run the underlying MessageLoop until you Quit() them.

[joedow, 2015/02/14 02:31:28]

Done.

+  // request that is made.
+  scoped_ptr<base::RunLoop> network_request_run_loop_;

[Wez, 2015/02/13 03:01:54]

Why does this need to be a class member, rather than being created on the stack?

[joedow, 2015/02/14 02:31:28]

Done.

+
+  // Stores the access token fetcher to use for network requests.

[Wez, 2015/02/13 03:01:53]

nit: Drop "stores the"

[joedow, 2015/02/14 02:31:28]

Done.

+  scoped_ptr<remoting::test::AccessTokenFetcher> access_token_fetcher_;
+
+  // Stores a token storage object to be used to read/write the refresh token

[Wez, 2015/02/13 03:01:53]

Drop everything before "Used to"

[joedow, 2015/02/14 02:31:28]

Done.

+  // from the disk.
+  scoped_ptr<remoting::test::RefreshTokenStorageInterface>
+      refresh_token_storage_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppRemotingTestDriverEnvironment);
+};
+
+// Unfortunately a global var is how this framework handles sharing data

[Wez, 2015/02/13 03:01:53]

Which framework?

[joedow, 2015/02/14 02:31:27]

Done.

+// between tests and keeping long-lived objects around.  We'll use this to
+// start up our environment and then share the data with the test fixtures/cases
+// as needed.

[Wez, 2015/02/13 03:01:53]

Suggest reducing this to "Used to share auth tokens and remote host connection
information across tests.

[joedow, 2015/02/14 02:31:28]

Done.

+extern AppRemotingTestDriverEnvironment* AppRemotingSharedData;

[Wez, 2015/02/13 03:01:53]

Suggest putting this at the top of the file, since it's at the top of the .cc

[joedow, 2015/02/14 02:31:28]

This line references the class declared just above it so moving it to the top of
the header causes a build break (unless I forward declare it).

[Wez, 2015/02/19 22:00:22]

Oh, fair point.

+
+}  // namespace test
+}  // namespace remoting
+
+#endif  // REMOTING_TEST_APP_REMOTING_TEST_DRIVER_ENVIRONMENT_H_