Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
880273006:
Adding the AccessTokenFetcher and Environment class to the app remoting test (Closed)
DescriptionAdding the AccessTokenFetcher and Environment class to the app remoting test
driver along with unit tests.
In this change, the test driver can now call GAIA and retrieve an access and
refresh token given an authorization code. It can then store the refresh token
locally and retrieve it on subsequent tool runs.
BUG=
Committed: https://crrev.com/ea77cec75493f936518fed723dcd6c371ad564d6
Cr-Commit-Position: refs/heads/master@{#318137}
Patch Set 1 #Patch Set 2 : Fixing some lint/readability errors #
Total comments: 213
Patch Set 3 : Addressing Wez's feedback #Patch Set 4 : Updating AccessTokenFetcher Unittests to use the FakeURLFetcherFactory #
Total comments: 70
Patch Set 5 : Addressing last round of CR feedback #
Total comments: 18
Patch Set 6 : Addressing CR feedback #
Total comments: 10
Patch Set 7 : CR feedback #
Total comments: 1
Patch Set 8 : Fixing final nit! #Patch Set 9 : Fixing vector initializer list syntax which was causing build failures on non C++11 compilers #Patch Set 10 : Fixing a Win platform only build issue. #Patch Set 11 : Fixing one last windows compile issue #Messages
Total messages: 30 (11 generated)
joedow@chromium.org changed reviewers: + jamiewalch@chromium.org, wez@chromium.org
Hi guys, Here is my second change list for the Remoting Test Driver. In this change I have added a class which will retrieve an Access Token given an authorization code or refresh token. The change also includes a helper class to read/write the refresh token on the local disk and a class which will be used to store access token and remote host info to be shared across test fixtures and cases. I've also included unit tests for the larger classes. Jamie/Wez, Please take a look and let me know if you have any questions. Thank you, Joe
Fixing a few lint errors, no major updates in this patch set.
On 2015/02/10 02:09:15, joedow wrote: > Fixing a few lint errors, no major updates in this patch set. Is there a BUG # associated with this work?
https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:25: using net::URLRequestContextGetter; nit: We rarely use using, and it seems strange to use it here to save only 5 chars per usage; is it really used that much in this file to be worth this? It's also confusing because it means that the use of net::URLRequestContextGetter is unqualified, while use of the remoting:: version is qualified... https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:33: } nit: Run git cl format on this; I think it'll put the {} on one line. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:40: DCHECK(auth_client_.get()); You create |auth_client_| in the ctor, so no need to DCHECK it here - client can't get here without calling the ctor! https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:54: DCHECK(auth_client_.get()); As above https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:67: DCHECK(auth_client_.get()); As above https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:88: DVLOG(2) << "Calling GetTokensFromAuthCode to exchange auth_code for token"; You're logging this log before actually making the call; it'll be logged even if you early-exit - consider moving it down to just before the call https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:95: DCHECK(access_token_.empty()); You DCHECK that |access_token_| is empty here but in GetAccessTokenFromRefreshToken you clear it instead - why? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:99: // Every call which uses a GaiaOAuthClient requires a new object. Not sure what this means; are you saying that you need a fresh GaiaOAuthClient for each request? If so then I'd suggest changing RefreshGaiaOAuthClientInstance to scoped_ptr<GaiaOAuthClient> CreateGaiaOAuthClient() and DCHECKing at the start of this method that |auth_client_| is null. You'd then reset() the |auth_client_| member in each of the response handler methods. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:118: access_token_.clear(); See comments above re DCHECKing vs clear()ing. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:129: scopes.push_back("https://www.googleapis.com/auth/drive"); Is there a way that you can initialize the vector<string> from a literal array, so you can move these up to the anonymous namespace at the top, alongside the other constants? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:217: DCHECK(!access_token_callback_.is_null()); There's no point DCHECKing this here; the Run() would crash if the callback is null - you want to DCHECK at the point where this operation actually starts, or at the point where the caller supplies the callback. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:240: DCHECK(!access_token_callback_.is_null()); See above re DCHECK https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:25: // the AccessTokenFetcher more efficiently. Why do we need this class? Why not just make the GaiaOAuthClient itself have virtual methods suitable for mocking? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:26: class GaiaOAuthClientAdapter { nit: Move this class into it's own .h & .cc files? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:28: explicit GaiaOAuthClientAdapter(net::URLRequestContextGetter* context_getter); const scoped_refptr<>& https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:31: // Uses the |auth_code| passed in to retrieve an access token. Since these three methods just mirror the GaiaOAuthClient API, just have a block comment "These methods mirror the GaiaOAuthClient API"? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:39: virtual void RefreshToken( This seems a daft name for the API; why is it not GetAccessTokenFromRefreshToken()? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:54: // to make those requests on the current thread's task runner. "Uses a ..." doesn't seem relevant here - the relevance of the context getter should be documented on the ctor, where it's passed-in. It's also the case that the thread that the context getter works on is an implementation detail of that class, not a property of this one. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:60: // The AccessTokenFetcher class is used to retrieve an access token from either You don't really need "The AccessTokenFetcher class is used to" - just start with "Retrieves an access token..." - the fact this is a class comment is implicit in its placement. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:62: // a refresh token will be retrieved as well. A new callback is supplied by the What does the fetching of the refresh token actually mean to the caller? Surely "if an auth code is provided" translates to "if GetAccessTokenFromAuthCode is used", i.e. this particular detail belongs on that API, not on the class? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:63: // client for each network call which will return a valid token on success or What network calls? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:66: // to destruct the AccessTokenFetcher instance within the callback if needed. This could be expressed more succinctly, e.g. "Destroying the AccessTokenFetcher while a request is outstanding will cancel the request. It is safe to delete the fetcher from within a completion callback." https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:68: // uses the current thread's task runner to make its network requests. Again, this could be shorter, e.g. "Must be used from a thread running an IO message loop." https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:75: // Method is marked virtual to allow for mocking. The comments re marking virtual feel like they'd make more sense as class-level comments explaining the virtual tags on the public & protected methods. What code is going to call AccessTokenFetcher, such that it's worth mocking it? (Sorry; it's been a while so I'm sure I've lost some context...) https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:86: virtual void RefreshGaiaOAuthClientInstance(); Why is this also virtual? For mocking? "Refresh" in the name implies that the client may be re-used across calls, but I don't think that's what you actually mean here? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:89: // to make those requests on the current thread's task runner. As above. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:112: void ValidateAccessToken(); ValidateAccessTokenAndNotifyCallback, or simply NotifyCallback? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:116: // be empty on failure. Why would the refresh token not be empty on failure? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:120: std::string access_token_; Why do you cache the access token in this class if you never return the same access token between consecutive calls? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:122: // Refresh token supplied by the caller or retrieved from a caller-supplied nit: No need for "Refresh token" https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:126: // Object which holds the client id, secret, and redirect url used to make nit: No need for "Object which" https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:12: const char kAuthCodeValue[] = "test_auth_code_value"; nit: Indentation https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:31: void OnAccessTokenRetrieved( Style-guide prefers that non-accessor methods not be implemented in-line. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:39: // the virtual members and have them call simplified versions. Since you'll ignore the other three parameters, doesn't that just save you _,_,_ from the EXPECT_CALL()s? Is it worth wrapping things just for that? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:71: void DelegateToCompleteCallbacks() { This function name isn't very descriptive - looks like this is called to set things up to mock a success? Or is the issue that you want to pull the delegate out of the mocked methods to pass to the completion methods below? Is there no cleaner way to do that? This class seems to have ended up being a hybrid mock/fake; would it be cleaner if it were a pure fake, i.e. the behaviour below is coded in rather than using gmock, and you then provide explicit call-count accessors for the tests to use to verify things? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:139: delegate->OnGetTokenInfoResponse(token_info.Pass()); nit: Just use make_scoped_ptr(new base::DictionaryValue()) here, and avoid the local variable? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:145: scoped_ptr<base::DictionaryValue> token_info(new base::DictionaryValue()); And here? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:163: // Verification members used to observe the value of the data retrieved. nit: Indentation https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:166: }; DISALLOW_COPY_AND_ASSIGN()? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:171: class TestAccessTokenFetcher : public AccessTokenFetcher { nit: AccessTokenFetcherForTest or FakeAccessTokenFetcher or MockAccessTokenFetcher? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:173: explicit TestAccessTokenFetcher(GaiaOAuthClientAdapter* mock_client) { Make this a scoped_ptr and use make_scoped_ptr() in calls? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:179: void RefreshGaiaOAuthClientInstance() override { If you follow the suggestion re CreateGaiaOAuthClient in the AccessTokenFetcher then you'd need to provide an AddMockGaiaOAuthClient() method and populate a list<> in this class, then have the Create...() method pop and return them in turn, I think. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:182: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:187: mock_gaia_client->DelegateToCompleteCallbacks(); Based on this usage, it looks like this should be called SetSuccessfulCompletionMocks() or similar? https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:207: // is destructed, but we do not want to free it at the end of the function. nit: destroyed :) https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:508: // is destructed, but we do not want to free it at the end of the function. Suggest moving this comment in this and the preceding tests, to the |mock_gaia_client| declaration, and stating that you're using a bare pointer so you can pass it off to the TestAccessTokenFetcher to own, while retaining a reference to it for mocking purposes. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:525: Times(0); Here and in earlier tests; can't you set these expectations the moment you create the |mock_gaia_client|? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:20: const char kLoggingLevelSwitchName[] = "verbosity"; Is this the name that other Chromium code uses for this? I thought Chrome just uses "v"? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:23: const char kUserNameSwitchName[] = "username"; Indentation https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:113: using remoting::test::AppRemotingSharedData; nit: You only refer to this a few times; suggest not using using here. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:120: // since we inject the single-process flag, but this is a safeguard in case Why would the single-process flag have any effect on how many times main() is run in a particular process? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:122: // initialize the global data as that will result in a leak or crash. If we Why do we not tear down the global data when main exits? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:173: switches::kAuthCodeSwitchName); Note that you can just call GetSwitchValueASCII and it'll return an empty string if there is no such value. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:33: if (initialized_) { Do you want to let things try to initialize this several times? What if one test initializes it with one auth-code and a different test uses a different one? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:37: if (!refresh_token_storage_.get()) { Drop .get() https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:69: std::string auth_code; // Empty auth code is used when refreshing. ? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:70: return RetrieveAccessToken(auth_code); RetrieveAccessToken might be better being RefreshAccessTokenFromAuthCode() - then it fits better w/ this function's name. With your comment in place you can just pass std::string() directly to the call. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:74: remoting::test::AccessTokenFetcher* mock_fetcher) { This parameter should be a scoped_ptr<> https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:79: remoting::test::RefreshTokenStorageInterface* mock_refresh_token_storage) { As above https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:84: // The object should already be initialized by the time SetUp is called. I thought SetUp and TearDown were called once per-test...? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:89: if (!initialized_) { Why would TearDown be called if we're not initialized? Surely that's a bug in the framework? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:108: network_request_run_loop_.reset(new base::RunLoop()); You can just create this on the stack, can't you, since you only use the RunLoop in this function's scope and Bind() the RunLoop::QuitClosure() into OnAccessTokenRetrieved to call? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:117: access_token_fetcher_.reset(new remoting::test::AccessTokenFetcher()); Are you sure that the fetcher won't bind to the potentially-temporary MessageLoop in some way? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:22: // access tokens and caching remote host connection information. 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? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:32: // a valid access token cannot be retrieved. The first part of the comment is pretty redundant; only the statement re return code is non-obvious https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:36: // This call blocks until either the token is retrieved or the call fails. You can shorten this to e.g. "Synchronously request a new access token using |refresh_token_|." https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:37: // Returns true if a valid access token has been retrieved, otherwise false. 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! https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:40: // Used for service call authentication. If this and the following accessor are used by tests then suggest bundling them together with a comment "Accessors for fields used by tests." https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:48: void SetAccessTokenFetcherForTesting( 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". https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:54: remoting::test::RefreshTokenStorageInterface* mock_refresh_token_storage); I'd recommend not mixing accessor methods and ones that done work, e.g. put all the accessor methods after the work methods. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:57: // Environment interface methods. nit: drop "methods"; that's implicit, since they are both methods. :) https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:61: // Used to retrieve an access token via either an auth code or refresh token. The parameter is called |auth_token|.. so presumably it only does the former? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:63: // false. See above re true/false in comments. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:66: // Called back after the access token fetcher completes its network requests. nit: No need for "back" nor for "its network requests". https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:68: // access token will be empty, the refresh token may be valid 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. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:73: // Used for service call authentication. What is "service call authentication"? Do you mean "Stored for used by tests to authenticate to the service."? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:76: // This token is used to retrieve a short lived access token. 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? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:79: // The user name to use for authentication. nit: Drop "the" https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:83: // host connection information. 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. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:88: bool initialized_; 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? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:91: // blocks until it completes. This member must be reset for every new network RunLoops don't run a nested MessageLoop, and they don't block - they actually run the underlying MessageLoop until you Quit() them. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:93: scoped_ptr<base::RunLoop> network_request_run_loop_; Why does this need to be a class member, rather than being created on the stack? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:95: // Stores the access token fetcher to use for network requests. nit: Drop "stores the" https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:98: // Stores a token storage object to be used to read/write the refresh token Drop everything before "Used to" https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:106: // Unfortunately a global var is how this framework handles sharing data Which framework? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:109: // as needed. Suggest reducing this to "Used to share auth tokens and remote host connection information across tests. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:110: extern AppRemotingTestDriverEnvironment* AppRemotingSharedData; Suggest putting this at the top of the file, since it's at the top of the .cc https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:59: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.cc:19: DCHECK(!callback.is_null()); No need to check a callback immediately before Run() https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:21: // class and network connection. See elsewhere re comment style. https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:35: void DelegateToCompleteCallbacks() { See elsewhere re naming of these methods! https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... File remoting/test/refresh_token_storage.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:20: return refresh_token_dir_path; nit: return base::FilePath() for clarity https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:23: // Build up the token path by appending the sub-directories. What happens if multiple instances of tests run at the same time? https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:26: refresh_token_dir_path = refresh_token_dir_path.Append(user_name.c_str()); Why c_str()? https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:42: std::string refresh_token; Declare this only when you actually need it, i.e. just before ReadFileToString https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:44: if (token_dir_path.empty()) { How can this ever be empty - just DCHECK here? https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:45: return refresh_token; You mean return std::string()? https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:52: DVLOG(1) << "Failed to read token file from: " << token_dir_path.value(); nit: Suggest explicitly returning std::string() here for clarity https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:109: NOTIMPLEMENTED() Do we compile the code for the other platforms? If not then suggest using #error here instead. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... File remoting/test/refresh_token_storage.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:14: // remove the file system dependency for testing. See elsewhere re comment style. Sounds like this interface is only required for mocking? https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:15: class RefreshTokenStorageInterface { It's traditional in Chromium to call this e.g. RefreshTokenStore and the implementation RefreshTokenStoreImpl https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:30: class RefreshTokenStorage : public RefreshTokenStorageInterface { I'd suggest moving this concrete implementation into the .cc file and adding a static RefreshTokenStore::CreateForDisk() method.
Addressing Wez's feedback. Beyond that, the big change was to move the mock GaiaOAuthClient class out into the gaia folder and treat it as a separate change. This CL assumes that change has gone in so I'm gated by that one. The other change was that I broke the mock AccessTokenFetcher class into a Mock and a Fake to make it cleaner (and it matches the layout of the GaiaOAuthClient change). PTAL, Joe https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:25: using net::URLRequestContextGetter; On 2015/02/13 03:01:50, Wez wrote: > nit: We rarely use using, and it seems strange to use it here to save only 5 > chars per usage; is it really used that much in this file to be worth this? It's > also confusing because it means that the use of net::URLRequestContextGetter is > unqualified, while use of the remoting:: version is qualified... Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:33: } On 2015/02/13 03:01:50, Wez wrote: > nit: Run git cl format on this; I think it'll put the {} on one line. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:40: DCHECK(auth_client_.get()); On 2015/02/13 03:01:50, Wez wrote: > You create |auth_client_| in the ctor, so no need to DCHECK it here - client > can't get here without calling the ctor! Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:54: DCHECK(auth_client_.get()); On 2015/02/13 03:01:50, Wez wrote: > As above Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:54: DCHECK(auth_client_.get()); On 2015/02/13 03:01:50, Wez wrote: > As above Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:67: DCHECK(auth_client_.get()); On 2015/02/13 03:01:50, Wez wrote: > As above Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:88: DVLOG(2) << "Calling GetTokensFromAuthCode to exchange auth_code for token"; On 2015/02/13 03:01:50, Wez wrote: > You're logging this log before actually making the call; it'll be logged even if > you early-exit - consider moving it down to just before the call Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:95: DCHECK(access_token_.empty()); On 2015/02/13 03:01:50, Wez wrote: > You DCHECK that |access_token_| is empty here but in > GetAccessTokenFromRefreshToken you clear it instead - why? I had assumed that this function would only be called once and that a second call would be an error. I'm updating the logic though as there isn't a business reason why someone couldn't call this twice with different auth codes. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:99: // Every call which uses a GaiaOAuthClient requires a new object. On 2015/02/13 03:01:50, Wez wrote: > Not sure what this means; are you saying that you need a fresh GaiaOAuthClient > for each request? > > If so then I'd suggest changing RefreshGaiaOAuthClientInstance to > scoped_ptr<GaiaOAuthClient> CreateGaiaOAuthClient() and DCHECKing at the start > of this method that |auth_client_| is null. You'd then reset() the > |auth_client_| member in each of the response handler methods. Updated the comment to be more clear. I don't want to pass a scoped_ptr as the scope of the auth_client_ object is longer than the function call. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:118: access_token_.clear(); On 2015/02/13 03:01:50, Wez wrote: > See comments above re DCHECKing vs clear()ing. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:129: scopes.push_back("https://www.googleapis.com/auth/drive"); On 2015/02/13 03:01:50, Wez wrote: > Is there a way that you can initialize the vector<string> from a literal array, > so you can move these up to the anonymous namespace at the top, alongside the > other constants? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:217: DCHECK(!access_token_callback_.is_null()); On 2015/02/13 03:01:50, Wez wrote: > There's no point DCHECKing this here; the Run() would crash if the callback is > null - you want to DCHECK at the point where this operation actually starts, or > at the point where the caller supplies the callback. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:240: DCHECK(!access_token_callback_.is_null()); On 2015/02/13 03:01:50, Wez wrote: > See above re DCHECK Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:25: // the AccessTokenFetcher more efficiently. On 2015/02/13 03:01:51, Wez wrote: > Why do we need this class? Why not just make the GaiaOAuthClient itself have > virtual methods suitable for mocking? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:26: class GaiaOAuthClientAdapter { On 2015/02/13 03:01:50, Wez wrote: > nit: Move this class into it's own .h & .cc files? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:28: explicit GaiaOAuthClientAdapter(net::URLRequestContextGetter* context_getter); On 2015/02/13 03:01:51, Wez wrote: > const scoped_refptr<>& Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:31: // Uses the |auth_code| passed in to retrieve an access token. On 2015/02/13 03:01:51, Wez wrote: > Since these three methods just mirror the GaiaOAuthClient API, just have a block > comment "These methods mirror the GaiaOAuthClient API"? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:39: virtual void RefreshToken( On 2015/02/13 03:01:51, Wez wrote: > This seems a daft name for the API; why is it not > GetAccessTokenFromRefreshToken()? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:54: // to make those requests on the current thread's task runner. On 2015/02/13 03:01:51, Wez wrote: > "Uses a ..." doesn't seem relevant here - the relevance of the context getter > should be documented on the ctor, where it's passed-in. > > It's also the case that the thread that the context getter works on is an > implementation detail of that class, not a property of this one. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:60: // The AccessTokenFetcher class is used to retrieve an access token from either On 2015/02/13 03:01:51, Wez wrote: > You don't really need "The AccessTokenFetcher class is used to" - just start > with "Retrieves an access token..." - the fact this is a class comment is > implicit in its placement. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:62: // a refresh token will be retrieved as well. A new callback is supplied by the On 2015/02/13 03:01:51, Wez wrote: > What does the fetching of the refresh token actually mean to the caller? Surely > "if an auth code is provided" translates to "if GetAccessTokenFromAuthCode is > used", i.e. this particular detail belongs on that API, not on the class? Done. Removed the comment since the tokens returned is tied to the callback signature rather than an aspect of this class. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:63: // client for each network call which will return a valid token on success or On 2015/02/13 03:01:51, Wez wrote: > What network calls? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:66: // to destruct the AccessTokenFetcher instance within the callback if needed. On 2015/02/13 03:01:51, Wez wrote: > This could be expressed more succinctly, e.g. "Destroying the AccessTokenFetcher > while a request is outstanding will cancel the request. It is safe to delete the > fetcher from within a completion callback." Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:68: // uses the current thread's task runner to make its network requests. On 2015/02/13 03:01:51, Wez wrote: > Again, this could be shorter, e.g. "Must be used from a thread running an IO > message loop." Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:75: // Method is marked virtual to allow for mocking. On 2015/02/13 03:01:51, Wez wrote: > The comments re marking virtual feel like they'd make more sense as class-level > comments explaining the virtual tags on the public & protected methods. > > What code is going to call AccessTokenFetcher, such that it's worth mocking it? > (Sorry; it's been a while so I'm sure I've lost some context...) Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:86: virtual void RefreshGaiaOAuthClientInstance(); On 2015/02/13 03:01:51, Wez wrote: > Why is this also virtual? For mocking? > > "Refresh" in the name implies that the client may be re-used across calls, but I > don't think that's what you actually mean here? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:89: // to make those requests on the current thread's task runner. On 2015/02/13 03:01:51, Wez wrote: > As above. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:112: void ValidateAccessToken(); On 2015/02/13 03:01:51, Wez wrote: > ValidateAccessTokenAndNotifyCallback, or simply NotifyCallback? Cleaned up the comment since the function doesn't actually call the callback, the GaiaOAuthClient::Delegate methods call the callback. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:116: // be empty on failure. On 2015/02/13 03:01:51, Wez wrote: > Why would the refresh token not be empty on failure? Comment was out of date, it is empty now. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:120: std::string access_token_; On 2015/02/13 03:01:51, Wez wrote: > Why do you cache the access token in this class if you never return the same > access token between consecutive calls? I make a validate token call after I receive the access token (which is suggested in the OAuth doc) and the data returned from the validate call does not include the original access token. So I squirrel away the token to return to the caller after the validate call succeeds. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:122: // Refresh token supplied by the caller or retrieved from a caller-supplied On 2015/02/13 03:01:50, Wez wrote: > nit: No need for "Refresh token" Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:126: // Object which holds the client id, secret, and redirect url used to make On 2015/02/13 03:01:50, Wez wrote: > nit: No need for "Object which" Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:12: const char kAuthCodeValue[] = "test_auth_code_value"; On 2015/02/13 03:01:52, Wez wrote: > nit: Indentation Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:31: void OnAccessTokenRetrieved( On 2015/02/13 03:01:52, Wez wrote: > Style-guide prefers that non-accessor methods not be implemented in-line. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:39: // the virtual members and have them call simplified versions. On 2015/02/13 03:01:52, Wez wrote: > Since you'll ignore the other three parameters, doesn't that just save you _,_,_ > from the EXPECT_CALL()s? Is it worth wrapping things just for that? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:71: void DelegateToCompleteCallbacks() { On 2015/02/13 03:01:51, Wez wrote: > This function name isn't very descriptive - looks like this is called to set > things up to mock a success? Or is the issue that you want to pull the delegate > out of the mocked methods to pass to the completion methods below? Is there no > cleaner way to do that? > > This class seems to have ended up being a hybrid mock/fake; would it be cleaner > if it were a pure fake, i.e. the behaviour below is coded in rather than using > gmock, and you then provide explicit call-count accessors for the tests to use > to verify things? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:139: delegate->OnGetTokenInfoResponse(token_info.Pass()); On 2015/02/13 03:01:52, Wez wrote: > nit: Just use make_scoped_ptr(new base::DictionaryValue()) here, and avoid the > local variable? Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:145: scoped_ptr<base::DictionaryValue> token_info(new base::DictionaryValue()); On 2015/02/13 03:01:52, Wez wrote: > And here? Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:163: // Verification members used to observe the value of the data retrieved. On 2015/02/13 03:01:52, Wez wrote: > nit: Indentation Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:166: }; On 2015/02/13 03:01:51, Wez wrote: > DISALLOW_COPY_AND_ASSIGN()? Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:171: class TestAccessTokenFetcher : public AccessTokenFetcher { On 2015/02/13 03:01:51, Wez wrote: > nit: AccessTokenFetcherForTest or FakeAccessTokenFetcher or > MockAccessTokenFetcher? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:173: explicit TestAccessTokenFetcher(GaiaOAuthClientAdapter* mock_client) { On 2015/02/13 03:01:51, Wez wrote: > Make this a scoped_ptr and use make_scoped_ptr() in calls? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:179: void RefreshGaiaOAuthClientInstance() override { On 2015/02/13 03:01:52, Wez wrote: > If you follow the suggestion re CreateGaiaOAuthClient in the AccessTokenFetcher > then you'd need to provide an AddMockGaiaOAuthClient() method and populate a > list<> in this class, then have the Create...() method pop and return them in > turn, I think. I don't think I need too make this complex (e.g. a list of objects). The idea is that the base class calls this function before every gaia call it makes and when I am using the mock object, I do not want to release/new another instance. I can override the function and do nothing and everything works great. I've updated the comment so it is clearer about what I am doing. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:182: }; On 2015/02/13 03:01:52, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:187: mock_gaia_client->DelegateToCompleteCallbacks(); On 2015/02/13 03:01:51, Wez wrote: > Based on this usage, it looks like this should be called > SetSuccessfulCompletionMocks() or similar? Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:207: // is destructed, but we do not want to free it at the end of the function. On 2015/02/13 03:01:52, Wez wrote: > nit: destroyed :) Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:508: // is destructed, but we do not want to free it at the end of the function. On 2015/02/13 03:01:52, Wez wrote: > Suggest moving this comment in this and the preceding tests, to the > |mock_gaia_client| declaration, and stating that you're using a bare pointer so > you can pass it off to the TestAccessTokenFetcher to own, while retaining a > reference to it for mocking purposes. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:525: Times(0); On 2015/02/13 03:01:51, Wez wrote: > Here and in earlier tests; can't you set these expectations the moment you > create the |mock_gaia_client|? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:20: const char kLoggingLevelSwitchName[] = "verbosity"; On 2015/02/13 03:01:52, Wez wrote: > Is this the name that other Chromium code uses for this? I thought Chrome just > uses "v"? I wanted a separate switch which only turned on logging for the remoting components. If you specify v, IIRC you also have to specify a vmodule pattern, and I wanted to make it simpler to turn logging on for all of the remoting components. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:23: const char kUserNameSwitchName[] = "username"; On 2015/02/13 03:01:52, Wez wrote: > Indentation Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:113: using remoting::test::AppRemotingSharedData; On 2015/02/13 03:01:52, Wez wrote: > nit: You only refer to this a few times; suggest not using using here. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:120: // since we inject the single-process flag, but this is a safeguard in case On 2015/02/13 03:01:52, Wez wrote: > Why would the single-process flag have any effect on how many times main() is > run in a particular process? It seemed like main was run twice when GTest was in multiprocess mode and I saw the global* get re-initialized but that was ~4 months ago so maybe I am remembering wrong... https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:122: // initialize the global data as that will result in a leak or crash. If we On 2015/02/13 03:01:52, Wez wrote: > Why do we not tear down the global data when main exits? The GTest framework handles it, there is a comment further down where I describe what happens. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:173: switches::kAuthCodeSwitchName); On 2015/02/13 03:01:52, Wez wrote: > Note that you can just call GetSwitchValueASCII and it'll return an empty string > if there is no such value. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:33: if (initialized_) { On 2015/02/13 03:01:53, Wez wrote: > Do you want to let things try to initialize this several times? > > What if one test initializes it with one auth-code and a different test uses a > different one? The Environment class is only initialized once (before tests run). We can assume that having an access token means it has been initialized (I removed the bool per you comment in the header file). https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:37: if (!refresh_token_storage_.get()) { On 2015/02/13 03:01:53, Wez wrote: > Drop .get() Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:69: std::string auth_code; // Empty auth code is used when refreshing. On 2015/02/13 03:01:52, Wez wrote: > ? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:70: return RetrieveAccessToken(auth_code); On 2015/02/13 03:01:53, Wez wrote: > RetrieveAccessToken might be better being RefreshAccessTokenFromAuthCode() - > then it fits better w/ this function's name. With your comment in place you can > just pass std::string() directly to the call. Done. I had thought about splitting the RetrieveAccessToken function up into auth code and refresh token specific functions, but there would be a lot of code duplication in that case. I'd like to leave it as-is and update the comment/param passed. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:74: remoting::test::AccessTokenFetcher* mock_fetcher) { On 2015/02/13 03:01:52, Wez wrote: > This parameter should be a scoped_ptr<> Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:79: remoting::test::RefreshTokenStorageInterface* mock_refresh_token_storage) { On 2015/02/13 03:01:53, Wez wrote: > As above Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:84: // The object should already be initialized by the time SetUp is called. On 2015/02/13 03:01:53, Wez wrote: > I thought SetUp and TearDown were called once per-test...? The environment class is setup and torn down once per test suite lifetime, SetUp and TearDown are called more or less frequently depending on whether you are talking about an Environment class, Static Test Fixture Method, or Test Fixture Method. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:89: if (!initialized_) { On 2015/02/13 03:01:52, Wez wrote: > Why would TearDown be called if we're not initialized? Surely that's a bug in > the framework? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:108: network_request_run_loop_.reset(new base::RunLoop()); On 2015/02/13 03:01:53, Wez wrote: > You can just create this on the stack, can't you, since you only use the RunLoop > in this function's scope and Bind() the RunLoop::QuitClosure() into > OnAccessTokenRetrieved to call? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:117: access_token_fetcher_.reset(new remoting::test::AccessTokenFetcher()); On 2015/02/13 03:01:53, Wez wrote: > Are you sure that the fetcher won't bind to the potentially-temporary > MessageLoop in some way? I updated the code so the real access token fetcher is released at the end of the function, however a mock class is not released which is what I want for the unit tests. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:22: // access tokens and caching remote host connection information. On 2015/02/13 03:01:54, Wez wrote: > 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? 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 : ) https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:32: // a valid access token cannot be retrieved. On 2015/02/13 03:01:54, Wez wrote: > The first part of the comment is pretty redundant; only the statement re return > code is non-obvious Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:36: // This call blocks until either the token is retrieved or the call fails. On 2015/02/13 03:01:54, Wez wrote: > You can shorten this to e.g. "Synchronously request a new access token using > |refresh_token_|." Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:37: // Returns true if a valid access token has been retrieved, otherwise false. On 2015/02/13 03:01:54, Wez wrote: > 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! Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:40: // Used for service call authentication. On 2015/02/13 03:01:53, Wez wrote: > If this and the following accessor are used by tests then suggest bundling them > together with a comment "Accessors for fields used by tests." Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:48: void SetAccessTokenFetcherForTesting( On 2015/02/13 03:01:53, Wez wrote: > 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". 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. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:54: remoting::test::RefreshTokenStorageInterface* mock_refresh_token_storage); On 2015/02/13 03:01:53, Wez wrote: > I'd recommend not mixing accessor methods and ones that done work, e.g. put all > the accessor methods after the work methods. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:57: // Environment interface methods. On 2015/02/13 03:01:54, Wez wrote: > nit: drop "methods"; that's implicit, since they are both methods. :) Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:61: // Used to retrieve an access token via either an auth code or refresh token. On 2015/02/13 03:01:53, Wez wrote: > The parameter is called |auth_token|.. so presumably it only does the former? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:63: // false. On 2015/02/13 03:01:53, Wez wrote: > See above re true/false in comments. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:66: // Called back after the access token fetcher completes its network requests. On 2015/02/13 03:01:54, Wez wrote: > nit: No need for "back" nor for "its network requests". Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:68: // access token will be empty, the refresh token may be valid On 2015/02/13 03:01:53, Wez wrote: > 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. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:73: // Used for service call authentication. On 2015/02/13 03:01:54, Wez wrote: > What is "service call authentication"? Do you mean "Stored for used by tests to > authenticate to the service."? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:76: // This token is used to retrieve a short lived access token. On 2015/02/13 03:01:53, Wez wrote: > 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? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:79: // The user name to use for authentication. On 2015/02/13 03:01:54, Wez wrote: > nit: Drop "the" Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:83: // host connection information. On 2015/02/13 03:01:53, Wez wrote: > 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. 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. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:88: bool initialized_; On 2015/02/13 03:01:53, Wez wrote: > 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? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:91: // blocks until it completes. This member must be reset for every new network On 2015/02/13 03:01:53, Wez wrote: > RunLoops don't run a nested MessageLoop, and they don't block - they actually > run the underlying MessageLoop until you Quit() them. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:93: scoped_ptr<base::RunLoop> network_request_run_loop_; On 2015/02/13 03:01:54, Wez wrote: > Why does this need to be a class member, rather than being created on the stack? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:95: // Stores the access token fetcher to use for network requests. On 2015/02/13 03:01:53, Wez wrote: > nit: Drop "stores the" Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:98: // Stores a token storage object to be used to read/write the refresh token On 2015/02/13 03:01:53, Wez wrote: > Drop everything before "Used to" Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:106: // Unfortunately a global var is how this framework handles sharing data On 2015/02/13 03:01:53, Wez wrote: > Which framework? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:109: // as needed. On 2015/02/13 03:01:53, Wez wrote: > Suggest reducing this to "Used to share auth tokens and remote host connection > information across tests. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:110: extern AppRemotingTestDriverEnvironment* AppRemotingSharedData; On 2015/02/13 03:01:53, Wez wrote: > Suggest putting this at the top of the file, since it's at the top of the .cc 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). https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:59: }; On 2015/02/13 03:01:54, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.cc:19: DCHECK(!callback.is_null()); On 2015/02/13 03:01:54, Wez wrote: > No need to check a callback immediately before Run() Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:21: // class and network connection. On 2015/02/13 03:01:54, Wez wrote: > See elsewhere re comment style. Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:35: void DelegateToCompleteCallbacks() { On 2015/02/13 03:01:54, Wez wrote: > See elsewhere re naming of these methods! Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... File remoting/test/refresh_token_storage.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:20: return refresh_token_dir_path; On 2015/02/13 03:01:54, Wez wrote: > nit: return base::FilePath() for clarity Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:23: // Build up the token path by appending the sub-directories. On 2015/02/13 03:01:54, Wez wrote: > What happens if multiple instances of tests run at the same time? Nothing interesting in this function, I think the interesting case is where we create the file and write the contents. At the moment I don't do anything special for that case except check return values and output some debugging messages if something interesting happens. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:26: refresh_token_dir_path = refresh_token_dir_path.Append(user_name.c_str()); On 2015/02/13 03:01:54, Wez wrote: > Why c_str()? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:42: std::string refresh_token; On 2015/02/13 03:01:54, Wez wrote: > Declare this only when you actually need it, i.e. just before ReadFileToString Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:44: if (token_dir_path.empty()) { On 2015/02/13 03:01:54, Wez wrote: > How can this ever be empty - just DCHECK here? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:45: return refresh_token; On 2015/02/13 03:01:54, Wez wrote: > You mean return std::string()? Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:52: DVLOG(1) << "Failed to read token file from: " << token_dir_path.value(); On 2015/02/13 03:01:54, Wez wrote: > nit: Suggest explicitly returning std::string() here for clarity Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.cc:109: NOTIMPLEMENTED() On 2015/02/13 03:01:54, Wez wrote: > Do we compile the code for the other platforms? If not then suggest using #error > here instead. I have not limited the platforms yet as I think it would be good to build on other platforms so engineers will find out faster if they break some interface the tool uses. Also, I can imagine a scenario where the tool will be supported in other environments but you have to provide an access token (or something along those lines). https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... File remoting/test/refresh_token_storage.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:14: // remove the file system dependency for testing. On 2015/02/13 03:01:55, Wez wrote: > See elsewhere re comment style. Sounds like this interface is only required for > mocking? Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:15: class RefreshTokenStorageInterface { On 2015/02/13 03:01:55, Wez wrote: > It's traditional in Chromium to call this e.g. RefreshTokenStore and the > implementation RefreshTokenStoreImpl Done. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:30: class RefreshTokenStorage : public RefreshTokenStorageInterface { On 2015/02/13 03:01:55, Wez wrote: > I'd suggest moving this concrete implementation into the .cc file and adding a > static RefreshTokenStore::CreateForDisk() method. Done.
Updated the AccessTokenFetcher unittests to use a FakeURLFetcherFactory per guidance from a GaiaOAuthClient developer.
https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... File remoting/test/access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:120: std::string access_token_; On 2015/02/14 02:31:26, joedow wrote: > On 2015/02/13 03:01:51, Wez wrote: > > Why do you cache the access token in this class if you never return the same > > access token between consecutive calls? > > I make a validate token call after I receive the access token (which is > suggested in the OAuth doc) and the data returned from the validate call does > not include the original access token. So I squirrel away the token to return > to the caller after the validate call succeeds. OK; it wasn't clear why you needed to store it rather than Bind()ing it into a completion callback for the validate operation. I think it'd make sense to clarify why we need this member, e.g. // Used to store the access-token while the GetTokenInfo request to verify it is in-progress. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver.cc:20: const char kLoggingLevelSwitchName[] = "verbosity"; On 2015/02/14 02:31:27, joedow wrote: > On 2015/02/13 03:01:52, Wez wrote: > > Is this the name that other Chromium code uses for this? I thought Chrome > just > > uses "v"? > > I wanted a separate switch which only turned on logging for the remoting > components. If you specify v, IIRC you also have to specify a vmodule pattern, > and I wanted to make it simpler to turn logging on for all of the remoting > components. Is --vmodule format really that complex? https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.cc (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:70: return RetrieveAccessToken(auth_code); On 2015/02/14 02:31:27, joedow wrote: > On 2015/02/13 03:01:53, Wez wrote: > > RetrieveAccessToken might be better being RefreshAccessTokenFromAuthCode() - > > then it fits better w/ this function's name. With your comment in place you > can > > just pass std::string() directly to the call. > > Done. I had thought about splitting the RetrieveAccessToken function up into > auth code and refresh token specific functions, but there would be a lot of code > duplication in that case. I'd like to leave it as-is and update the > comment/param passed. Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:84: // The object should already be initialized by the time SetUp is called. On 2015/02/14 02:31:27, joedow wrote: > On 2015/02/13 03:01:53, Wez wrote: > > I thought SetUp and TearDown were called once per-test...? > > The environment class is setup and torn down once per test suite lifetime, SetUp > and TearDown are called more or less frequently depending on whether you are > talking about an Environment class, Static Test Fixture Method, or Test Fixture > Method. Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:117: access_token_fetcher_.reset(new remoting::test::AccessTokenFetcher()); On 2015/02/14 02:31:27, joedow wrote: > On 2015/02/13 03:01:53, Wez wrote: > > Are you sure that the fetcher won't bind to the potentially-temporary > > MessageLoop in some way? > > I updated the code so the real access token fetcher is released at the end of > the function, however a mock class is not released which is what I want for the > unit tests. Sorry, I don't understand what you mean. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:48: void SetAccessTokenFetcherForTesting( On 2015/02/14 02:31:28, joedow wrote: > On 2015/02/13 03:01:53, Wez wrote: > > 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". > > 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. Acknowledged. https://codereview.chromium.org/880273006/diff/20001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:110: extern AppRemotingTestDriverEnvironment* AppRemotingSharedData; On 2015/02/14 02:31:28, joedow wrote: > On 2015/02/13 03:01:53, Wez wrote: > > Suggest putting this at the top of the file, since it's at the top of the .cc > > 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). Oh, fair point. https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... File remoting/test/refresh_token_storage.h (right): https://codereview.chromium.org/880273006/diff/20001/remoting/test/refresh_to... remoting/test/refresh_token_storage.h:15: class RefreshTokenStorageInterface { On 2015/02/14 02:31:29, joedow wrote: > On 2015/02/13 03:01:55, Wez wrote: > > It's traditional in Chromium to call this e.g. RefreshTokenStore and the > > implementation RefreshTokenStoreImpl > > Done. Doesn't look like this actually got done. ;) https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:49: DCHECK(!callback.is_null()); nit: Typically we leave a blank line between DCHECKs on preconditions, and actual function logic. Now that you're DCHECK()ing these preconditions I'd suggest doing the same for access_token_callback_.is_null(), since that is another precondition - the caller is broken if we reach here while a request is already in progress. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:61: // |auth_client_| objects can only be used for one service call so we need to What's a "service call"? Do you mean "request to GAIA", or something else? |auth_client_| is a member, not a type, so it doesn't make sense to refer to "|auth_client_| objects" - do you mean "OAuthClientClient instances" for example? https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:62: // create a new one before each gaia service request. Not sure what a "gaia service request" is? Also, gaia -> GAIA here and everywhere else in this code. Could this simply be "Create a new GaiaOAuthClient for each request" for example? https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:73: DCHECK(!callback.is_null()); See above https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:86: // create a new one before each gaia service request. See above https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:102: auth_client_.reset(new gaia::GaiaOAuthClient(request_context_getter.get())); When you reset this member, any existing GaiaOAuthClient instance will be destroyed, even if it has a request pending - is that safe & correct? https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:22: // which will return a valid token on success or an empty token on failure. I think you mean "for each request to GAIA". Why are you documenting the contents of the token field to the callback here, rather than on the definition of the callback type, or the function to which the callback is supplied? https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:78: // to make those requests on the current thread's task runner. Do you need the second line? Is that property not part of the GaiaOAuthClient's interface? There is no request_context_getter member - do you mean URLRequestContextGetter, the type? https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:56: // passed to the GaiaOAuthClient instance on construction. This seems to be an irrelevant implementation detail, from the PoV of the class' callers. I'd suggest something more like "The fixture also creates an IO MessageLoop, if necessary, for use by the AccessTokenFetcher." - I don't see any mention of GaiaOAuthClient in this file, nor of creation of any "resource context getter", so that is presumably an internal detail of AccessTokenFetcher and not something that should be mentioned here. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:64: base::Closure run_loop_quit_closure, Suggest calling this done_closure, since in principle you could pass any closure you want, not just a RunLoop::QuitClosure(). https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:91: std::string access_token_retrieved; Members must end in _ https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:114: base::RunLoop network_request_run_loop; Why not just |run_loop|? https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:144: base::RunLoop network_request_run_loop; As above https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:174: scoped_ptr<base::RunLoop> network_request_run_loop; As above https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:80: void AppRemotingTestDriverEnvironment::SetUp() { Do you need these at all, now that they are empty? https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:96: base::RunLoop network_request_run_loop; nit: Just run_loop? https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:109: access_token_fetcher.reset(mock_access_token_fetcher_.get()); This is a risky construct; instead of taking ownership of mock_access_token_fetcher_ and later releasing it, I'd suggest something like: scoped_ptr<AccessTokenFetcher> temporary_access_token_fetcher; AccessTokenFetcher* access_token_fetcher = access_token_fetcher_for_test_; if (!access_token_fetcher) { temporary_access_token_fetcher.reset(...); access_token_fetcher = ...; } .. do some work .. i.e. only "own" a local temporary AccessTokenFetcher, otherwise just use whatever was was passed in via ...ForTest(). https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:34: // Used to set a mock version of the fetcher for testing. nit: Clarify "for testing the driver environment itself." https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:36: scoped_ptr<remoting::test::AccessTokenFetcher> mock_fetcher); nit: mock_fetcher->fetcher - it'd be valid for a caller to provide a fake here, for example :) Do you need the caller to actually pass in the fetcher? Or would it make sense to supply only a bare pointer to the fetcher - if you pass it then can't it potentially outlive its originating test? That seems like something we need to avoid - you might want to provide a helper class that takes a mock or fake fetcher and sets it on the driver environment, then removes it again when done - then your fetchers can be created on the stack in the test and everything should be cleanly torn down between tests. Or is it the case that since these are only used in unit-tests for the driver environment itself, we don't need to worry about the environment spanning multiple test lifetimes? https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:38: // Used to set a mock version of the refresh token store object for testing. nit: If you make the comment on the fetcher setter more generic you could get away with a single comment applying to both of these methods, e.g. "Setters used by driver environment tests." https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:86: // the environment object is destructed, but we do not need to free it. I don't understand this comment - there's no "token_store" variable here. Do you mean that you're handing off the "fake_token_store"? https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:26: void GetAccessTokenFromAuthCode( nit: Comment "// AccessTokenFetcher interface." before these two methods. https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:35: // an error until the flag is reset by the caller. Suggest changing this to an in-line setter set_fail_access_token_from_auth_code(bool fail), that sets a member fail_access_token_from_auth_code_. Then you probably don't even need a comment. https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:43: // Causes GetAccessTokenFromAuthCode() to simulate an error. nit: This member doesn't cause anything; you might say "True if getAccessTokenFromAuthCode() should fail." https://codereview.chromium.org/880273006/diff/60001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:33: // appropriate method on the fake object. So the idea is that by default the supplied fake will be used, unless the test overrides by mocking? What happens if the caller never supplies a fake? https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... File remoting/test/refresh_token_store.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:14: // functions in this file. "Returns the FilePath of the token store file for |user_name|." https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:15: base::FilePath GetRefreshTokenDirPath(const std::string& user_name) { You never use the directory path on it's own; you always add the token file path to it - why not change this to GetRefreshTokenFilePath()? https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:22: // Build up the token path by appending the sub-directories. We can see that you're appending directories; no need to say so :) https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:43: // and returns the refresh token value if successful or empty string if not. This and the method below are inherited from the interface - you shouldn't need to re-document them here, just add a comment "RefreshTokenStore interface." to clarify that that is what they override. In general, avoid documenting implementation details in the interface of any class or interface; in this case a simple class-level comment saying that this concrete implementation loads/saves from disk, and then comments in the method definitions to describe how it does that, would be appropriate. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... File remoting/test/refresh_token_store.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:16: // dependency for testing. What functionality? https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:17: class RefreshTokenStoreInterface { RefreshTokenStore - Chromium style has the interface with a "plain" name and implementations with longer names, e.g. RefreshTokenStoreOnDisk, etc. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:22: virtual std::string ReadRefreshTokenFromDisk( This name assumes an on-disk implementation, but this is an interface so I think you want just Get/SetRefreshToken or Store/FetchRefreshToken. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:29: scoped_ptr<RefreshTokenStoreInterface> GetRefreshTokenStoreForDisk(); Suggest making this a static OnDisk() method in RefreshTokenStore, i.e. RefreshTokenStore::OnDisk()
Addressed wez's feedback, PTAL. Thanks, Joe https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:49: DCHECK(!callback.is_null()); On 2015/02/19 22:00:23, Wez wrote: > nit: Typically we leave a blank line between DCHECKs on preconditions, and > actual function logic. > > Now that you're DCHECK()ing these preconditions I'd suggest doing the same for > access_token_callback_.is_null(), since that is another precondition - the > caller is broken if we reach here while a request is already in progress. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:61: // |auth_client_| objects can only be used for one service call so we need to On 2015/02/19 22:00:23, Wez wrote: > What's a "service call"? Do you mean "request to GAIA", or something else? > > |auth_client_| is a member, not a type, so it doesn't make sense to refer to > "|auth_client_| objects" - do you mean "OAuthClientClient instances" for > example? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:62: // create a new one before each gaia service request. On 2015/02/19 22:00:23, Wez wrote: > Not sure what a "gaia service request" is? > > Also, gaia -> GAIA here and everywhere else in this code. > > Could this simply be "Create a new GaiaOAuthClient for each request" for > example? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:73: DCHECK(!callback.is_null()); On 2015/02/19 22:00:23, Wez wrote: > See above Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:86: // create a new one before each gaia service request. On 2015/02/19 22:00:23, Wez wrote: > See above Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:102: auth_client_.reset(new gaia::GaiaOAuthClient(request_context_getter.get())); On 2015/02/19 22:00:23, Wez wrote: > When you reset this member, any existing GaiaOAuthClient instance will be > destroyed, even if it has a request pending - is that safe & correct? A caller cannot make two concurrent requests (I had a condition check and have replaced it was a DCHECK() in the two GetAccessTokenFrom* calls). And the code in the class only creates a new client in the Validate function which is called after the previous call has completed. That said, the GaiaOAuthClient could always be destroyed with a pending request if the object is torn down so I'd expect that it should handle that (since there isn't much a consumer of the class could do to prevent it from being destroyed if it is also being torn down). https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:22: // which will return a valid token on success or an empty token on failure. On 2015/02/19 22:00:23, Wez wrote: > I think you mean "for each request to GAIA". > > Why are you documenting the contents of the token field to the callback here, > rather than on the definition of the callback type, or the function to which the > callback is supplied? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.h:78: // to make those requests on the current thread's task runner. On 2015/02/19 22:00:23, Wez wrote: > Do you need the second line? Is that property not part of the GaiaOAuthClient's > interface? > > There is no request_context_getter member - do you mean URLRequestContextGetter, > the type? I think that line was more pertinent in a previous iteration, I've removed it. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher_unittest.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:56: // passed to the GaiaOAuthClient instance on construction. On 2015/02/19 22:00:23, Wez wrote: > This seems to be an irrelevant implementation detail, from the PoV of the class' > callers. > > I'd suggest something more like "The fixture also creates an IO MessageLoop, if > necessary, for use by the AccessTokenFetcher." - I don't see any mention of > GaiaOAuthClient in this file, nor of creation of any "resource context getter", > so that is presumably an internal detail of AccessTokenFetcher and not something > that should be mentioned here. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:64: base::Closure run_loop_quit_closure, On 2015/02/19 22:00:23, Wez wrote: > Suggest calling this done_closure, since in principle you could pass any closure > you want, not just a RunLoop::QuitClosure(). Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:91: std::string access_token_retrieved; On 2015/02/19 22:00:23, Wez wrote: > Members must end in _ Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:114: base::RunLoop network_request_run_loop; On 2015/02/19 22:00:23, Wez wrote: > Why not just |run_loop|? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:144: base::RunLoop network_request_run_loop; On 2015/02/19 22:00:23, Wez wrote: > As above Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher_unittest.cc:174: scoped_ptr<base::RunLoop> network_request_run_loop; On 2015/02/19 22:00:23, Wez wrote: > As above Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:80: void AppRemotingTestDriverEnvironment::SetUp() { On 2015/02/19 22:00:23, Wez wrote: > Do you need these at all, now that they are empty? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:96: base::RunLoop network_request_run_loop; On 2015/02/19 22:00:23, Wez wrote: > nit: Just run_loop? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.cc:109: access_token_fetcher.reset(mock_access_token_fetcher_.get()); On 2015/02/19 22:00:23, Wez wrote: > This is a risky construct; instead of taking ownership of > mock_access_token_fetcher_ and later releasing it, I'd suggest something like: > > scoped_ptr<AccessTokenFetcher> temporary_access_token_fetcher; > AccessTokenFetcher* access_token_fetcher = access_token_fetcher_for_test_; > if (!access_token_fetcher) { > temporary_access_token_fetcher.reset(...); > access_token_fetcher = ...; > } > > .. do some work .. > > i.e. only "own" a local temporary AccessTokenFetcher, otherwise just use > whatever was was passed in via ...ForTest(). Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:34: // Used to set a mock version of the fetcher for testing. On 2015/02/19 22:00:24, Wez wrote: > nit: Clarify "for testing the driver environment itself." Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:36: scoped_ptr<remoting::test::AccessTokenFetcher> mock_fetcher); On 2015/02/19 22:00:24, Wez wrote: > nit: mock_fetcher->fetcher - it'd be valid for a caller to provide a fake here, > for example :) > > Do you need the caller to actually pass in the fetcher? Or would it make sense > to supply only a bare pointer to the fetcher - if you pass it then can't it > potentially outlive its originating test? > > That seems like something we need to avoid - you might want to provide a helper > class that takes a mock or fake fetcher and sets it on the driver environment, > then removes it again when done - then your fetchers can be created on the stack > in the test and everything should be cleanly torn down between tests. > > Or is it the case that since these are only used in unit-tests for the driver > environment itself, we don't need to worry about the environment spanning > multiple test lifetimes? I've updated this code to use a friend class. That way I can remove the ForTest setters and simplify the class a bit, plus it will be simpler to add additional unit tests later if I need to mock/fake new members out. The environment object's lifetime is limited to function scope for its unit tests so it won't hold on to objects across multiple tests. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:38: // Used to set a mock version of the refresh token store object for testing. On 2015/02/19 22:00:23, Wez wrote: > nit: If you make the comment on the fetcher setter more generic you could get > away with a single comment applying to both of these methods, e.g. "Setters used > by driver environment tests." Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:86: // the environment object is destructed, but we do not need to free it. On 2015/02/19 22:00:24, Wez wrote: > I don't understand this comment - there's no "token_store" variable here. Do you > mean that you're handing off the "fake_token_store"? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:26: void GetAccessTokenFromAuthCode( On 2015/02/19 22:00:24, Wez wrote: > nit: Comment "// AccessTokenFetcher interface." before these two methods. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:35: // an error until the flag is reset by the caller. On 2015/02/19 22:00:24, Wez wrote: > Suggest changing this to an in-line setter > set_fail_access_token_from_auth_code(bool fail), that sets a member > fail_access_token_from_auth_code_. Then you probably don't even need a comment. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:43: // Causes GetAccessTokenFromAuthCode() to simulate an error. On 2015/02/19 22:00:24, Wez wrote: > nit: This member doesn't cause anything; you might say "True if > getAccessTokenFromAuthCode() should fail." Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:33: // appropriate method on the fake object. On 2015/02/19 22:00:24, Wez wrote: > So the idea is that by default the supplied fake will be used, unless the test > overrides by mocking? > > What happens if the caller never supplies a fake? It depends on the class that uses it, if the class waits for the callback to run then it should supply a fake to run the callback, if the class doesn't care about the callback it could just use the mock. The first version this class had the Mock and Fake in the same class which wasn't right, so my approach this time is to have separate classes that can be used independently. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... File remoting/test/refresh_token_store.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:14: // functions in this file. On 2015/02/19 22:00:24, Wez wrote: > "Returns the FilePath of the token store file for |user_name|." Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:15: base::FilePath GetRefreshTokenDirPath(const std::string& user_name) { On 2015/02/19 22:00:24, Wez wrote: > You never use the directory path on it's own; you always add the token file path > to it - why not change this to GetRefreshTokenFilePath()? I use the path w/o file name to see if the directory exists and then create it if not. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:22: // Build up the token path by appending the sub-directories. On 2015/02/19 22:00:24, Wez wrote: > We can see that you're appending directories; no need to say so :) Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:43: // and returns the refresh token value if successful or empty string if not. On 2015/02/19 22:00:24, Wez wrote: > This and the method below are inherited from the interface - you shouldn't need > to re-document them here, just add a comment "RefreshTokenStore interface." to > clarify that that is what they override. > > In general, avoid documenting implementation details in the interface of any > class or interface; in this case a simple class-level comment saying that this > concrete implementation loads/saves from disk, and then comments in the method > definitions to describe how it does that, would be appropriate. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... File remoting/test/refresh_token_store.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:16: // dependency for testing. On 2015/02/19 22:00:24, Wez wrote: > What functionality? Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:17: class RefreshTokenStoreInterface { On 2015/02/19 22:00:24, Wez wrote: > RefreshTokenStore - Chromium style has the interface with a "plain" name and > implementations with longer names, e.g. RefreshTokenStoreOnDisk, etc. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:22: virtual std::string ReadRefreshTokenFromDisk( On 2015/02/19 22:00:24, Wez wrote: > This name assumes an on-disk implementation, but this is an interface so I think > you want just Get/SetRefreshToken or Store/FetchRefreshToken. Done. https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:29: scoped_ptr<RefreshTokenStoreInterface> GetRefreshTokenStoreForDisk(); On 2015/02/19 22:00:24, Wez wrote: > Suggest making this a static OnDisk() method in RefreshTokenStore, i.e. > RefreshTokenStore::OnDisk() Done.
https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... File remoting/test/access_token_fetcher.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/access_tok... remoting/test/access_token_fetcher.cc:102: auth_client_.reset(new gaia::GaiaOAuthClient(request_context_getter.get())); On 2015/02/20 02:58:35, joedow wrote: > On 2015/02/19 22:00:23, Wez wrote: > > When you reset this member, any existing GaiaOAuthClient instance will be > > destroyed, even if it has a request pending - is that safe & correct? > > A caller cannot make two concurrent requests (I had a condition check and have > replaced it was a DCHECK() in the two GetAccessTokenFrom* calls). And the code > in the class only creates a new client in the Validate function which is called > after the previous call has completed. > > That said, the GaiaOAuthClient could always be destroyed with a pending request > if the object is torn down so I'd expect that it should handle that (since there > isn't much a consumer of the class could do to prevent it from being destroyed > if it is also being torn down). Agreed; it was really the AccessTokenFetcher caller semantics I was questioning, since if you tear down an existing GaiaOAuthClient then you'll never get the callbacks you'd have expected from it. https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment.h:36: scoped_ptr<remoting::test::AccessTokenFetcher> mock_fetcher); On 2015/02/20 02:58:36, joedow wrote: > On 2015/02/19 22:00:24, Wez wrote: > > nit: mock_fetcher->fetcher - it'd be valid for a caller to provide a fake > here, > > for example :) > > > > Do you need the caller to actually pass in the fetcher? Or would it make sense > > to supply only a bare pointer to the fetcher - if you pass it then can't it > > potentially outlive its originating test? > > > > That seems like something we need to avoid - you might want to provide a > helper > > class that takes a mock or fake fetcher and sets it on the driver environment, > > then removes it again when done - then your fetchers can be created on the > stack > > in the test and everything should be cleanly torn down between tests. > > > > Or is it the case that since these are only used in unit-tests for the driver > > environment itself, we don't need to worry about the environment spanning > > multiple test lifetimes? > > I've updated this code to use a friend class. That way I can remove the ForTest > setters and simplify the class a bit, plus it will be simpler to add additional > unit tests later if I need to mock/fake new members out. > > The environment object's lifetime is limited to function scope for its unit > tests so it won't hold on to objects across multiple tests. Acknowledged. https://codereview.chromium.org/880273006/diff/60001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:33: // appropriate method on the fake object. On 2015/02/20 02:58:36, joedow wrote: > On 2015/02/19 22:00:24, Wez wrote: > > So the idea is that by default the supplied fake will be used, unless the test > > overrides by mocking? > > > > What happens if the caller never supplies a fake? > > It depends on the class that uses it, if the class waits for the callback to run > then it should supply a fake to run the callback, if the class doesn't care > about the callback it could just use the mock. > > The first version this class had the Mock and Fake in the same class which > wasn't right, so my approach this time is to have separate classes that can be > used independently. I understand breaking the two classes apart; it's the purpose of this SetFake...() method that seems to set up the mock to just fall-back to calling on to a supplied read or fake fetcher. IIUC the aim is to use the supplied fetcher for the actual fecther behaviour, but still allow tests to use mocking to detect calls and verify them? https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... File remoting/test/refresh_token_store.cc (right): https://codereview.chromium.org/880273006/diff/60001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:15: base::FilePath GetRefreshTokenDirPath(const std::string& user_name) { On 2015/02/20 02:58:36, joedow wrote: > On 2015/02/19 22:00:24, Wez wrote: > > You never use the directory path on it's own; you always add the token file > path > > to it - why not change this to GetRefreshTokenFilePath()? > > I use the path w/o file name to see if the directory exists and then create it > if not. Ah yes. It may be clearer to construct the file filepath and use DirName() on it to get the directory name in the one location that you need it. https://codereview.chromium.org/880273006/diff/80001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:65: class AppRemotingTestDriverEnvironmentUnittestHelper { What's the benefit of having this class versus having the two settings in the real class with a ForTest() name? Naming something with ForTest() will mean the PRESUBMIT checks will fire if non-test code tries to use it. If you prefer to keep this class then consider making it a class that tests actually instantiate, and that owns the AccessTokenFetcher and RefreshTokenStore - that way the driver environment can have bare pointers for those, rather than owning them itself, and you won't have to Pass() them to it, which will simplify your tests below. https://codereview.chromium.org/880273006/diff/80001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:88: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:35: void SetAuthCodeError(bool fail) { set_fail_access_token_from_auth_code(), to match the name of the member that it sets. https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:39: void SetRefreshTokenError(bool fail) { See above. https://codereview.chromium.org/880273006/diff/80001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:34: void SetFakeAccessTokenFetcher(scoped_ptr<AccessTokenFetcher> fake); This accepts any AccessTokenFetcher, so you shouldn't describe it as "fake". e.g. rename this SetRealAccessTokenFetcher() or SetDelegateAccessTokenFetcher(). You can still mention in the comment that this is typically used to pass a fake to which the mock should delegate, so that the calling test can mock it just to catch calls. https://codereview.chromium.org/880273006/diff/80001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:38: // This must be called *before* the custom ON_CALL() statements. This comment doesn't make sense - the method is private so no-one can call it at all. Move this functionality into SetFakeAccessTokenFetcher()? https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... File remoting/test/refresh_token_store.cc (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:42: nit: Remove this blank line, so it's clear that "RefreshTokenStore interface" refers to both of these methods. https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... File remoting/test/refresh_token_store.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:28: static scoped_ptr<RefreshTokenStore> OnDisk(); nit: Add a comment to explain any specific properties of the implementation that this method returns, e.g. how the on-disk path is chosen.
https://codereview.chromium.org/880273006/diff/80001/remoting/test/app_remoti... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:65: class AppRemotingTestDriverEnvironmentUnittestHelper { On 2015/02/23 20:17:14, Wez wrote: > What's the benefit of having this class versus having the two settings in the > real class with a ForTest() name? Naming something with ForTest() will mean the > PRESUBMIT checks will fire if non-test code tries to use it. > > If you prefer to keep this class then consider making it a class that tests > actually instantiate, and that owns the AccessTokenFetcher and RefreshTokenStore > - that way the driver environment can have bare pointers for those, rather than > owning them itself, and you won't have to Pass() them to it, which will simplify > your tests below. Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/app_remoti... remoting/test/app_remoting_test_driver_environment_unittest.cc:88: }; On 2015/02/23 20:17:14, Wez wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:35: void SetAuthCodeError(bool fail) { On 2015/02/23 20:17:15, Wez wrote: > set_fail_access_token_from_auth_code(), to match the name of the member that it > sets. Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:39: void SetRefreshTokenError(bool fail) { On 2015/02/23 20:17:15, Wez wrote: > See above. Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/mock_acces... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:34: void SetFakeAccessTokenFetcher(scoped_ptr<AccessTokenFetcher> fake); On 2015/02/23 20:17:15, Wez wrote: > This accepts any AccessTokenFetcher, so you shouldn't describe it as "fake". > e.g. rename this SetRealAccessTokenFetcher() or SetDelegateAccessTokenFetcher(). > You can still mention in the comment that this is typically used to pass a fake > to which the mock should delegate, so that the calling test can mock it just to > catch calls. Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/mock_acces... remoting/test/mock_access_token_fetcher.h:38: // This must be called *before* the custom ON_CALL() statements. On 2015/02/23 20:17:15, Wez wrote: > This comment doesn't make sense - the method is private so no-one can call it at > all. > > Move this functionality into SetFakeAccessTokenFetcher()? Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... File remoting/test/refresh_token_store.cc (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... remoting/test/refresh_token_store.cc:42: On 2015/02/23 20:17:15, Wez wrote: > nit: Remove this blank line, so it's clear that "RefreshTokenStore interface" > refers to both of these methods. Done. https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... File remoting/test/refresh_token_store.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/refresh_to... remoting/test/refresh_token_store.h:28: static scoped_ptr<RefreshTokenStore> OnDisk(); On 2015/02/23 20:17:15, Wez wrote: > nit: Add a comment to explain any specific properties of the implementation that > this method returns, e.g. how the on-disk path is chosen. Done.
https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:35: void SetAuthCodeError(bool fail) { On 2015/02/24 02:01:24, joedow wrote: > On 2015/02/23 20:17:15, Wez wrote: > > set_fail_access_token_from_auth_code(), to match the name of the member that > it > > sets. > > Done. No, I literally meant set_fail_access_token_from_auth_code() - style-guide has us use unix_style for simple setters/getters and CamelCase elsewhere, basically. :) https://codereview.chromium.org/880273006/diff/100001/remoting/test/app_remot... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/app_remot... remoting/test/app_remoting_test_driver_environment_unittest.cc:71: new FakeRefreshTokenStore()); You could just create these (and the mock above) on the stack now. https://codereview.chromium.org/880273006/diff/100001/remoting/test/fake_acce... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/fake_acce... remoting/test/fake_access_token_fetcher.h:30: nit: Suggest removing this blank line, so it's clear that the comment above applies to both of these overrides. https://codereview.chromium.org/880273006/diff/100001/remoting/test/mock_acce... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/mock_acce... remoting/test/mock_access_token_fetcher.h:34: // pass a fake access_token_fetcher object in but will also work for any Why access_token_fetcher? The class is called AccessTokenFetcher - access_token_fetcher would be a variable name, since it is unix_style. You probably mean "... to pass a FakeAccessTokenFetcher." (the rest of that line is redundant since I can see from the interface, and the previous line of the comment, that it accepts any AccessTokenFetcher. https://codereview.chromium.org/880273006/diff/100001/remoting/test/refresh_t... File remoting/test/refresh_token_store.h (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/refresh_t... remoting/test/refresh_token_store.h:29: // on the local disk. If needed, the file is created and has its permissions No it doesn't. ;) It returns an token store that reads/writes a user-specific token file. https://codereview.chromium.org/880273006/diff/100001/remoting/test/refresh_t... remoting/test/refresh_token_store.h:32: // multiple users can be stored on the same machine. Should |user_name| be a parameter to the store creation method, rather than to APIs? As it is it would be possible to use a single store to read/write tokens for multiple different users, which seems odd.
joedow@google.com changed reviewers: + joedow@google.com
https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/80001/remoting/test/fake_acces... remoting/test/fake_access_token_fetcher.h:35: void SetAuthCodeError(bool fail) { On 2015/02/24 03:10:19, Wez wrote: > On 2015/02/24 02:01:24, joedow wrote: > > On 2015/02/23 20:17:15, Wez wrote: > > > set_fail_access_token_from_auth_code(), to match the name of the member that > > it > > > sets. > > > > Done. > > No, I literally meant set_fail_access_token_from_auth_code() - style-guide has > us use unix_style for simple setters/getters and CamelCase elsewhere, basically. > :) Done. https://codereview.chromium.org/880273006/diff/100001/remoting/test/app_remot... File remoting/test/app_remoting_test_driver_environment_unittest.cc (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/app_remot... remoting/test/app_remoting_test_driver_environment_unittest.cc:71: new FakeRefreshTokenStore()); On 2015/02/24 03:10:19, Wez wrote: > You could just create these (and the mock above) on the stack now. Done. https://codereview.chromium.org/880273006/diff/100001/remoting/test/fake_acce... File remoting/test/fake_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/fake_acce... remoting/test/fake_access_token_fetcher.h:30: On 2015/02/24 03:10:19, Wez wrote: > nit: Suggest removing this blank line, so it's clear that the comment above > applies to both of these overrides. Done. https://codereview.chromium.org/880273006/diff/100001/remoting/test/mock_acce... File remoting/test/mock_access_token_fetcher.h (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/mock_acce... remoting/test/mock_access_token_fetcher.h:34: // pass a fake access_token_fetcher object in but will also work for any On 2015/02/24 03:10:19, Wez wrote: > Why access_token_fetcher? The class is called AccessTokenFetcher - > access_token_fetcher would be a variable name, since it is unix_style. You > probably mean "... to pass a FakeAccessTokenFetcher." (the rest of that line is > redundant since I can see from the interface, and the previous line of the > comment, that it accepts any AccessTokenFetcher. Done. https://codereview.chromium.org/880273006/diff/100001/remoting/test/refresh_t... File remoting/test/refresh_token_store.h (right): https://codereview.chromium.org/880273006/diff/100001/remoting/test/refresh_t... remoting/test/refresh_token_store.h:29: // on the local disk. If needed, the file is created and has its permissions On 2015/02/24 03:10:19, Wez wrote: > No it doesn't. ;) It returns an token store that reads/writes a user-specific > token file. Done. https://codereview.chromium.org/880273006/diff/100001/remoting/test/refresh_t... remoting/test/refresh_token_store.h:32: // multiple users can be stored on the same machine. On 2015/02/24 03:10:19, Wez wrote: > Should |user_name| be a parameter to the store creation method, rather than to > APIs? As it is it would be possible to use a single store to read/write tokens > for multiple different users, which seems odd. I think this is a by product of the original functions being converted to a class. I'll update it as I think it makes sense to create a user specific RefreshTokenStore using this method.
LGTM w/ the comment below addressed! https://codereview.chromium.org/880273006/diff/120001/remoting/test/refresh_t... File remoting/test/refresh_token_store.cc (right): https://codereview.chromium.org/880273006/diff/120001/remoting/test/refresh_t... remoting/test/refresh_token_store.cc:46: std::string user_name_; DISALLOW_COPY_AND_ASSIGN
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/880273006/#ps140001 (title: "Fixing final nit!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880273006/140001
The CQ bit was unchecked by joedow@chromium.org
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/880273006/#ps160001 (title: "Fixing vector initializer list syntax which was causing build failures on non C++11 compilers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880273006/160001
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/880273006/#ps180001 (title: "Fixing a Win platform only build issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880273006/180001
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/880273006/#ps200001 (title: "Fixing one last windows compile issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880273006/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ea77cec75493f936518fed723dcd6c371ad564d6 Cr-Commit-Position: refs/heads/master@{#318137} |
