Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149973003:
Extracts CT DNS test utilities into a standalone class (Closed)
|
Created:
4 years, 5 months ago by Rob Percival Modified:
4 years, 5 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, Eran Messeri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtracts CT DNS test utilities into a standalone class
This will allow them to be re-used in tests for classes that use
LogDnsClient.
BUG=612439
Committed: https://crrev.com/6e7096f3a76b5da380dc7af628b95f768948a6b7
Cr-Commit-Position: refs/heads/master@{#407463}
Patch Set 1 #Patch Set 2 : Swap forward declaration for #include #Patch Set 3 : Rebase #
Total comments: 19
Patch Set 4 : Merge test_support with unittest build rule #Patch Set 5 : Renames "detail" namespace to "internal" #Patch Set 6 : Adds helper for emplacing MockSocketData #Patch Set 7 : Documents magic numbers in DNS writers #Patch Set 8 : Rebase and integrate #
Messages
Total messages: 40 (25 generated)
The CQ bit was checked by robpercival@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...
robpercival@chromium.org changed reviewers: + eranm@chromium.org, mmenke@chromium.org
PTAL
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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by robpercival@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...
robpercival@chromium.org changed reviewers: + juliatuttle@chromium.org
Hi Julia, mind taking a look at this? Following Matt Menke's advice, I implemented tests for certificate_transparency::LogDnsClient using mock sockets. Users of LogDnsClient also need to mock out the socket operations, so I'm extracting the test code into something reusable. Does this seem reasonable?
The CQ bit was checked by robpercival@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!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with some minor comments. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/BUILD.gn:33: source_set("test_support") { I suggest adding the mock_log_dns_traffic files to the unit_tests source_set and not introducing another build rule until something outside of components/certificate_transparency needs it. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.cc (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:34: const size_t query_section_size = encoded_qname.size() + 4; What does the '4' here stand for? Would it make sense having a constant for it ? https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:56: const size_t answers_section_size = 12 + answer.size(); Another magic number, please have a constant explaining it. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:170: mock_socket_data_.emplace_back(new MockSocketData(request, response)); inline request and response, since they're not used after being passed into the c'tor (or you could have a MockSocketData c'tor that'll invoke the CreateDns* methods.) https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:179: mock_socket_data_.emplace_back(new MockSocketData(request, net_error)); Seems like you could have a helper method/function for emplacing a new MockSocketData instance, then setting the read mode and adding it to factory. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:22: namespace detail { nit: Why not use 'internal' or 'internal_testing' ? That's the convention as far as I can tell for implementation details that have to be exposed. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:58: net::MockRead expected_reads_[2]; Why the 2 here, if we only support one read at most?
Description was changed from ========== Extracts CT DNS test utilities into a standalone class This will allow them to be re-used in tests for classes that use LogDnsClient. BUG=612439 ========== to ========== Extracts CT DNS test utilities into a standalone class This will allow them to be re-used in tests for classes that use LogDnsClient. BUG=612439 ==========
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/BUILD.gn:33: source_set("test_support") { On 2016/07/21 14:47:02, Eran Messeri wrote: > I suggest adding the mock_log_dns_traffic files to the unit_tests source_set and > not introducing another build rule until something outside of > components/certificate_transparency needs it. Done. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:22: namespace detail { On 2016/07/21 14:47:02, Eran Messeri wrote: > nit: Why not use 'internal' or 'internal_testing' ? That's the convention as > far as I can tell for implementation details that have to be exposed. Done. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:58: net::MockRead expected_reads_[2]; On 2016/07/21 14:47:02, Eran Messeri wrote: > Why the 2 here, if we only support one read at most? There is always a second "expected" read which is |eof_|. This makes the mock socket hang as though it were waiting for something to read, and should time out. This is more like the behaviour of an actual socket and gives better test failure messages (mock sockets CHECK-fail if more reads are attempted than there are MockReads). See https://cs.chromium.org/chromium/src/net/socket/socket_test_util.h?l=408 for the explanation of what |eof_| does. The DnsTransaction tests do the same thing: https://cs.chromium.org/chromium/src/net/dns/dns_transaction_unittest.cc?l=125. Should I improve the comment above to better explain that?
https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:58: net::MockRead expected_reads_[2]; On 2016/07/21 15:27:19, Rob Percival wrote: > On 2016/07/21 14:47:02, Eran Messeri wrote: > > Why the 2 here, if we only support one read at most? > > There is always a second "expected" read which is |eof_|. This makes the mock > socket hang as though it were waiting for something to read, and should time > out. This is more like the behaviour of an actual socket and gives better test > failure messages (mock sockets CHECK-fail if more reads are attempted than there > are MockReads). See > https://cs.chromium.org/chromium/src/net/socket/socket_test_util.h?l=408 for the > explanation of what |eof_| does. The DnsTransaction tests do the same thing: > https://cs.chromium.org/chromium/src/net/dns/dns_transaction_unittest.cc?l=125. > > Should I improve the comment above to better explain that? I noticed the eof_ later when reviewing the implementation. Yes, please improve the comment to explain.
https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.cc (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:34: const size_t query_section_size = encoded_qname.size() + 4; On 2016/07/21 14:47:02, Eran Messeri wrote: > What does the '4' here stand for? Would it make sense having a constant for it ? It's the 4 bytes you see written below, just after the encoded_qname is written. I could call it "query_section_size_minus_qname", or perhaps more sensibly split it into "record_type_size" and "record_class_size"? https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:56: const size_t answers_section_size = 12 + answer.size(); On 2016/07/21 14:47:02, Eran Messeri wrote: > Another magic number, please have a constant explaining it. Again, this is the 12 bytes you see written a little further down this function. I could break this down into all of the constituent parts, e.g. "qname_size", "record_type_size", "record_class_size", "ttl_size", "answer_size_size" (not so sure about that last one)? Alternatively, "answers_section_size_minus_answer". https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:170: mock_socket_data_.emplace_back(new MockSocketData(request, response)); On 2016/07/21 14:47:02, Eran Messeri wrote: > inline request and response, since they're not used after being passed into the > c'tor (or you could have a MockSocketData c'tor that'll invoke the CreateDns* > methods.) Creating |response| requires |request|, so they can't both be inlined that simply. Making MockSocketData invoke the CreateDns* functions is an option though. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:179: mock_socket_data_.emplace_back(new MockSocketData(request, net_error)); On 2016/07/21 14:47:02, Eran Messeri wrote: > Seems like you could have a helper method/function for emplacing a new > MockSocketData instance, then setting the read mode and adding it to factory. Done. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:58: net::MockRead expected_reads_[2]; On 2016/07/21 16:49:21, Eran Messeri wrote: > On 2016/07/21 15:27:19, Rob Percival wrote: > > On 2016/07/21 14:47:02, Eran Messeri wrote: > > > Why the 2 here, if we only support one read at most? > > > > There is always a second "expected" read which is |eof_|. This makes the mock > > socket hang as though it were waiting for something to read, and should time > > out. This is more like the behaviour of an actual socket and gives better test > > failure messages (mock sockets CHECK-fail if more reads are attempted than > there > > are MockReads). See > > https://cs.chromium.org/chromium/src/net/socket/socket_test_util.h?l=408 for > the > > explanation of what |eof_| does. The DnsTransaction tests do the same thing: > > > https://cs.chromium.org/chromium/src/net/dns/dns_transaction_unittest.cc?l=125. > > > > Should I improve the comment above to better explain that? > > I noticed the eof_ later when reviewing the implementation. Yes, please improve > the comment to explain. Done.
The CQ bit was checked by robpercival@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...
still lgtm https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.cc (right): https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:34: const size_t query_section_size = encoded_qname.size() + 4; On 2016/07/21 17:32:21, Rob Percival wrote: > On 2016/07/21 14:47:02, Eran Messeri wrote: > > What does the '4' here stand for? Would it make sense having a constant for it > ? > > It's the 4 bytes you see written below, just after the encoded_qname is written. > I could call it "query_section_size_minus_qname", or perhaps more sensibly split > it into "record_type_size" and "record_class_size"? Splitting sounds better. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:56: const size_t answers_section_size = 12 + answer.size(); On 2016/07/21 17:32:21, Rob Percival wrote: > On 2016/07/21 14:47:02, Eran Messeri wrote: > > Another magic number, please have a constant explaining it. > > Again, this is the 12 bytes you see written a little further down this function. > I could break this down into all of the constituent parts, e.g. "qname_size", > "record_type_size", "record_class_size", "ttl_size", "answer_size_size" (not so > sure about that last one)? Alternatively, "answers_section_size_minus_answer". Again, breaking down seems better as it's clear what we're adding and it's easy to review and make sure that the size of each added component matches what's being written to the BigEndianWriter. https://codereview.chromium.org/2149973003/diff/60001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:170: mock_socket_data_.emplace_back(new MockSocketData(request, response)); On 2016/07/21 17:32:21, Rob Percival wrote: > On 2016/07/21 14:47:02, Eran Messeri wrote: > > inline request and response, since they're not used after being passed into > the > > c'tor (or you could have a MockSocketData c'tor that'll invoke the CreateDns* > > methods.) > > Creating |response| requires |request|, so they can't both be inlined that > simply. Making MockSocketData invoke the CreateDns* functions is an option > though. So at the very least you could inline creating the response? Having the MockSocketData do that is also fine, whatever looks best.
Patchset #7 (id:140001) has been deleted
PTAL - non-trivial rebase performed.
lgtm On Fri, Jul 22, 2016 at 1:53 PM <robpercival@chromium.org> wrote: > PTAL - non-trivial rebase performed. > > https://codereview.chromium.org/2149973003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by robpercival@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.
still lgtm
The CQ bit was checked by robpercival@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 ========== Extracts CT DNS test utilities into a standalone class This will allow them to be re-used in tests for classes that use LogDnsClient. BUG=612439 ========== to ========== Extracts CT DNS test utilities into a standalone class This will allow them to be re-used in tests for classes that use LogDnsClient. BUG=612439 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Extracts CT DNS test utilities into a standalone class This will allow them to be re-used in tests for classes that use LogDnsClient. BUG=612439 ========== to ========== Extracts CT DNS test utilities into a standalone class This will allow them to be re-used in tests for classes that use LogDnsClient. BUG=612439 Committed: https://crrev.com/6e7096f3a76b5da380dc7af628b95f768948a6b7 Cr-Commit-Position: refs/heads/master@{#407463} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6e7096f3a76b5da380dc7af628b95f768948a6b7 Cr-Commit-Position: refs/heads/master@{#407463} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
