Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331923003:
Allow LogDnsClient queries to be rate-limited (Closed)
|
Created:
4 years, 3 months ago by Rob Percival Modified:
4 years, 2 months ago CC:
Eran Messeri, certificate-transparency-chrome_googlegroups.com, chromium-reviews, eroman, rsleevi+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow LogDnsClient queries to be rate-limited
A maximum number of concurrent queries can be specified. By default,
there is no limit.
BUG=624894
Committed: https://crrev.com/2a9f5c09189a26b2f6140c02946bbd44ca9876f7
Cr-Commit-Position: refs/heads/master@{#424541}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Set LogDnsClient max_concurrent_queries via constructor param #
Total comments: 19
Patch Set 3 : Addresses some of Ryan Sleevi's comments #Patch Set 4 : Use Eq matcher with EXPECT_THAT #
Total comments: 20
Patch Set 5 : Addresses some of Ryan Sleevi's comments #Patch Set 6 : Fix uint64_t format specifiers on 32-bit builds #Patch Set 7 : Change "EXPECT_TRUE(foo) + if(foo)" to "ASSERT_TRUE(foo)" #Patch Set 8 : Fix size_t format specifiers on 32-bit builds #Patch Set 9 : Rebase #Patch Set 10 : Improves tests #
Total comments: 2
Patch Set 11 : Removes unnecessary error messages #Patch Set 12 : Rebase #Patch Set 13 : Rebase #
Total comments: 6
Patch Set 14 : Addresses last of Ryan's comments #
Messages
Total messages: 93 (68 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eranm@chromium.org changed reviewers: + eranm@chromium.org
https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:117: base::ThreadTaskRunnerHandle::Get()->PostTask( Rather than invoke the callback, I suggest returning a value here to indicate the request has not been queued. This would simplify handling of this case in the client - i.e. the caller could immediately make a decision on what to do with the request given it's throttled, rather than having to wait with the decision until the callback is involved. So easier reasoning and more straightforward handling of this case (also less code). https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:62: size_t GetMaxConcurrentQueries() const { return max_concurrent_queries_; } Nit: What use do clients have for this method? https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.h:68: void SetMaxConcurrentQueries(size_t max_queries) { Nit: I'd make the value a c'tor parameter so clients that get a pointer to this instance can't set the number.
https://chromiumcodereview-hr.appspot.com/2331923003/diff/1/components/certif... File components/certificate_transparency/log_dns_client.cc (right): https://chromiumcodereview-hr.appspot.com/2331923003/diff/1/components/certif... components/certificate_transparency/log_dns_client.cc:117: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/09/12 13:26:48, Eran Messeri wrote: > Rather than invoke the callback, I suggest returning a value here to indicate > the request has not been queued. This would simplify handling of this case in > the client - i.e. the caller could immediately make a decision on what to do > with the request given it's throttled, rather than having to wait with the > decision until the callback is involved. So easier reasoning and more > straightforward handling of this case (also less code). Would you suggest the other errors that can be returned synchronously (ERR_INVALID_ARGUMENT, ERR_NAME_RESOLUTION_FAILED) also be returned in this way? I thought it was simpler having a single way to report errors (the callback), especially since the likely response to being throttled (retry later) is the same response you might want to take to other errors. However, if you aren't worrying about retrying in those other scenarios, I can imagine that knowing you're being throttled immediately could simplify calling code. https://chromiumcodereview-hr.appspot.com/2331923003/diff/1/components/certif... File components/certificate_transparency/log_dns_client.h (right): https://chromiumcodereview-hr.appspot.com/2331923003/diff/1/components/certif... components/certificate_transparency/log_dns_client.h:62: size_t GetMaxConcurrentQueries() const { return max_concurrent_queries_; } On 2016/09/12 13:26:48, Eran Messeri wrote: > Nit: What use do clients have for this method? Probably none, so I'm happy to remove it. https://chromiumcodereview-hr.appspot.com/2331923003/diff/1/components/certif... components/certificate_transparency/log_dns_client.h:68: void SetMaxConcurrentQueries(size_t max_queries) { On 2016/09/12 13:26:48, Eran Messeri wrote: > Nit: I'd make the value a c'tor parameter so clients that get a pointer to this > instance can't set the number. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
+eroman and mmenke, because I'm potentially relitigating something from a past CL that Eric approved and Matt raised concerns about. https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client.cc:120: base::Bind(callback, net::Error::ERR_TEMPORARILY_THROTTLED, 0)); The downside to this approach is that it forces a thread sleep/wakeup, potentially, for what could be a synchronous error handling. Most of the //net APIs try to follow a sync-or-async pattern that lends itself naturally into forcing a state machine progression style loop. In that model, the overhead of sync-or-async should be eliminated, with the benefit that it reduces overheads. To that end, I suppose it equally applies to 111-115 (and other error returns). That seems like a much larger change, so I can certainly buy that we should be treating that as separate; if this was the first instance of a sync return, I would say in this CL, but since we have pre-existing code, we should try to update it. https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... File components/certificate_transparency/log_dns_client.h (right): https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client.h:119: bool HasMaxConcurrentQueriesInProgress() const; +1 to comment about use case https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:125: size_t max_concurrent_queries = 0 /* unlimited */) { https://google.github.io/styleguide/cppguide.html#Default_Arguments generally discourages this pattern. https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:142: auto log_client = CreateLogDnsClient(); There's been a variety of threads on this particular usage of auto (e.g. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5-Bt3BJzAo0/dis... https://groups.google.com/a/chromium.org/d/topic/chromium-dev/MVLrzzu3DcM/dis... ) TL;DR: Prefer to be explicit here std::unique_ptr<LogDnsClient> log_client(CreateLogDnsClient()); https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:161: auto log_client = CreateLogDnsClient(); see above https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:186: EXPECT_THAT(callback.leaf_index(), 123456); This is a really odd usage of GMock. There's already a general anti-GMock sentiment (which is to say, readability is generally felt to be hindered, rather than helped, by it), but it's especially odd to see EXPECT_THAT's which are essentially GTest EXPECT_EQs For example, it's unclear why this isn't EXPECT_EQ(net::OK, callback.net_error()) // Or EXPECT_TRUE(IsOK(callback.net_error())); EXPECT_EQ(123456, callback.leaf_index()) In looking through the history, I see some of these concerns were raised in https://codereview.chromium.org/2111093002 . Had I been aware, I would have pointed to the multiple threads on chromium-dev@ discussing this, and generally coming away with the "Avoid GMock when possible" discussions. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... I realize that GMock has now been fully folded into GTest, and so the distinction between the two is much less significant, but I think I share Matt's concerns about unease introducing this. https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:406: // EXPECT_THAT(callback.proof()->tree_size, 999999); Dead code? https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:588: for (size_t i = 0; i < num_of_parallel_queries; ++i) { More inline documentation about the tests and what's being tested. https://codereview.chromium.org/2331923003/diff/20001/components/certificate_... components/certificate_transparency/log_dns_client_unittest.cc:608: // only space for 7 nodes per UDP packet. Expect each of these queries to What is the source of this magic value (7 nodes per UDP packet). How does it fit into things like various network fragmentation limits? This seems to open more questions than it answers, and hints of a magic number somewhere far away that could invalidate this test.
Patchset #3 (id:40001) 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...
Patchset #3 (id:60001) 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...
https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... File components/certificate_transparency/log_dns_client.cc (right): https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client.cc:120: base::Bind(callback, net::Error::ERR_TEMPORARILY_THROTTLED, 0)); On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > The downside to this approach is that it forces a thread sleep/wakeup, > potentially, for what could be a synchronous error handling. > > Most of the //net APIs try to follow a sync-or-async pattern that lends itself > naturally into forcing a state machine progression style loop. In that model, > the overhead of sync-or-async should be eliminated, with the benefit that it > reduces overheads. > > To that end, I suppose it equally applies to 111-115 (and other error returns). > > That seems like a much larger change, so I can certainly buy that we should be > treating that as separate; if this was the first instance of a sync return, I > would say in this CL, but since we have pre-existing code, we should try to > update it. I originally began implementing this class using that pattern, but found it to be less clear given there are two distinct state machines in this class. Breaking the state machines out into LeafIndexQuery and AuditProofQuery classes, using that pattern for both, and gutting this down to just tracking the DNS config and constructing instances of those classes might have been better, but seemed overly complex. I could do something like that in a separate CL though if you think it's worth it (or just adapt this class to have a single, mixed state machine). On a related note, should I update the comment for ERR_TEMPORARILY_THROTTLED at https://cs.chromium.org/chromium/src/net/base/net_error_list.h?l=250 to make it less specific (currently talks about the "request throttler module")? There's another error code, ERR_HOST_RESOLVER_QUEUE_TOO_LARGE, which could alternatively be used, but that too suffers from being a bit too specialised. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... File components/certificate_transparency/log_dns_client.h (right): https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client.h:119: bool HasMaxConcurrentQueriesInProgress() const; On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > +1 to comment about use case There was no comment about the use case of this method. Eran's comment referred to a different, public, now-removed method. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:125: size_t max_concurrent_queries = 0 /* unlimited */) { On 2016/09/12 18:12:44, Ryan Sleevi (slow) wrote: > https://google.github.io/styleguide/cppguide.html#Default_Arguments generally > discourages this pattern. That's why I didn't set a default value for the constructor parameter, but it seemed more reasonable in test code, which almost never sets a limit on concurrent queries. Otherwise, I'd either have to declare "size_t max_concurrent_queries = 0; // unlimited" in a few different places or just have a magic number 0 on the end of the various CreateLogDnsClient calls. This seemed cleaner. However, I suppose I've ended up with those trailing 0 arguments in a couple of places anyway so I'm ok with changing this. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:142: auto log_client = CreateLogDnsClient(); On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > There's been a variety of threads on this particular usage of auto (e.g. > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5-Bt3BJzAo0/dis... > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/MVLrzzu3DcM/dis... > ) > > TL;DR: Prefer to be explicit here > > std::unique_ptr<LogDnsClient> log_client(CreateLogDnsClient()); Done. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:161: auto log_client = CreateLogDnsClient(); On 2016/09/12 18:12:44, Ryan Sleevi (slow) wrote: > see above Done. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:186: EXPECT_THAT(callback.leaf_index(), 123456); On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > This is a really odd usage of GMock. There's already a general anti-GMock > sentiment (which is to say, readability is generally felt to be hindered, rather > than helped, by it), but it's especially odd to see EXPECT_THAT's which are > essentially GTest EXPECT_EQs > > For example, it's unclear why this isn't > EXPECT_EQ(net::OK, callback.net_error()) // Or > EXPECT_TRUE(IsOK(callback.net_error())); > EXPECT_EQ(123456, callback.leaf_index()) > > In looking through the history, I see some of these concerns were raised in > https://codereview.chromium.org/2111093002 . Had I been aware, I would have > pointed to the multiple threads on chromium-dev@ discussing this, and generally > coming away with the "Avoid GMock when possible" discussions. > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... > > > I realize that GMock has now been fully folded into GTest, and so the > distinction between the two is much less significant, but I think I share Matt's > concerns about unease introducing this. This is actually the recommended usage, according to the GTest/GMock FAQ: "We are encouraging people to use the unified EXPECT_THAT(value, matcher) syntax more often in tests. One significant advantage of the matcher approach is that matchers can be easily combined to form new matchers, while the EXPECT_NE, etc, macros cannot be easily combined. Therefore we want to invest more in the matchers than in the EXPECT_XX() macros." I've also seen arguments in favour of EXPECT_THAT related to the predictable argument order (actual, expected) vs the EXPECT_XX macros. In some cases, EXPECT_THAT gives better failure messages too. In particular, with net::Error, EXPECT_THAT(foo, IsOk()) gives a much better error message than EXPECT_EQ(net::OK, foo). The former tells you the name of the error that occurred, the latter prints the integer value and you have to go look it up. The linked discussions actually appear to support my position, if anything. They make reasonable arguments against *mocking* but, when *matchers* (i.e. EXPECT_THAT) are mentioned, it's practically always in a positive tone. I can understand some unease at learning a slightly different way of expressing expectations, but it is a more fluent and expressive method, not to mention being widely recommended. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:406: // EXPECT_THAT(callback.proof()->tree_size, 999999); On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > Dead code? Future code - it's pending the addition of a tree_size member to MerkleAuditProof (done in https://chromiumcodereview-hr.appspot.com/2182533002). There's a TODO that covers this in LogDnsClient.cc, but I've added some here too now. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:588: for (size_t i = 0; i < num_of_parallel_queries; ++i) { On 2016/09/12 18:12:44, Ryan Sleevi (slow) wrote: > More inline documentation about the tests and what's being tested. Done. https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:608: // only space for 7 nodes per UDP packet. Expect each of these queries to On 2016/09/12 18:12:44, Ryan Sleevi (slow) wrote: > What is the source of this magic value (7 nodes per UDP packet). How does it fit > into things like various network fragmentation limits? This seems to open more > questions than it answers, and hints of a magic number somewhere far away that > could invalidate this test. I've now clarified this. That should have said TXT RDATA, not UDP packet.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... components/certificate_transparency/log_dns_client_unittest.cc:186: EXPECT_THAT(callback.leaf_index(), 123456); On 2016/09/13 14:06:31, Rob Percival wrote: > On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > > This is a really odd usage of GMock. There's already a general anti-GMock > > sentiment (which is to say, readability is generally felt to be hindered, > rather > > than helped, by it), but it's especially odd to see EXPECT_THAT's which are > > essentially GTest EXPECT_EQs > > > > For example, it's unclear why this isn't > > EXPECT_EQ(net::OK, callback.net_error()) // Or > > EXPECT_TRUE(IsOK(callback.net_error())); > > EXPECT_EQ(123456, callback.leaf_index()) > > > > In looking through the history, I see some of these concerns were raised in > > https://codereview.chromium.org/2111093002 . Had I been aware, I would have > > pointed to the multiple threads on chromium-dev@ discussing this, and > generally > > coming away with the "Avoid GMock when possible" discussions. > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... > > > > > > I realize that GMock has now been fully folded into GTest, and so the > > distinction between the two is much less significant, but I think I share > Matt's > > concerns about unease introducing this. > > This is actually the recommended usage, according to the GTest/GMock FAQ: > "We are encouraging people to use the unified EXPECT_THAT(value, matcher) syntax > more often in tests. One significant advantage of the matcher approach is that > matchers can be easily combined to form new matchers, while the EXPECT_NE, etc, > macros cannot be easily combined. Therefore we want to invest more in the > matchers than in the EXPECT_XX() macros." > > I've also seen arguments in favour of EXPECT_THAT related to the predictable > argument order (actual, expected) vs the EXPECT_XX macros. In some cases, > EXPECT_THAT gives better failure messages too. In particular, with net::Error, > EXPECT_THAT(foo, IsOk()) gives a much better error message than > EXPECT_EQ(net::OK, foo). The former tells you the name of the error that > occurred, the latter prints the integer value and you have to go look it up. > > The linked discussions actually appear to support my position, if anything. They > make reasonable arguments against *mocking* but, when *matchers* (i.e. > EXPECT_THAT) are mentioned, it's practically always in a positive tone. I can > understand some unease at learning a slightly different way of expressing > expectations, but it is a more fluent and expressive method, not to mention > being widely recommended. The fact that they're reversed seems extremely confusing when the code base uses both. EXPECT_THAT also seems rather less intuitive than EXPECT_EQ - I have no idea what it's doing from reading it. The "IsBlah()" macros are a bit clearer...But I have even less idea what those macros are actually doing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/13 14:31:29, mmenke wrote: > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > components/certificate_transparency/log_dns_client_unittest.cc:186: > EXPECT_THAT(callback.leaf_index(), 123456); > On 2016/09/13 14:06:31, Rob Percival wrote: > > On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > > > This is a really odd usage of GMock. There's already a general anti-GMock > > > sentiment (which is to say, readability is generally felt to be hindered, > > rather > > > than helped, by it), but it's especially odd to see EXPECT_THAT's which are > > > essentially GTest EXPECT_EQs > > > > > > For example, it's unclear why this isn't > > > EXPECT_EQ(net::OK, callback.net_error()) // Or > > > EXPECT_TRUE(IsOK(callback.net_error())); > > > EXPECT_EQ(123456, callback.leaf_index()) > > > > > > In looking through the history, I see some of these concerns were raised in > > > https://codereview.chromium.org/2111093002 . Had I been aware, I would have > > > pointed to the multiple threads on chromium-dev@ discussing this, and > > generally > > > coming away with the "Avoid GMock when possible" discussions. > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... > > > > > > > > > I realize that GMock has now been fully folded into GTest, and so the > > > distinction between the two is much less significant, but I think I share > > Matt's > > > concerns about unease introducing this. > > > > This is actually the recommended usage, according to the GTest/GMock FAQ: > > "We are encouraging people to use the unified EXPECT_THAT(value, matcher) > syntax > > more often in tests. One significant advantage of the matcher approach is that > > matchers can be easily combined to form new matchers, while the EXPECT_NE, > etc, > > macros cannot be easily combined. Therefore we want to invest more in the > > matchers than in the EXPECT_XX() macros." > > > > I've also seen arguments in favour of EXPECT_THAT related to the predictable > > argument order (actual, expected) vs the EXPECT_XX macros. In some cases, > > EXPECT_THAT gives better failure messages too. In particular, with net::Error, > > EXPECT_THAT(foo, IsOk()) gives a much better error message than > > EXPECT_EQ(net::OK, foo). The former tells you the name of the error that > > occurred, the latter prints the integer value and you have to go look it up. > > > > The linked discussions actually appear to support my position, if anything. > They > > make reasonable arguments against *mocking* but, when *matchers* (i.e. > > EXPECT_THAT) are mentioned, it's practically always in a positive tone. I can > > understand some unease at learning a slightly different way of expressing > > expectations, but it is a more fluent and expressive method, not to mention > > being widely recommended. > > The fact that they're reversed seems extremely confusing when the code base uses > both. EXPECT_THAT also seems rather less intuitive than EXPECT_EQ - I have no > idea what it's doing from reading it. The "IsBlah()" macros are a bit > clearer...But I have even less idea what those macros are actually doing. If you read "EXPECT_THAT(foo, Eq(bar))" naturally, I think it's intuitive what it's doing, i.e. "expect that foo equals bar". That can be extended to do all sorts of other comparisons and continues to be readable in the same way. Compare this to "EXPECT_EQ(bar, foo)", which (traditionally) directly translates to "expect equal to bar, is foo". This is only less confusing because you're accustomed to it, but is likely to be more confusing to people who haven't used GTest. It's made all the more confusing by the fact that macros like EXPECT_GT tend to have the expected argument second. In fact, it's also contrary to Google's internal C++ readability advice (see "avoid "Yoda-style" comparisons"). Having said all that, EXPECT_EQ stopped treating its arguments as "expected" and "actual" earlier in the year, so there's nothing stopping people from using either ordering at the moment. EXPECT_THAT at least enforces a sensible ordering. It's hard to argue over whether EXPECT_THAT is more or less readable or confusing, since it's heavily subjective based on how much you've used EXPECT_XX vs EXPECT_THAT. The latter does, unarguably, have improved capabilities though. Regardless, in this case, I'm fine with changing everything that doesn't use a matcher to use EXPECT_EQ if you'd strongly prefer that Ryan.
On 2016/09/14 19:04:55, Rob Percival wrote: > On 2016/09/13 14:31:29, mmenke wrote: > > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > > > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > > components/certificate_transparency/log_dns_client_unittest.cc:186: > > EXPECT_THAT(callback.leaf_index(), 123456); > > On 2016/09/13 14:06:31, Rob Percival wrote: > > > On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > > > > This is a really odd usage of GMock. There's already a general anti-GMock > > > > sentiment (which is to say, readability is generally felt to be hindered, > > > rather > > > > than helped, by it), but it's especially odd to see EXPECT_THAT's which > are > > > > essentially GTest EXPECT_EQs > > > > > > > > For example, it's unclear why this isn't > > > > EXPECT_EQ(net::OK, callback.net_error()) // Or > > > > EXPECT_TRUE(IsOK(callback.net_error())); > > > > EXPECT_EQ(123456, callback.leaf_index()) > > > > > > > > In looking through the history, I see some of these concerns were raised > in > > > > https://codereview.chromium.org/2111093002 . Had I been aware, I would > have > > > > pointed to the multiple threads on chromium-dev@ discussing this, and > > > generally > > > > coming away with the "Avoid GMock when possible" discussions. > > > > > > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... > > > > > > > > > > > > I realize that GMock has now been fully folded into GTest, and so the > > > > distinction between the two is much less significant, but I think I share > > > Matt's > > > > concerns about unease introducing this. > > > > > > This is actually the recommended usage, according to the GTest/GMock FAQ: > > > "We are encouraging people to use the unified EXPECT_THAT(value, matcher) > > syntax > > > more often in tests. One significant advantage of the matcher approach is > that > > > matchers can be easily combined to form new matchers, while the EXPECT_NE, > > etc, > > > macros cannot be easily combined. Therefore we want to invest more in the > > > matchers than in the EXPECT_XX() macros." > > > > > > I've also seen arguments in favour of EXPECT_THAT related to the predictable > > > argument order (actual, expected) vs the EXPECT_XX macros. In some cases, > > > EXPECT_THAT gives better failure messages too. In particular, with > net::Error, > > > EXPECT_THAT(foo, IsOk()) gives a much better error message than > > > EXPECT_EQ(net::OK, foo). The former tells you the name of the error that > > > occurred, the latter prints the integer value and you have to go look it up. > > > > > > The linked discussions actually appear to support my position, if anything. > > They > > > make reasonable arguments against *mocking* but, when *matchers* (i.e. > > > EXPECT_THAT) are mentioned, it's practically always in a positive tone. I > can > > > understand some unease at learning a slightly different way of expressing > > > expectations, but it is a more fluent and expressive method, not to mention > > > being widely recommended. > > > > The fact that they're reversed seems extremely confusing when the code base > uses > > both. EXPECT_THAT also seems rather less intuitive than EXPECT_EQ - I have no > > idea what it's doing from reading it. The "IsBlah()" macros are a bit > > clearer...But I have even less idea what those macros are actually doing. > > If you read "EXPECT_THAT(foo, Eq(bar))" naturally, I think it's intuitive what > it's doing, i.e. "expect that foo equals bar". That can be extended to do all > sorts of other comparisons and continues to be readable in the same way. Compare > this to "EXPECT_EQ(bar, foo)", which (traditionally) directly translates to > "expect equal to bar, is foo". This is only less confusing because you're > accustomed to it, but is likely to be more confusing to people who haven't used > GTest. It's made all the more confusing by the fact that macros like EXPECT_GT > tend to have the expected argument second. In fact, it's also contrary to > Google's internal C++ readability advice (see "avoid "Yoda-style" comparisons"). > Having said all that, EXPECT_EQ stopped treating its arguments as "expected" and > "actual" earlier in the year, so there's nothing stopping people from using > either ordering at the moment. EXPECT_THAT at least enforces a sensible > ordering. > > It's hard to argue over whether EXPECT_THAT is more or less readable or > confusing, since it's heavily subjective based on how much you've used EXPECT_XX > vs EXPECT_THAT. The latter does, unarguably, have improved capabilities though. > Regardless, in this case, I'm fine with changing everything that doesn't use a > matcher to use EXPECT_EQ if you'd strongly prefer that Ryan. So where does "EXPECT_THAT(callback.proof()->nodes, audit_proof);" fall? That seems completely incomprehensible to me.
On 2016/09/14 19:27:26, mmenke wrote: > On 2016/09/14 19:04:55, Rob Percival wrote: > > On 2016/09/13 14:31:29, mmenke wrote: > > > > > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > > > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > > > > > > > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > > > components/certificate_transparency/log_dns_client_unittest.cc:186: > > > EXPECT_THAT(callback.leaf_index(), 123456); > > > On 2016/09/13 14:06:31, Rob Percival wrote: > > > > On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > > > > > This is a really odd usage of GMock. There's already a general > anti-GMock > > > > > sentiment (which is to say, readability is generally felt to be > hindered, > > > > rather > > > > > than helped, by it), but it's especially odd to see EXPECT_THAT's which > > are > > > > > essentially GTest EXPECT_EQs > > > > > > > > > > For example, it's unclear why this isn't > > > > > EXPECT_EQ(net::OK, callback.net_error()) // Or > > > > > EXPECT_TRUE(IsOK(callback.net_error())); > > > > > EXPECT_EQ(123456, callback.leaf_index()) > > > > > > > > > > In looking through the history, I see some of these concerns were raised > > in > > > > > https://codereview.chromium.org/2111093002 . Had I been aware, I would > > have > > > > > pointed to the multiple threads on chromium-dev@ discussing this, and > > > > generally > > > > > coming away with the "Avoid GMock when possible" discussions. > > > > > > > > > > > > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > > > > > > > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... > > > > > > > > > > > > > > > I realize that GMock has now been fully folded into GTest, and so the > > > > > distinction between the two is much less significant, but I think I > share > > > > Matt's > > > > > concerns about unease introducing this. > > > > > > > > This is actually the recommended usage, according to the GTest/GMock FAQ: > > > > "We are encouraging people to use the unified EXPECT_THAT(value, matcher) > > > syntax > > > > more often in tests. One significant advantage of the matcher approach is > > that > > > > matchers can be easily combined to form new matchers, while the EXPECT_NE, > > > etc, > > > > macros cannot be easily combined. Therefore we want to invest more in the > > > > matchers than in the EXPECT_XX() macros." > > > > > > > > I've also seen arguments in favour of EXPECT_THAT related to the > predictable > > > > argument order (actual, expected) vs the EXPECT_XX macros. In some cases, > > > > EXPECT_THAT gives better failure messages too. In particular, with > > net::Error, > > > > EXPECT_THAT(foo, IsOk()) gives a much better error message than > > > > EXPECT_EQ(net::OK, foo). The former tells you the name of the error that > > > > occurred, the latter prints the integer value and you have to go look it > up. > > > > > > > > The linked discussions actually appear to support my position, if > anything. > > > They > > > > make reasonable arguments against *mocking* but, when *matchers* (i.e. > > > > EXPECT_THAT) are mentioned, it's practically always in a positive tone. I > > can > > > > understand some unease at learning a slightly different way of expressing > > > > expectations, but it is a more fluent and expressive method, not to > mention > > > > being widely recommended. > > > > > > The fact that they're reversed seems extremely confusing when the code base > > uses > > > both. EXPECT_THAT also seems rather less intuitive than EXPECT_EQ - I have > no > > > idea what it's doing from reading it. The "IsBlah()" macros are a bit > > > clearer...But I have even less idea what those macros are actually doing. > > > > If you read "EXPECT_THAT(foo, Eq(bar))" naturally, I think it's intuitive what > > it's doing, i.e. "expect that foo equals bar". That can be extended to do all > > sorts of other comparisons and continues to be readable in the same way. > Compare > > this to "EXPECT_EQ(bar, foo)", which (traditionally) directly translates to > > "expect equal to bar, is foo". This is only less confusing because you're > > accustomed to it, but is likely to be more confusing to people who haven't > used > > GTest. It's made all the more confusing by the fact that macros like EXPECT_GT > > tend to have the expected argument second. In fact, it's also contrary to > > Google's internal C++ readability advice (see "avoid "Yoda-style" > comparisons"). > > Having said all that, EXPECT_EQ stopped treating its arguments as "expected" > and > > "actual" earlier in the year, so there's nothing stopping people from using > > either ordering at the moment. EXPECT_THAT at least enforces a sensible > > ordering. > > > > It's hard to argue over whether EXPECT_THAT is more or less readable or > > confusing, since it's heavily subjective based on how much you've used > EXPECT_XX > > vs EXPECT_THAT. The latter does, unarguably, have improved capabilities > though. > > Regardless, in this case, I'm fine with changing everything that doesn't use a > > matcher to use EXPECT_EQ if you'd strongly prefer that Ryan. > > So where does "EXPECT_THAT(callback.proof()->nodes, audit_proof);" fall? That > seems completely incomprehensible to me. Or "EXPECT_THAT(callback1.proof()->leaf_index, 123456);"
Also, just from a C/C++ perspective... "Eq(blah)" doesn't make much sense. x.Eq(Blah) or "Eq(x, Blah)", sure. But Eq(Blah) doesn't map to C++ syntax, which leaves me scratching my head a bit.
On 2016/09/14 19:27:26, mmenke wrote: > On 2016/09/14 19:04:55, Rob Percival wrote: > > On 2016/09/13 14:31:29, mmenke wrote: > > > > > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > > > File components/certificate_transparency/log_dns_client_unittest.cc (right): > > > > > > > > > https://chromiumcodereview-hr.appspot.com/2331923003/diff/20001/components/ce... > > > components/certificate_transparency/log_dns_client_unittest.cc:186: > > > EXPECT_THAT(callback.leaf_index(), 123456); > > > On 2016/09/13 14:06:31, Rob Percival wrote: > > > > On 2016/09/12 18:12:43, Ryan Sleevi (slow) wrote: > > > > > This is a really odd usage of GMock. There's already a general > anti-GMock > > > > > sentiment (which is to say, readability is generally felt to be > hindered, > > > > rather > > > > > than helped, by it), but it's especially odd to see EXPECT_THAT's which > > are > > > > > essentially GTest EXPECT_EQs > > > > > > > > > > For example, it's unclear why this isn't > > > > > EXPECT_EQ(net::OK, callback.net_error()) // Or > > > > > EXPECT_TRUE(IsOK(callback.net_error())); > > > > > EXPECT_EQ(123456, callback.leaf_index()) > > > > > > > > > > In looking through the history, I see some of these concerns were raised > > in > > > > > https://codereview.chromium.org/2111093002 . Had I been aware, I would > > have > > > > > pointed to the multiple threads on chromium-dev@ discussing this, and > > > > generally > > > > > coming away with the "Avoid GMock when possible" discussions. > > > > > > > > > > > > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/-KH_IP0rIWQ/dis... > > > > > > > > > > > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/1zktlTqFb6o/dis... > > > > > > > > > > > > > > > I realize that GMock has now been fully folded into GTest, and so the > > > > > distinction between the two is much less significant, but I think I > share > > > > Matt's > > > > > concerns about unease introducing this. > > > > > > > > This is actually the recommended usage, according to the GTest/GMock FAQ: > > > > "We are encouraging people to use the unified EXPECT_THAT(value, matcher) > > > syntax > > > > more often in tests. One significant advantage of the matcher approach is > > that > > > > matchers can be easily combined to form new matchers, while the EXPECT_NE, > > > etc, > > > > macros cannot be easily combined. Therefore we want to invest more in the > > > > matchers than in the EXPECT_XX() macros." > > > > > > > > I've also seen arguments in favour of EXPECT_THAT related to the > predictable > > > > argument order (actual, expected) vs the EXPECT_XX macros. In some cases, > > > > EXPECT_THAT gives better failure messages too. In particular, with > > net::Error, > > > > EXPECT_THAT(foo, IsOk()) gives a much better error message than > > > > EXPECT_EQ(net::OK, foo). The former tells you the name of the error that > > > > occurred, the latter prints the integer value and you have to go look it > up. > > > > > > > > The linked discussions actually appear to support my position, if > anything. > > > They > > > > make reasonable arguments against *mocking* but, when *matchers* (i.e. > > > > EXPECT_THAT) are mentioned, it's practically always in a positive tone. I > > can > > > > understand some unease at learning a slightly different way of expressing > > > > expectations, but it is a more fluent and expressive method, not to > mention > > > > being widely recommended. > > > > > > The fact that they're reversed seems extremely confusing when the code base > > uses > > > both. EXPECT_THAT also seems rather less intuitive than EXPECT_EQ - I have > no > > > idea what it's doing from reading it. The "IsBlah()" macros are a bit > > > clearer...But I have even less idea what those macros are actually doing. > > > > If you read "EXPECT_THAT(foo, Eq(bar))" naturally, I think it's intuitive what > > it's doing, i.e. "expect that foo equals bar". That can be extended to do all > > sorts of other comparisons and continues to be readable in the same way. > Compare > > this to "EXPECT_EQ(bar, foo)", which (traditionally) directly translates to > > "expect equal to bar, is foo". This is only less confusing because you're > > accustomed to it, but is likely to be more confusing to people who haven't > used > > GTest. It's made all the more confusing by the fact that macros like EXPECT_GT > > tend to have the expected argument second. In fact, it's also contrary to > > Google's internal C++ readability advice (see "avoid "Yoda-style" > comparisons"). > > Having said all that, EXPECT_EQ stopped treating its arguments as "expected" > and > > "actual" earlier in the year, so there's nothing stopping people from using > > either ordering at the moment. EXPECT_THAT at least enforces a sensible > > ordering. > > > > It's hard to argue over whether EXPECT_THAT is more or less readable or > > confusing, since it's heavily subjective based on how much you've used > EXPECT_XX > > vs EXPECT_THAT. The latter does, unarguably, have improved capabilities > though. > > Regardless, in this case, I'm fine with changing everything that doesn't use a > > matcher to use EXPECT_EQ if you'd strongly prefer that Ryan. > > So where does "EXPECT_THAT(callback.proof()->nodes, audit_proof);" fall? That > seems completely incomprehensible to me. That's admittedly the result of me using the shorthand, which allows you to omit "Eq()". This is probably a bad idea when you can't expect people to know that this is the default comparison. I can put in the Eq() calls if you think that improves readability. Alternatively, as I just said, I'm fine with changing these cases to EXPECT_EQ. The benefit of EXPECT_THAT is primarily for the tests that use matchers (e.g. net::Error comparisons).
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.
https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... components/certificate_transparency/log_dns_client.cc:117: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/09/12 at 17:26:24, Rob Percival wrote: > On 2016/09/12 13:26:48, Eran Messeri wrote: > > Rather than invoke the callback, I suggest returning a value here to indicate > > the request has not been queued. This would simplify handling of this case in > > the client - i.e. the caller could immediately make a decision on what to do > > with the request given it's throttled, rather than having to wait with the > > decision until the callback is involved. So easier reasoning and more > > straightforward handling of this case (also less code). > > Would you suggest the other errors that can be returned synchronously (ERR_INVALID_ARGUMENT, ERR_NAME_RESOLUTION_FAILED) also be returned in this way? > > I thought it was simpler having a single way to report errors (the callback), especially since the likely response to being throttled (retry later) is the same response you might want to take to other errors. However, if you aren't worrying about retrying in those other scenarios, I can imagine that knowing you're being throttled immediately could simplify calling code. Yes, I would suggest other errors are also returned this way. It does simplify calling code, since as you pointed out, the caller would simply stop calling this method. Other errors should definitely not be re-tried - if the caller is not positive that the return value indicates throttling, it may be a usage error (it could DCHECK or simply drop the request).
On 2016/09/14 23:25:43, Eran Messeri wrote: > https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... > File components/certificate_transparency/log_dns_client.cc (right): > > https://codereview.chromium.org/2331923003/diff/1/components/certificate_tran... > components/certificate_transparency/log_dns_client.cc:117: > base::ThreadTaskRunnerHandle::Get()->PostTask( > On 2016/09/12 at 17:26:24, Rob Percival wrote: > > On 2016/09/12 13:26:48, Eran Messeri wrote: > > > Rather than invoke the callback, I suggest returning a value here to > indicate > > > the request has not been queued. This would simplify handling of this case > in > > > the client - i.e. the caller could immediately make a decision on what to do > > > with the request given it's throttled, rather than having to wait with the > > > decision until the callback is involved. So easier reasoning and more > > > straightforward handling of this case (also less code). > > > > Would you suggest the other errors that can be returned synchronously > (ERR_INVALID_ARGUMENT, ERR_NAME_RESOLUTION_FAILED) also be returned in this way? > > > > I thought it was simpler having a single way to report errors (the callback), > especially since the likely response to being throttled (retry later) is the > same response you might want to take to other errors. However, if you aren't > worrying about retrying in those other scenarios, I can imagine that knowing > you're being throttled immediately could simplify calling code. > > Yes, I would suggest other errors are also returned this way. > It does simplify calling code, since as you pointed out, the caller would simply > stop calling this method. > Other errors should definitely not be re-tried - if the caller is not positive > that the return value indicates throttling, it may be a usage error (it could > DCHECK or simply drop the request). Ok, I'll make this change in a follow-up CL. With regards to other errors that might lead you to retry, I was thinking of transient network errors, e.g. no internet connection.
https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client.cc:137: qname << encoded_leaf_hash << ".hash." << domain_for_log << "."; As an aside, is ostringstream the right tool here? https://google.github.io/styleguide/cppguide.html#Streams (which used to outright forbid them) says "Use them when they're the right tool". But this just seems to be concatenating strings. Why not just std::string qname = encoded_leaf_hash; qname.append(".hash."); qname.append(domain_for_log); qname.append("."); (We don't have StrCat, otherwise that'd totally be the right tool) https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:587: "123456"); Is this value necessary as part of the test? The same applies to all of the tests that you add here - are the actual values meaningful? You're testing that queries can be made in parallel, which seems to be more about state transitions; it's unclear to me whether the values themselves are significant and relevant to the tests. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:607: } For what it's worth, I spent 20 minutes digging through code to try and convince myself that MockLeafIndexCallback was going to be safe if these parallel queries' callbacks are invoked out of order (e.g. started as 1-2-3 but finish as 3-2-1). The answer is that it's only safe because base::RunLoop is 'safe' to have Quit() called before Run() is called, and it terminates immediately. That said, I think the heart of my unease is that it feels very... weird... to be smuggling base::RunLoops on callbacks, considering that a base::RunLoop's intended purpose was with respect to nesting message loops, and we're totally not trying to do that here (they're independent state machines) https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:746: if (callback1.called()) { This is non-obvious to me why you have an EXPECT_TRUE but then subsequently EXPECT_THAT; why isn't this just ASSERT_TRUE ? That is, put differently, "Expect that this code will be called, but then check if we weren't called" is... unclear to me the testing value. Failing faster in the face of failure seems more beneficial.
https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client.cc:137: qname << encoded_leaf_hash << ".hash." << domain_for_log << "."; On 2016/09/16 02:11:03, Ryan Sleevi (slow) wrote: > As an aside, is ostringstream the right tool here? > > https://google.github.io/styleguide/cppguide.html#Streams (which used to > outright forbid them) says "Use them when they're the right tool". But this just > seems to be concatenating strings. Why not just > > std::string qname = encoded_leaf_hash; > qname.append(".hash."); > qname.append(domain_for_log); > qname.append("."); > > (We don't have StrCat, otherwise that'd totally be the right tool) One of the earlier iterations of this class did pretty much what you're suggesting, but a review comment suggested changing it. I'm happy to change it back though :) The comment was just a "nit" anyway. On the topic of StrCat, when I first wrote this I went looking for StrCat and was disappointed that it isn't in Chromium :( https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:587: "123456"); On 2016/09/16 02:11:03, Ryan Sleevi (slow) wrote: > Is this value necessary as part of the test? The same applies to all of the > tests that you add here - are the actual values meaningful? You're testing that > queries can be made in parallel, which seems to be more about state transitions; > it's unclear to me whether the values themselves are significant and relevant to > the tests. They're required to test that nothing goes quietly wrong when running queries in parallel, i.e. the queries don't fail, but just return the wrong data. These tests would be better if they used different inputs for each of the queries, and perhaps that would demonstrate that the values are meaningful. As it happens, I had already started on doing that. Along with extra documentation, would that address this issue? https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:607: } On 2016/09/16 02:11:03, Ryan Sleevi (slow) wrote: > For what it's worth, I spent 20 minutes digging through code to try and convince > myself that MockLeafIndexCallback was going to be safe if these parallel > queries' callbacks are invoked out of order (e.g. started as 1-2-3 but finish as > 3-2-1). > > The answer is that it's only safe because base::RunLoop is 'safe' to have Quit() > called before Run() is called, and it terminates immediately. > > That said, I think the heart of my unease is that it feels very... weird... to > be smuggling base::RunLoops on callbacks, considering that a base::RunLoop's > intended purpose was with respect to nesting message loops, and we're totally > not trying to do that here (they're independent state machines) This is another thing that was only added as a result of reviewer feedback when I originally wrote it. What would you recommend instead? https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:746: if (callback1.called()) { On 2016/09/16 02:11:03, Ryan Sleevi (slow) wrote: > This is non-obvious to me why you have an EXPECT_TRUE but then subsequently > EXPECT_THAT; why isn't this just ASSERT_TRUE ? > > That is, put differently, "Expect that this code will be called, but then check > if we weren't called" is... unclear to me the testing value. Failing faster in > the face of failure seems more beneficial. It's actually "Expect that this code will be called, *and* then check if we *were* called ...". It's because I'd like a failure of this test to tell me which of the two callbacks, if any, were called, rather than just telling me that the first one wasn't. This requires that I use EXPECT_*. Only if a callback has been called, is it then valid to check what it was called with.
https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client.cc:137: qname << encoded_leaf_hash << ".hash." << domain_for_log << "."; On 2016/09/16 16:28:19, Rob Percival wrote: > One of the earlier iterations of this class did pretty much what you're > suggesting, but a review comment suggested changing it. I'm happy to change it > back though :) The comment was just a "nit" anyway. Any idea what the review was? :) My inclination is that this isn't an approved use of ostringstream, but I'd be curious to understand the original nit. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:587: "123456"); On 2016/09/16 16:28:19, Rob Percival wrote: > They're required to test that nothing goes quietly wrong when running queries in > parallel, i.e. the queries don't fail, but just return the wrong data. These > tests would be better if they used different inputs for each of the queries, and > perhaps that would demonstrate that the values are meaningful. As it happens, I > had already started on doing that. Along with extra documentation, would that > address this issue? Right, with the current structure (using the same inputs) it doesn't seem like you're getting what you wanted to be tested. Using different inputs would make it more obvious that the inputs are themselves meaningful. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:607: } On 2016/09/16 16:28:19, Rob Percival wrote: > This is another thing that was only added as a result of reviewer feedback when > I originally wrote it. What would you recommend instead? Link to previous CL? https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:746: if (callback1.called()) { On 2016/09/16 16:28:19, Rob Percival wrote: > It's because I'd like a failure of this test to tell > me which of the two callbacks, if any, were called, rather than just telling me > that the first one wasn't. Why? In the face of failure, what new and additional information do you gain, versus the complexity in trying to figure out whether this is intentional or if it's an anti-pattern like DCHECK(thing) if (thing) [The above is prohibited]
https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client.cc:137: qname << encoded_leaf_hash << ".hash." << domain_for_log << "."; On 2016/09/17 01:51:52, Ryan Sleevi (slow) wrote: > On 2016/09/16 16:28:19, Rob Percival wrote: > > One of the earlier iterations of this class did pretty much what you're > > suggesting, but a review comment suggested changing it. I'm happy to change it > > back though :) The comment was just a "nit" anyway. > > Any idea what the review was? :) > > My inclination is that this isn't an approved use of ostringstream, but I'd be > curious to understand the original nit. The original nit suggested using StringPrintf instead of appending, but this has slightly different behaviour than appending or ostringstream (and that was undesirable, though I can't recall exactly what the issue was - I think it related to handling of NULL). The reviewer was happy with ostringstream as a compromise. However, according to the tests, Stringprintf is ok now so I'll use that. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:587: "123456"); On 2016/09/17 01:51:52, Ryan Sleevi (slow) wrote: > On 2016/09/16 16:28:19, Rob Percival wrote: > > They're required to test that nothing goes quietly wrong when running queries > in > > parallel, i.e. the queries don't fail, but just return the wrong data. These > > tests would be better if they used different inputs for each of the queries, > and > > perhaps that would demonstrate that the values are meaningful. As it happens, > I > > had already started on doing that. Along with extra documentation, would that > > address this issue? > > Right, with the current structure (using the same inputs) it doesn't seem like > you're getting what you wanted to be tested. Using different inputs would make > it more obvious that the inputs are themselves meaningful. Done. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:607: } On 2016/09/17 01:51:52, Ryan Sleevi (slow) wrote: > On 2016/09/16 16:28:19, Rob Percival wrote: > > This is another thing that was only added as a result of reviewer feedback > when > > I originally wrote it. What would you recommend instead? > > Link to previous CL? https://codereview.chromium.org/2066553002/#ps180001 https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:746: if (callback1.called()) { On 2016/09/17 01:51:52, Ryan Sleevi (slow) wrote: > On 2016/09/16 16:28:19, Rob Percival wrote: > > It's because I'd like a failure of this test to tell > > me which of the two callbacks, if any, were called, rather than just telling > me > > that the first one wasn't. > > Why? > > In the face of failure, what new and additional information do you gain, versus > the complexity in trying to figure out whether this is intentional or if it's an > anti-pattern like > > DCHECK(thing) > if (thing) > > [The above is prohibited] As discussed in https://testing.googleblog.com/2008/07/tott-expect-vs-assert.html, EXPECT gives you more information about why a test is failing. In this case, it's potentially useful to know that callback2 was called but callback1 wasn't, as that could hint at what the bug is. DCHECK and EXPECT should not be confused, as the former expresses a condition that should result in immediate termination, whereas EXPECT does not. I could expand the comment to hopefully avoid such confusion if you'd like?
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: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (id:140001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:607: } On 2016/09/20 18:54:42, Rob Percival wrote: > https://codereview.chromium.org/2066553002/#ps180001 Thanks. I think there was a breakdown in communication there. I agree with Matt that RunUntilIdle() is not a safe pattern to endorse here, for all the reasons he mentioned. The core of the 'surprise' here, which required digging into, is that you create a nested RunLoop context each time you create a MockLeafIndexCallback, which thus implies an ordering dependency that's not actually related. Compare with net::TestCompletionCallback, which creates a RunLoop on-demand to execute until task completion, unless the task has already completed. You can create as many net::TestCompletionCallback's as necessary (which do not create any RunLoops), and it's not until you attempt to WaitForResult that a RunLoop is created (and only if the result is not already available). Further, this code is also confusing, in that it's non-obvious that WaitUntilRun() will ever return if a callback is not called (c.f. line 603). There's nothing that will ever quit the specific callback runloop until it's Run - that is, you could NEVER hit 602/603 if 599 succeeded. And if it didn't succeed, it would run until eternity. In terms of concrete suggestions for how to change this, I think we should revisit this after we've adapted LogDnsClient to a more natural form of async programming, and then see if there are concrete suggestions to be made here. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:746: if (callback1.called()) { On 2016/09/20 18:54:42, Rob Percival wrote: > I could expand the comment to hopefully avoid such confusion if > you'd like? Apologies that I worded the question poorly, and perhaps did not give a clearer understanding of why I have considerable unease with this, to the point I'm not comfortable approving it as written. Concretely, I would prefer you write this as: ASSERT_TRUE(callback1.called()); EXPECT_THAT(callback1.net_error(), IsOk()); EXPECT_THAT(callback1.leaf_index(), Eq(123456u)); ASSERT_TRUE(callback2.called()); EXPECT_THAT(callback2.net_error(), IsError(net::ERR_TEMPORARILY_THROTTLED)); EXPECT_THAT(callback2.leaf_index(), Eq(0u)); My reasoning is simple: 1) I do not believe that the distinction (knowing that callback1 is not called, but callback2 is) is relevant or useful information to help diagnose the test. 2) Using that TOTT as an example, I do not believe it makes any sense to continue if callback1 has failed. The test will fail, and knowing information about callback2 is irrelevant (and likely to result in more noise than use). 3) Conditionals such as this are like 'debugging breadcrumbs'. Expect strong push back from me on any use of debugging breadcrumbs in Chrome code, as I do not believe in their value, from working on Chrome and other codebases. They add complexity (to reading, to writing, and to maintaining), their cost is not visible to the author (because they don't have to support it), and it's a bit like 'forward engineering' - you never know what debugging breadcrumbs are necessary until you're debugging, and you can't be sure what was necessary this time will be useful in the future. The above rewrite makes it clear that both callback1 and callback2 are expected to be called, but that there's no point in continuing the test if they're not. The only possible information that can be gleaned from this is that callback2 was called when callback1 was not called, but that's readily visible to anyone with a debugger, or can easily be investigated if they fail. To be clear, this logic applies to just about every tuple of EXPECT_THAT(conditional) + if(conditional) - I am not comfortable with any one of them in this CL, for the reasons above.
PTAL https://codereview.chromium.org/2331923003/diff/100001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:607: } On 2016/09/22 04:37:07, Ryan Sleevi (slow) wrote: > On 2016/09/20 18:54:42, Rob Percival wrote: > > https://codereview.chromium.org/2066553002/#ps180001 > > Thanks. I think there was a breakdown in communication there. > > I agree with Matt that RunUntilIdle() is not a safe pattern to endorse here, for > all the reasons he mentioned. The core of the 'surprise' here, which required > digging into, is that you create a nested RunLoop context each time you create a > MockLeafIndexCallback, which thus implies an ordering dependency that's not > actually related. > > Compare with net::TestCompletionCallback, which creates a RunLoop on-demand to > execute until task completion, unless the task has already completed. You can > create as many net::TestCompletionCallback's as necessary (which do not create > any RunLoops), and it's not until you attempt to WaitForResult that a RunLoop is > created (and only if the result is not already available). > > Further, this code is also confusing, in that it's non-obvious that > WaitUntilRun() will ever return if a callback is not called (c.f. line 603). > There's nothing that will ever quit the specific callback runloop until it's Run > - that is, you could NEVER hit 602/603 if 599 succeeded. And if it didn't > succeed, it would run until eternity. > > In terms of concrete suggestions for how to change this, I think we should > revisit this after we've adapted LogDnsClient to a more natural form of async > programming, and then see if there are concrete suggestions to be made here. Ah I see, TestCompletionCallback has a unique_ptr<RunLoop> and defers creating the RunLoop. Using RunUntilIdle, rather than Run, might actually be an improvement WRT ensuring WaitUntilRun terminates (though it's still not guaranteed). However, I won't change anything until the async-sync pattern CL as you suggest. https://codereview.chromium.org/2331923003/diff/100001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:746: if (callback1.called()) { On 2016/09/22 04:37:07, Ryan Sleevi (slow) wrote: > On 2016/09/20 18:54:42, Rob Percival wrote: > > I could expand the comment to hopefully avoid such confusion if > > you'd like? > > Apologies that I worded the question poorly, and perhaps did not give a clearer > understanding of why I have considerable unease with this, to the point I'm not > comfortable approving it as written. Concretely, I would prefer you write this > as: > > ASSERT_TRUE(callback1.called()); > EXPECT_THAT(callback1.net_error(), IsOk()); > EXPECT_THAT(callback1.leaf_index(), Eq(123456u)); > > ASSERT_TRUE(callback2.called()); > EXPECT_THAT(callback2.net_error(), IsError(net::ERR_TEMPORARILY_THROTTLED)); > EXPECT_THAT(callback2.leaf_index(), Eq(0u)); > > > My reasoning is simple: > 1) I do not believe that the distinction (knowing that callback1 is not called, > but callback2 is) is relevant or useful information to help diagnose the test. > 2) Using that TOTT as an example, I do not believe it makes any sense to > continue if callback1 has failed. The test will fail, and knowing information > about callback2 is irrelevant (and likely to result in more noise than use). > 3) Conditionals such as this are like 'debugging breadcrumbs'. Expect strong > push back from me on any use of debugging breadcrumbs in Chrome code, as I do > not believe in their value, from working on Chrome and other codebases. They add > complexity (to reading, to writing, and to maintaining), their cost is not > visible to the author (because they don't have to support it), and it's a bit > like 'forward engineering' - you never know what debugging breadcrumbs are > necessary until you're debugging, and you can't be sure what was necessary this > time will be useful in the future. > > The above rewrite makes it clear that both callback1 and callback2 are expected > to be called, but that there's no point in continuing the test if they're not. > > The only possible information that can be gleaned from this is that callback2 > was called when callback1 was not called, but that's readily visible to anyone > with a debugger, or can easily be investigated if they fail. > > To be clear, this logic applies to just about every tuple of > EXPECT_THAT(conditional) + if(conditional) - I am not comfortable with any one > of them in this CL, for the reasons above. Very well. I'm not entirely convinced, but I'll defer to your experience.
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.
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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.
My face is still melting from all the reviews, but this is still on my radar. https://codereview.chromium.org/2331923003/diff/240001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/240001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:96: EXPECT_TRUE(!called_) << "Callback invoked more than once"; I don't believe these comments are useful.
https://codereview.chromium.org/2331923003/diff/240001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/240001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:96: EXPECT_TRUE(!called_) << "Callback invoked more than once"; On 2016/10/03 23:57:29, Ryan Sleevi (slow) wrote: > I don't believe these comments are useful. 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...
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 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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #13 (id:300001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2331923003/diff/320001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/320001/components/certificate... components/certificate_transparency/log_dns_client.cc:8: #include "base/format_macros.h" AFAICT, it should be sufficient to just include <stdint.h> , since C++11 defines these. https://codereview.chromium.org/2331923003/diff/320001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/320001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:44: const char* kLeafHashes[] = { const char* const https://codereview.chromium.org/2331923003/diff/320001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:54: const char* kBase32LeafHashes[] = { const char* const
https://codereview.chromium.org/2331923003/diff/320001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2331923003/diff/320001/components/certificate... components/certificate_transparency/log_dns_client.cc:8: #include "base/format_macros.h" On 2016/10/07 15:46:23, Ryan Sleevi (slow) wrote: > AFAICT, it should be sufficient to just include <stdint.h> , since C++11 defines > these. Just tested this and I get a compilation error without format_macros.h. https://codereview.chromium.org/2331923003/diff/320001/components/certificate... File components/certificate_transparency/log_dns_client_unittest.cc (right): https://codereview.chromium.org/2331923003/diff/320001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:44: const char* kLeafHashes[] = { On 2016/10/07 15:46:23, Ryan Sleevi (slow) wrote: > const char* const Done. https://codereview.chromium.org/2331923003/diff/320001/components/certificate... components/certificate_transparency/log_dns_client_unittest.cc:54: const char* kBase32LeafHashes[] = { On 2016/10/07 15:46:23, Ryan Sleevi (slow) wrote: > const char* const 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...
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 robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2331923003/#ps340001 (title: "Addresses last of Ryan's comments")
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.
Committed patchset #14 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Allow LogDnsClient queries to be rate-limited A maximum number of concurrent queries can be specified. By default, there is no limit. BUG=624894 ========== to ========== Allow LogDnsClient queries to be rate-limited A maximum number of concurrent queries can be specified. By default, there is no limit. BUG=624894 Committed: https://crrev.com/2a9f5c09189a26b2f6140c02946bbd44ca9876f7 Cr-Commit-Position: refs/heads/master@{#424541} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/2a9f5c09189a26b2f6140c02946bbd44ca9876f7 Cr-Commit-Position: refs/heads/master@{#424541} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
