Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672253003:
[Remote fetcher] Add unit-tests for the authenticated case (Closed)
|
Created:
3 years, 10 months ago by jkrcal Modified:
3 years, 10 months ago Reviewers:
Marc Treib CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Remote fetcher] Add unit-tests for the authenticated case
This CL adds basic tests for authenticated fetches of
remote_suggestions_fetcher.
BUG=688310
Review-Url: https://codereview.chromium.org/2672253003
Cr-Commit-Position: refs/heads/master@{#448581}
Committed: https://chromium.googlesource.com/chromium/src/+/70957b2671edd5d21ec7b8a6cd36c0cc1f92df99
Patch Set 1 #Patch Set 2 : Cleaning up DLOGs #
Total comments: 19
Patch Set 3 : Marc's comments and a bit of renaming #
Total comments: 8
Patch Set 4 : Marc's comment #2 #Messages
Total messages: 17 (9 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, could you PTAL? This does not cover the "authentication in progress" case as it requires more changes (instead FakeSigninManagerBase use FakeSigninManager which does not exist on ChromeOS).
On 2017/02/06 09:47:38, jkrcal wrote: > Marc, could you PTAL? > > This does not cover the "authentication in progress" case as it requires more > changes (instead FakeSigninManagerBase use FakeSigninManager which does not > exist on ChromeOS). I've recently learned that the AuthInProgress state doesn't actually exist on Android, so this seems fine. I'm looking at the code now.
https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc (right): https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:60: const char kTestChromeContentSuggestionsUrl[] = nit: Maybe rename this to ...SignedOutUrl to make the difference clear? https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:300: new net::TestURLRequestContextGetter(mock_task_runner_.get()); Why do we create another request context getter here? Since it's refcounted, can't we use the same one? Also, looks like we don't actually need it as a member? https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:302: new FakeOAuth2TokenServiceDelegate(request_context_getter_.get()); This looks like a memleak? https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:330: void SignIn() { nitty nit: Put this before IssueRefreshToken? That would correspond better to the actual order of events. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:374: void SetFakeResponse(const std::string& test_url, Should this be a GURL? https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:421: : RemoteSuggestionsFetcherTestBase() {} Possible alternative: Keep the TestUrl in the ctor, but make another test class ContentSuggestionsFetcherSignedInTest. WDYT? https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:508: const std::string kJsonStr = Completely orthogonal to this CL, but at some point we should use raw string literals throughout this file. That makes the inline json string SO much easier to read. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:537: FastForwardUntilNoTasksRemain(); This is necessary for the fake response, right? Worth a comment maybe? https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:550: ShouldFetchSuccessfullyWhenSignedInAndOAuthCancelled) { This really tests the retry, correct? Maybe something like "ShouldRetryWhenSignedInAndOAuthCancelled"?
Thanks, PTAL, again! https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc (right): https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:60: const char kTestChromeContentSuggestionsUrl[] = On 2017/02/06 11:03:17, Marc Treib wrote: > nit: Maybe rename this to ...SignedOutUrl to make the difference clear? Done. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:300: new net::TestURLRequestContextGetter(mock_task_runner_.get()); On 2017/02/06 11:03:17, Marc Treib wrote: > Why do we create another request context getter here? Since it's refcounted, > can't we use the same one? > Also, looks like we don't actually need it as a member? Done. Sorry, this was a rest from my experiments how to make the signed-in flow work. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:302: new FakeOAuth2TokenServiceDelegate(request_context_getter_.get()); On 2017/02/06 11:03:17, Marc Treib wrote: > This looks like a memleak? It does but it is not. Added a comment to explain. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:330: void SignIn() { On 2017/02/06 11:03:17, Marc Treib wrote: > nitty nit: Put this before IssueRefreshToken? That would correspond better to > the actual order of events. Done. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:374: void SetFakeResponse(const std::string& test_url, On 2017/02/06 11:03:17, Marc Treib wrote: > Should this be a GURL? Removed (see below). https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:421: : RemoteSuggestionsFetcherTestBase() {} On 2017/02/06 11:03:17, Marc Treib wrote: > Possible alternative: Keep the TestUrl in the ctor, but make another test class > ContentSuggestionsFetcherSignedInTest. WDYT? Done. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:508: const std::string kJsonStr = On 2017/02/06 11:03:17, Marc Treib wrote: > Completely orthogonal to this CL, but at some point we should use raw string > literals throughout this file. That makes the inline json string SO much easier > to read. Acknowledged. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:537: FastForwardUntilNoTasksRemain(); On 2017/02/06 11:03:17, Marc Treib wrote: > This is necessary for the fake response, right? Worth a comment maybe? Done. https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:550: ShouldFetchSuccessfullyWhenSignedInAndOAuthCancelled) { On 2017/02/06 11:03:17, Marc Treib wrote: > This really tests the retry, correct? Maybe something like > "ShouldRetryWhenSignedInAndOAuthCancelled"? Done. https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc (right): https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:268: base::Bind(&ParseJson, json, std::move(success_callback), I did many find-and-replace changes so I used clang formatter for the whole file. https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:296: void ResetFetcher() { A bit non-conceptional: I took the chance to remove "Snippets" from this file. https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:402: class RemoteSuggestionsChromeReaderFetcherTest When creating a new class, I took the chance to rename existing ones to make them more consistent with the base class.
Thanks! LGTM with one comment nit. For bonus brownie points, next time try uploading the renaming/reformatting in a separate patch set. That makes reviewing the actual changes much easier. (But don't worry too much about it; I know from personal experience that a separate patch set isn't always feasible.) https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc (right): https://codereview.chromium.org/2672253003/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:302: new FakeOAuth2TokenServiceDelegate(request_context_getter_.get()); On 2017/02/07 08:44:18, jkrcal wrote: > On 2017/02/06 11:03:17, Marc Treib wrote: > > This looks like a memleak? > > It does but it is not. Added a comment to explain. Acknowledged. https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc (right): https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:268: base::Bind(&ParseJson, json, std::move(success_callback), On 2017/02/07 08:44:18, jkrcal wrote: > I did many find-and-replace changes so I used clang formatter for the whole > file. Acknowledged. https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:276: // sign-in / refresh tokens / access token code). crbug.com/688310 I guess this comment can be removed now :) (or maybe replaced by a more specific one, if you think some important tests are still missing) https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:296: void ResetFetcher() { On 2017/02/07 08:44:18, jkrcal wrote: > A bit non-conceptional: I took the chance to remove "Snippets" from this file. Yay! https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:402: class RemoteSuggestionsChromeReaderFetcherTest On 2017/02/07 08:44:18, jkrcal wrote: > When creating a new class, I took the chance to rename existing ones to make > them more consistent with the base class. Acknowledged.
Thanks and sorry for mixing the patch sets! https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc (right): https://codereview.chromium.org/2672253003/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_fetcher_unittest.cc:276: // sign-in / refresh tokens / access token code). crbug.com/688310 On 2017/02/07 09:18:12, Marc Treib wrote: > I guess this comment can be removed now :) (or maybe replaced by a more > specific one, if you think some important tests are still missing) Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2672253003/#ps60001 (title: "Marc's comment #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486462117391190,
"parent_rev": "13452f1a1149352dc13b53027bbceac54ed217e6", "commit_rev":
"70957b2671edd5d21ec7b8a6cd36c0cc1f92df99"}
Message was sent while issue was closed.
Description was changed from ========== [Remote fetcher] Add unit-tests for the authenticated case This CL adds basic tests for authenticated fetches of remote_suggestions_fetcher. BUG=688310 ========== to ========== [Remote fetcher] Add unit-tests for the authenticated case This CL adds basic tests for authenticated fetches of remote_suggestions_fetcher. BUG=688310 Review-Url: https://codereview.chromium.org/2672253003 Cr-Commit-Position: refs/heads/master@{#448581} Committed: https://chromium.googlesource.com/chromium/src/+/70957b2671edd5d21ec7b8a6cd36... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/70957b2671edd5d21ec7b8a6cd36... |
