Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655353005:
[Sync] Add autocomplete unittests for GetClientTag. (Closed)
|
Created:
3 years, 10 months ago by skym Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Add autocomplete unittests for GetClientTag.
BUG=675991
Review-Url: https://codereview.chromium.org/2655353005
Cr-Commit-Position: refs/heads/master@{#447336}
Committed: https://chromium.googlesource.com/chromium/src/+/0da7a38852b601f5964d44a374f86ea0b30b6ef0
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update for Patrick's offline comment. #Patch Set 3 : More updates for Patrick. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (16 generated)
The CQ bit was checked by skym@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...
skym@chromium.org changed reviewers: + pnoland@chromium.org
PTAL Painfully similar to the GetStorageKey tests. I decided to keep them separate so regression causes would be more obvious from the cases that fail.
The CQ bit was checked by skym@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...
lgtm % comments https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:297: TEST_F(AutocompleteSyncBridgeTest, GetClientTagTimestmap) { Can you give this a more descriptive name or add a comment? https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:312: TEST_F(AutocompleteSyncBridgeTest, GetClientTagNull) { You can probably incorporate the comment into the test name https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:323: EXPECT_EQ("autofill_entry|name%201|value%201", Can you make these strings variables with names that describe what they are?
The CQ bit was checked by skym@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...
skym@chromium.org changed reviewers: + mathp@google.com
Updates for Patrick. Adding mathp@ for owners approval, PTAL. https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:297: TEST_F(AutocompleteSyncBridgeTest, GetClientTagTimestmap) { On 2017/01/27 18:19:23, Patrick Noland wrote: > Can you give this a more descriptive name or add a comment? Done. https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:312: TEST_F(AutocompleteSyncBridgeTest, GetClientTagNull) { On 2017/01/27 18:19:23, Patrick Noland wrote: > You can probably incorporate the comment into the test name Done. https://codereview.chromium.org/2655353005/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:323: EXPECT_EQ("autofill_entry|name%201|value%201", On 2017/01/27 18:19:23, Patrick Noland wrote: > Can you make these strings variables with names that describe what they are? Per offline discussion, expanded upon the comment to give more context.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Sync] Add autocomplete unittests for GetClientTag. BUG=675991 ========== to ========== [Sync] Add autocomplete unittests for GetClientTag. BUG=675991 ==========
mathp@chromium.org changed reviewers: + mathp@chromium.org - mathp@google.com
lgtm
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pnoland@chromium.org Link to the patchset: https://codereview.chromium.org/2655353005/#ps40001 (title: "More updates for Patrick.")
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": 40001, "attempt_start_ts": 1485895478059790,
"parent_rev": "9eacb4506c2ab9569430df9f2450a8f695ef9c4d", "commit_rev":
"0da7a38852b601f5964d44a374f86ea0b30b6ef0"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Add autocomplete unittests for GetClientTag. BUG=675991 ========== to ========== [Sync] Add autocomplete unittests for GetClientTag. BUG=675991 Review-Url: https://codereview.chromium.org/2655353005 Cr-Commit-Position: refs/heads/master@{#447336} Committed: https://chromium.googlesource.com/chromium/src/+/0da7a38852b601f5964d44a374f8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0da7a38852b601f5964d44a374f8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
