Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2761313002:
[User classifier] Add a unit-test (Closed)
|
Created:
3 years, 9 months ago by jkrcal Modified:
3 years, 9 months ago Reviewers:
Marc Treib CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[User classifier] Add a unit-test
This CL adds basic test support for UserClassifier. More advance tests
may follow.
BUG=699027
Review-Url: https://codereview.chromium.org/2761313002
Cr-Commit-Position: refs/heads/master@{#458701}
Committed: https://chromium.googlesource.com/chromium/src/+/afbbf48b9c592f0e857bb5bba9179c925b24bf01
Patch Set 1 #
Total comments: 8
Patch Set 2 : Marc's comment #
Total comments: 6
Patch Set 3 : Marc's comments #2 #
Depends on Patchset: Messages
Total messages: 20 (13 generated)
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...
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, could you PTAL?
Very nice! Just some small comments. https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier_unittest.cc (right): https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:38: test_clock_ = test_clock.get(); This is dangerous: It hands out the UserClassifier which owns the clock, but keeps a raw pointer to the clock around. If the UserClassifier gets destroyed, you have a dangling pointer. Could the test class retain ownership of the UserClassifier? If that's not possible, maybe pass the clock pointer out through an out param? https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:71: // One more click to become an active consumer. Do we really want to test that the *second* click already puts you into the ACTIVE_SUGGESTIONS_CONSUMER bucket? https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:210: // After 30 minutes, it gets updated. ...I don't get this one. What's going on here? Maybe the test just needs a more descriptive name? https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:292: , What's this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! PTAL, again. https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... File components/ntp_snippets/user_classifier_unittest.cc (right): https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:38: test_clock_ = test_clock.get(); On 2017/03/21 15:15:45, Marc Treib wrote: > This is dangerous: It hands out the UserClassifier which owns the clock, but > keeps a raw pointer to the clock around. If the UserClassifier gets destroyed, > you have a dangling pointer. > > Could the test class retain ownership of the UserClassifier? If that's not > possible, maybe pass the clock pointer out through an out param? Fixed - the classifier is now owned by the test class so that the tests themselves cannot get the dangling pointer without knowing. https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:71: // One more click to become an active consumer. On 2017/03/21 15:15:46, Marc Treib wrote: > Do we really want to test that the *second* click already puts you into the > ACTIVE_SUGGESTIONS_CONSUMER bucket? Made it a bit more relaxed. https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:210: // After 30 minutes, it gets updated. On 2017/03/21 15:15:46, Marc Treib wrote: > ...I don't get this one. What's going on here? > Maybe the test just needs a more descriptive name? Done. https://codereview.chromium.org/2761313002/diff/1/components/ntp_snippets/use... components/ntp_snippets/user_classifier_unittest.cc:292: , On 2017/03/21 15:15:45, Marc Treib wrote: > What's this? This is a prefix for instantiating the tests. An empty prefix, in particular :) I added a comment.
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...
Thanks! LGTM with some optional comments/nits. https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier_unittest.cc (right): https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier_unittest.cc:6: nitty nit: missing some std includes, like memory, string, utility https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier_unittest.cc:52: base::SimpleTestClock* test_clock_; // Owned by the UserClassifier. ? https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier_unittest.cc:53: std::unique_ptr<UserClassifier> user_classifier_; Could all these be private? (And instead have public getters if necessary)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier_unittest.cc (right): https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier_unittest.cc:6: On 2017/03/21 17:49:43, Marc Treib wrote: > nitty nit: missing some std includes, like memory, string, utility Done. https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier_unittest.cc:52: base::SimpleTestClock* test_clock_; On 2017/03/21 17:49:43, Marc Treib wrote: > // Owned by the UserClassifier. > ? Done. https://codereview.chromium.org/2761313002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier_unittest.cc:53: std::unique_ptr<UserClassifier> user_classifier_; On 2017/03/21 17:49:43, Marc Treib wrote: > Could all these be private? (And instead have public getters if necessary) Done.
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/2761313002/#ps40001 (title: "Marc's comments #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": 40001, "attempt_start_ts": 1490172360059060,
"parent_rev": "f9428b42fa8f7d0dee29fdc5bcad0d8fd695adc8", "commit_rev":
"afbbf48b9c592f0e857bb5bba9179c925b24bf01"}
Message was sent while issue was closed.
Description was changed from ========== [User classifier] Add a unit-test This CL adds basic test support for UserClassifier. More advance tests may follow. BUG=699027 ========== to ========== [User classifier] Add a unit-test This CL adds basic test support for UserClassifier. More advance tests may follow. BUG=699027 Review-Url: https://codereview.chromium.org/2761313002 Cr-Commit-Position: refs/heads/master@{#458701} Committed: https://chromium.googlesource.com/chromium/src/+/afbbf48b9c592f0e857bb5bba917... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/afbbf48b9c592f0e857bb5bba917... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
