Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660253003:
Unit test that DnsSocketPool copies RandIntCallback (Closed)
|
Created:
3 years, 10 months ago by Julia Tuttle Modified:
3 years, 10 months ago Reviewers:
mmenke CC:
cbentzel+watch_chromium.org, chromium-reviews, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnit test that DnsSocketPool copies RandIntCallback
BUG=686659
Review-Url: https://codereview.chromium.org/2660253003
Cr-Commit-Position: refs/heads/master@{#447250}
Committed: https://chromium.googlesource.com/chromium/src/+/8ff9d153a2aee2f4a7acac6910a97fade45a54c8
Patch Set 1 #
Total comments: 16
Patch Set 2 : Make requested changes. #
Total comments: 5
Patch Set 3 : Make requested changes. #
Total comments: 2
Patch Set 4 : Make requested change #Patch Set 5 : Fix DummyRandIntCallback tests #
Messages
Total messages: 38 (23 generated)
The CQ bit was checked by juliatuttle@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...
juliatuttle@chromium.org changed reviewers: + mmenke@chromium.org
PTAL, mmenke.
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:44: }; Is all this needed, or does the following work: int SillyRandCallback(int foo) { return foo; } // Could put these in the test fixture as well. int foo = 4; base::WeakPtrFactory<int> weak_int_factory(&foo); pool_ = DnsSocketPool::CreateDefault( ClientSocketFactory::GetDefaultFactory(), // Again, could make this a method in the test fixture base::Bind(&SillyRandCallback, weak_int_factory.GetWeakPtr())); EXPECT_TRUE(weak_int_factory_.HasWeakPtrs());
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:44: }; On 2017/01/30 18:18:44, mmenke (Out Feb 4 to March 5) wrote: > Is all this needed, or does the following work: > > int SillyRandCallback(int foo) { > return foo; > } > > // Could put these in the test fixture as well. > int foo = 4; > base::WeakPtrFactory<int> weak_int_factory(&foo); > pool_ = DnsSocketPool::CreateDefault( > ClientSocketFactory::GetDefaultFactory(), > // Again, could make this a method in the test fixture > base::Bind(&SillyRandCallback, weak_int_factory.GetWeakPtr())); > > EXPECT_TRUE(weak_int_factory_.HasWeakPtrs()); Actually, would probably have to be "int SillyRandCallback(int* foo)"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:44: }; On 2017/01/30 18:25:03, mmenke (Out Feb 4 to March 5) wrote: > On 2017/01/30 18:18:44, mmenke (Out Feb 4 to March 5) wrote: > > Is all this needed, or does the following work: > > > > int SillyRandCallback(int foo) { > > return foo; > > } > > > > // Could put these in the test fixture as well. > > int foo = 4; > > base::WeakPtrFactory<int> weak_int_factory(&foo); > > pool_ = DnsSocketPool::CreateDefault( > > ClientSocketFactory::GetDefaultFactory(), > > // Again, could make this a method in the test fixture > > base::Bind(&SillyRandCallback, weak_int_factory.GetWeakPtr())); > > > > EXPECT_TRUE(weak_int_factory_.HasWeakPtrs()); > > Actually, would probably have to be "int SillyRandCallback(int* foo)" It's not *needed*, but I think it's clearer to encapsulate things so it's plain to see that I'm creating a callback and then checking if there are refs to it. The "put a weak pointer into the callback args" thing is kind of magic, and would clutter the test cases themselves.
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:26: base::WeakPtrFactory<DummyObject> weak_factory_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:40: // guaranteed to be random. nit: Chosen / Guaranteed. I'd also suggest putting them before the return, rather than after it, as that's the more common style, particularly for multi-line comments, but that's up to you. https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:43: DummyObject dummy_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:46: TEST(DummyRandIntCallbackTest, Referenced) { I think that all 4 tests are sufficiently weird that they need comments (I'd say don't bother with the Referenced and Copied tests, since they're just checking that callbacks work as specced, and that should be tested in base, not in net/). https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:51: EXPECT_EQ(4, reference.Run(0, 6)); EXPECT_TRUE(dummy.HasRefs());?
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:26: base::WeakPtrFactory<DummyObject> weak_factory_; On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:40: // guaranteed to be random. On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > nit: Chosen / Guaranteed. > > I'd also suggest putting them before the return, rather than after it, as that's > the more common style, particularly for multi-line comments, but that's up to > you. It's acceptable per Randall Munroe: https://xkcd.com/221/ As President of the Internet, I believe he outranks our style guide. https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:43: DummyObject dummy_; On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:46: TEST(DummyRandIntCallbackTest, Referenced) { On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > I think that all 4 tests are sufficiently weird that they need comments (I'd say > don't bother with the Referenced and Copied tests, since they're just checking > that callbacks work as specced, and that should be tested in base, not in net/). I'll comment them. The first two are mostly to ensure that my assumptions about callbacks are correct, and demonstrate how the test class is supposed to work. https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:51: EXPECT_EQ(4, reference.Run(0, 6)); On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > EXPECT_TRUE(dummy.HasRefs());? Oh, sure.
The CQ bit was checked by juliatuttle@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...
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:40: // guaranteed to be random. On 2017/01/30 19:35:04, Julia Tuttle wrote: > On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > > nit: Chosen / Guaranteed. > > > > I'd also suggest putting them before the return, rather than after it, as > that's > > the more common style, particularly for multi-line comments, but that's up to > > you. > > It's acceptable per Randall Munroe: > > https://xkcd.com/221/ > > As President of the Internet, I believe he outranks our style guide. I'm pretty sure the very name "xkcd" violates the style guide. https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:44: // guaranteed to be random. I'd rather remove this, then - jokes not following the style guide seem better than no jokes, honestly (And it does require correct grammar and punctuation - which includes capitalizing sentences) https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:52: TEST(DummyRandIntCallbackTest, Referenced) { Comment these? Looking at them, it's rather non-obvious why they exist.
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_...)
The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run
https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:44: // guaranteed to be random. On 2017/01/30 19:49:30, mmenke (Out Feb 4 to March 5) wrote: > I'd rather remove this, then - jokes not following the style guide seem better > than no jokes, honestly (And it does require correct grammar and punctuation - > which includes capitalizing sentences) Fiiine, I'll follow the style guide for it. :) https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:52: TEST(DummyRandIntCallbackTest, Referenced) { On 2017/01/30 19:49:30, mmenke (Out Feb 4 to March 5) wrote: > Comment these? Looking at them, it's rather non-obvious why they exist. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM! https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/20001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:44: // guaranteed to be random. On 2017/01/30 20:10:52, Julia Tuttle wrote: > On 2017/01/30 19:49:30, mmenke (Out Feb 4 to March 5) wrote: > > I'd rather remove this, then - jokes not following the style guide seem better > > than no jokes, honestly (And it does require correct grammar and punctuation - > > which includes capitalizing sentences) > > Fiiine, I'll follow the style guide for it. :) Sorry to be so fixated on minor details, I've just noticed that comment styles tend to viral. :) https://codereview.chromium.org/2660253003/diff/40001/net/dns/dns_socket_pool... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/40001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:52: // Before the below tests rely upon it, make sure that DummyRandIntCallback nit: Before -> Because? Tests aren't guaranteed to run in order on the bots, I believe?
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:40: // guaranteed to be random. On 2017/01/30 at 19:49:30, mmenke (Out Feb 4 to March 5) wrote: > On 2017/01/30 19:35:04, Julia Tuttle wrote: > > On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > > > nit: Chosen / Guaranteed. > > > > > > I'd also suggest putting them before the return, rather than after it, as > > that's > > > the more common style, particularly for multi-line comments, but that's up to > > > you. > > > > It's acceptable per Randall Munroe: > > > > https://xkcd.com/221/ > > > > As President of the Internet, I believe he outranks our style guide. > > I'm pretty sure the very name "xkcd" violates the style guide. http://dilbert.com/strip/2001-10-25 mandates that the random number be NINE. NINE can be #defined to be 4 though.
https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/1/net/dns/dns_socket_pool_uni... net/dns/dns_socket_pool_unittest.cc:40: // guaranteed to be random. On 2017/01/30 20:17:18, asanka wrote: > On 2017/01/30 at 19:49:30, mmenke (Out Feb 4 to March 5) wrote: > > On 2017/01/30 19:35:04, Julia Tuttle wrote: > > > On 2017/01/30 19:14:42, mmenke (Out Feb 4 to March 5) wrote: > > > > nit: Chosen / Guaranteed. > > > > > > > > I'd also suggest putting them before the return, rather than after it, as > > > that's > > > > the more common style, particularly for multi-line comments, but that's up > to > > > > you. > > > > > > It's acceptable per Randall Munroe: > > > > > > https://xkcd.com/221/ > > > > > > As President of the Internet, I believe he outranks our style guide. > > > > I'm pretty sure the very name "xkcd" violates the style guide. > > http://dilbert.com/strip/2001-10-25 mandates that the random number be NINE. > NINE can be #defined to be 4 though. Only if it's a sufficiently large 4, though.
The CQ bit was checked by juliatuttle@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...
On 2017/01/30 20:17:18, asanka wrote: > http://dilbert.com/strip/2001-10-25 mandates that the random number be NINE. > NINE can be #defined to be 4 though. I'm gonna stick with 4: https://en.wikipedia.org/wiki/Scott_Adams#Controversies
https://codereview.chromium.org/2660253003/diff/40001/net/dns/dns_socket_pool... File net/dns/dns_socket_pool_unittest.cc (right): https://codereview.chromium.org/2660253003/diff/40001/net/dns/dns_socket_pool... net/dns/dns_socket_pool_unittest.cc:52: // Before the below tests rely upon it, make sure that DummyRandIntCallback On 2017/01/30 20:15:29, mmenke (Out Feb 4 to March 5) wrote: > nit: Before -> Because? Tests aren't guaranteed to run in order on the bots, I > believe? Done.
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_...) 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 juliatuttle@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.
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2660253003/#ps80001 (title: "Fix DummyRandIntCallback tests")
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": 80001, "attempt_start_ts": 1485881984802820,
"parent_rev": "2f0183c3f049882c0de912cf468d01e8f79790ce", "commit_rev":
"8ff9d153a2aee2f4a7acac6910a97fade45a54c8"}
Message was sent while issue was closed.
Description was changed from ========== Unit test that DnsSocketPool copies RandIntCallback BUG=686659 ========== to ========== Unit test that DnsSocketPool copies RandIntCallback BUG=686659 Review-Url: https://codereview.chromium.org/2660253003 Cr-Commit-Position: refs/heads/master@{#447250} Committed: https://chromium.googlesource.com/chromium/src/+/8ff9d153a2aee2f4a7acac6910a9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8ff9d153a2aee2f4a7acac6910a9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
