Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954053002:
Add a unit test for translation event logging. (Closed)
|
Created:
3 years, 6 months ago by renjieliu1 Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a unit test for translation event logging.
BUG=728491
Review-Url: https://codereview.chromium.org/2954053002
Cr-Commit-Position: refs/heads/master@{#483565}
Committed: https://chromium.googlesource.com/chromium/src/+/61ffe40bc6dbafd8baebd66fc6737e8abee06036
Patch Set 1 #
Total comments: 8
Patch Set 2 : updates #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : rebase #
Messages
Total messages: 38 (21 generated)
The CQ bit was checked by renjieliu@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== add unit tests for translationeventlogging Add a unit test for translation event logging. BUG=728491 ========== to ========== add unit tests for translationeventlogging Add a unit test for translation event logging. BUG=728491 ==========
renjieliu@chromium.org changed reviewers: + napper@chromium.org
https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... File chrome/browser/translate/chrome_translate_client_unittest.cc (right): https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:96: scoped_feature_list_->InitAndEnableFeature( Can't you just do this once in SetUp()? https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:98: GURL url("http://yahoo.com"); const? https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:105: EXPECT_EQ(1ul, GetUserEventService()->GetRecordedUserEvents().size()); I think 1 instead of 1ul https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:106: } Can you also test the content of the event?
https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... File chrome/browser/translate/chrome_translate_client_unittest.cc (right): https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:96: scoped_feature_list_->InitAndEnableFeature( On 2017/06/26 00:32:04, napper wrote: > Can't you just do this once in SetUp()? Done. https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:98: GURL url("http://yahoo.com"); On 2017/06/26 00:32:04, napper wrote: > const? Done. https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:105: EXPECT_EQ(1ul, GetUserEventService()->GetRecordedUserEvents().size()); On 2017/06/26 00:32:04, napper wrote: > I think 1 instead of 1ul .size() returns a unsigned long and it seems EXPECT_EQ does not handle implicit conversion :( https://codereview.chromium.org/2954053002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client_unittest.cc:106: } On 2017/06/26 00:32:04, napper wrote: > Can you also test the content of the event? Done.
lgtm
renjieliu@chromium.org changed reviewers: + mgiuca@chromium.org
Add Matt for chrome committer review. Thank you!
The CL description doesn't need to duplicate the same line twice. (Don't think of it as a separate "title" and "description". Think of it as a description whose first line is also the title.) https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client_unittest.cc (right): https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:26: using ::metrics::TranslateEventProto; I don't think it's worth using this for just 2 mentions in the whole file. Can you just fully qualify the name below? https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:99: // Event we care. What does this comment mean? "An event we care about." maybe? https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:119: // Event we don't care. Same.
thanks for the review! https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client_unittest.cc (right): https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:26: using ::metrics::TranslateEventProto; On 2017/06/29 00:26:51, Matt Giuca wrote: > I don't think it's worth using this for just 2 mentions in the whole file. Can > you just fully qualify the name below? Done. https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:99: // Event we care. On 2017/06/29 00:26:51, Matt Giuca wrote: > What does this comment mean? "An event we care about." maybe? good suggestion! https://codereview.chromium.org/2954053002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:119: // Event we don't care. On 2017/06/29 00:26:51, Matt Giuca wrote: > Same. Done.
See my comment about the CL description above.
Description was changed from ========== add unit tests for translationeventlogging Add a unit test for translation event logging. BUG=728491 ========== to ========== Add a unit test for translation event logging. BUG=728491 ==========
On 2017/06/29 01:27:23, Matt Giuca wrote: > See my comment about the CL description above. sorry I missed that, update the description to get rid of the duplicate line
On 2017/06/29 01:36:29, renjieliu1 wrote: > On 2017/06/29 01:27:23, Matt Giuca wrote: > > See my comment about the CL description above. > > sorry I missed that, update the description to get rid of the duplicate line You should edit the title to match the first line of the description. (I've just done this for you.) PS. Use git cl upload --gerrit from now on; among other things it means we can write code review comments on the CL description! lgtm
On 2017/06/29 01:38:37, Matt Giuca wrote: > On 2017/06/29 01:36:29, renjieliu1 wrote: > > On 2017/06/29 01:27:23, Matt Giuca wrote: > > > See my comment about the CL description above. > > > > sorry I missed that, update the description to get rid of the duplicate line > > You should edit the title to match the first line of the description. (I've just > done this for you.) > > PS. Use git cl upload --gerrit from now on; among other things it means we can > write code review comments on the CL description! > > lgtm Thank you! We will use gerrit instead!
The CQ bit was checked by renjieliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org Link to the patchset: https://codereview.chromium.org/2954053002/#ps40001 (title: "rebase")
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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by renjieliu@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by renjieliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2954053002/#ps60001 (title: "rebase")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by renjieliu@chromium.org
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": 1498783530477320,
"parent_rev": "d86871c7511c6a2f76468afb953c6219c7083097", "commit_rev":
"61ffe40bc6dbafd8baebd66fc6737e8abee06036"}
Message was sent while issue was closed.
Description was changed from ========== Add a unit test for translation event logging. BUG=728491 ========== to ========== Add a unit test for translation event logging. BUG=728491 Review-Url: https://codereview.chromium.org/2954053002 Cr-Commit-Position: refs/heads/master@{#483565} Committed: https://chromium.googlesource.com/chromium/src/+/61ffe40bc6dbafd8baebd66fc673... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/61ffe40bc6dbafd8baebd66fc673... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
