Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451213003:
Demonstrates example when GetTotalCountsForPrefix returns true for the suffix. (Closed)
|
Created:
4 years, 1 month ago by melandory Modified:
4 years, 1 month ago Reviewers:
Paweł Hajdan Jr. CC:
chromium-reviews, engedy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDemonstrates example when GetTotalCountsForPrefix returns ture for the suffix.
BUG=659977
Committed: https://crrev.com/5d97a507995f0a7945123b6d78c566c27567264a
Cr-Commit-Position: refs/heads/master@{#428307}
Patch Set 1 : Demonstrates that test fails #Patch Set 2 : Disables the test #
Total comments: 2
Messages
Total messages: 19 (12 generated)
The CQ bit was checked by melandory@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...
Description was changed from ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 ========== to ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 ==========
melandory@chromium.org changed reviewers: + phajdan.jr@chromium.org
PTAL, thanks!
PTAL, thanks!
The CQ bit was checked by melandory@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.
LGTM w/an optional suggestion https://codereview.chromium.org/2451213003/diff/20001/base/test/histogram_tes... File base/test/histogram_tester_unittest.cc (right): https://codereview.chromium.org/2451213003/diff/20001/base/test/histogram_tes... base/test/histogram_tester_unittest.cc:124: UMA_HISTOGRAM_ENUMERATION(kHistogramSuffix, 2, 5); nit: Is it worth extracting this as a constant? In a way this obscures what we're testing here. Other tests do this when they reference the same string multiple times in an obvious pattern. Just a suggestion though, I'd be fine with this code as is.
https://codereview.chromium.org/2451213003/diff/20001/base/test/histogram_tes... File base/test/histogram_tester_unittest.cc (right): https://codereview.chromium.org/2451213003/diff/20001/base/test/histogram_tes... base/test/histogram_tester_unittest.cc:124: UMA_HISTOGRAM_ENUMERATION(kHistogramSuffix, 2, 5); On 2016/10/27 16:55:51, Paweł Hajdan Jr. wrote: > nit: Is it worth extracting this as a constant? In a way this obscures what > we're testing here. > > Other tests do this when they reference the same string multiple times in an > obvious pattern. > > Just a suggestion though, I'd be fine with this code as is. Funnily, in-lining Test4.Test5 triggers the presubmit warning: Some UMA_HISTOGRAM lines have been modified and the associated histogram name has no match in either tools/metrics/histograms/histograms.xml or the modifications of it: [base/test/histogram_tester_unittest.cc:123] Test4.Test5
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 ========== to ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 ========== to ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 Committed: https://crrev.com/5d97a507995f0a7945123b6d78c566c27567264a Cr-Commit-Position: refs/heads/master@{#428307} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5d97a507995f0a7945123b6d78c566c27567264a Cr-Commit-Position: refs/heads/master@{#428307}
Message was sent while issue was closed.
Description was changed from ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 Committed: https://crrev.com/5d97a507995f0a7945123b6d78c566c27567264a Cr-Commit-Position: refs/heads/master@{#428307} ========== to ========== Demonstrates example when GetTotalCountsForPrefix returns ture for the suffix. BUG=659977 Committed: https://crrev.com/5d97a507995f0a7945123b6d78c566c27567264a Cr-Commit-Position: refs/heads/master@{#428307} ========== |
