Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947853002:
[signin] Unit test for DiceResponseHandler (Closed)
Description[signin] Unit test for DiceResponseHandler
BUG=730589
Review-Url: https://codereview.chromium.org/2947853002
Cr-Commit-Position: refs/heads/master@{#481864}
Committed: https://chromium.googlesource.com/chromium/src/+/2290d78497eb6de3d0614ef252daebdf98855167
Patch Set 1 #
Total comments: 13
Patch Set 2 : Review comments #Patch Set 3 : Add missing destructor #Patch Set 4 : Fix memory leak #
Depends on Patchset: Messages
Total messages: 23 (14 generated)
Description was changed from ========== cleanup no fake working test instantiate the thing add more stuff add unittest BUG= ========== to ========== [signin] Unit test for DiceResponseHandler BUG=730589 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
droger@chromium.org changed reviewers: + msarda@chromium.org
https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... File chrome/browser/signin/dice_response_handler_unittest.cc (right): https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:31: const char kObfuscatedGaiaID[] = "obfuscated_gaia_id"; I think we can drop the obfuscated from the Gaia ID - all Gaia IDs are obfuscated in Chrome. https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:41: std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher( Should this be override? https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:48: // Pass |this| as a dummy consumer. Drop everything on the floor. I do not understand this comment - this method is not returning |this|. https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:57: DiceResponseHandlerTest() I think in general we prefer to do the initialization in SetUp and the tear down in TearDown, not in the constructor. Is there a reason for doing them in the constructor? https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:79: dice_params.obfuscated_gaia_id = kObfuscatedGaiaID; Same remark as above - the Gaia ID in Chrome is always obfuscated - Gaia never shares the non-obfuscated ID with clients. Let's rename this to gaia_id to avoid any confusion in the future. https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:86: base::MessageLoop loop_; Is the message loop needed? https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:105: signin_client_.consumer_->OnClientOAuthSuccess( Please also add a test for OnClientOAuthFailure.
I forgot the LGTM. LGTM with some comments. Please address before landing.
Thanks. https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... File chrome/browser/signin/dice_response_handler_unittest.cc (right): https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:31: const char kObfuscatedGaiaID[] = "obfuscated_gaia_id"; On 2017/06/22 09:29:48, msarda wrote: > I think we can drop the obfuscated from the Gaia ID - all Gaia IDs are > obfuscated in Chrome. SGTM. I'm doing this in another CL: https://codereview.chromium.org/2944383006 https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:41: std::unique_ptr<GaiaAuthFetcher> CreateGaiaAuthFetcher( On 2017/06/22 09:29:48, msarda wrote: > Should this be override? Yes, it is override. Did you miss it on line 44? https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:48: // Pass |this| as a dummy consumer. Drop everything on the floor. On 2017/06/22 09:29:48, msarda wrote: > I do not understand this comment - this method is not returning |this|. I am passing |this| to CreateGaiaAuthFetcher instead of the original consumer. Clarified the comment. https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:57: DiceResponseHandlerTest() On 2017/06/22 09:29:47, msarda wrote: > I think in general we prefer to do the initialization in SetUp and the tear down > in TearDown, not in the constructor. Is there a reason for doing them in the > constructor? This is the other way around. Initialization in SetUp is discouraged. See: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:86: base::MessageLoop loop_; On 2017/06/22 09:29:48, msarda wrote: > Is the message loop needed? Yes, GaiaAuthFetcher requires it. Removing it gives a crash: [38249:38249:0622/151854.856368:2089634038704:FATAL:sequenced_task_runner_handle.cc(54)] Check failed: pool. #0 0x7f2a570d454b base::debug::StackTrace::StackTrace() #1 0x7f2a570d324c base::debug::StackTrace::StackTrace() #2 0x7f2a57147703 logging::LogMessage::~LogMessage() #3 0x7f2a572bb587 base::SequencedTaskRunnerHandle::Get() #4 0x7f2a5a90549b net::URLFetcherCore::URLFetcherCore() #5 0x7f2a5a91415a net::URLFetcherImpl::URLFetcherImpl() #6 0x7f2a5a904a6a net::URLFetcher::Create() #7 0x0000022fab5b GaiaAuthFetcher::CreateAndStartGaiaFetcher() https://codereview.chromium.org/2947853002/diff/40001/chrome/browser/signin/d... chrome/browser/signin/dice_response_handler_unittest.cc:105: signin_client_.consumer_->OnClientOAuthSuccess( On 2017/06/22 09:29:47, msarda wrote: > Please also add a test for OnClientOAuthFailure. Doing this in another CL because then I can use the GetPendingDiceTokenFetchersCountForTesting() function for that test.
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2947853002/#ps60001 (title: "Review comments")
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 droger@chromium.org
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2947853002/#ps80001 (title: "Add missing destructor")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2947853002/#ps100001 (title: "Fix memory leak")
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": 100001, "attempt_start_ts": 1498216610168940,
"parent_rev": "4e46f03a4ddae8ac970c9e8fadba8dce49931e49", "commit_rev":
"2290d78497eb6de3d0614ef252daebdf98855167"}
Message was sent while issue was closed.
Description was changed from ========== [signin] Unit test for DiceResponseHandler BUG=730589 ========== to ========== [signin] Unit test for DiceResponseHandler BUG=730589 Review-Url: https://codereview.chromium.org/2947853002 Cr-Commit-Position: refs/heads/master@{#481864} Committed: https://chromium.googlesource.com/chromium/src/+/2290d78497eb6de3d0614ef252da... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2290d78497eb6de3d0614ef252da... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
