Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2348393002:
Minor improvements to MockLogDnsTraffic (Closed)
|
Created:
4 years, 3 months ago by Rob Percival Modified:
4 years, 2 months ago CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImproves documentation and formatting.
Replaces some test ASSERT/EXPECT calls with CHECK, as they would only be the result of incorrect test setup.
Small refactorings to improve encapsulation.
BUG=624894
Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c
Committed: https://crrev.com/3106d3fe1758a68145023d2047ebaf70e1b7cbcd
Cr-Original-Commit-Position: refs/heads/master@{#424984}
Cr-Commit-Position: refs/heads/master@{#425924}
Patch Set 1 #Patch Set 2 : More documentation about socket read mode of MockLogDnsTraffic #
Total comments: 2
Patch Set 3 : Document use of MessageLoop with MockLogDnsTraffic #
Total comments: 35
Patch Set 4 : Addresses Ryan's comments #Patch Set 5 : Forward declare MockSocketData #
Total comments: 6
Patch Set 6 : Rebase #Patch Set 7 : Addresses Ryan's comments #
Total comments: 2
Patch Set 8 : Rebase #Patch Set 9 : Addresses the last of Ryan's comments #Patch Set 10 : Changes DCHECK to CHECK #
Messages
Total messages: 49 (27 generated)
robpercival@chromium.org changed reviewers: + eranm@chromium.org
PTAL
lgtm, the example could be a bit more verbose though (see comment). Off to Ryan for approval. https://codereview.chromium.org/2348393002/diff/20001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/20001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:92: // log_client.QueryLeafIndex("ct.test", ...); Which message loop should be pumped to guarantee the callback has been invoked?
Description was changed from ========== Add example usage to documentation for MockLogDnsTraffic Also documents the default socket read mode. BUG=624894 ========== to ========== Add example usage to documentation for MockLogDnsTraffic Also documents the default socket read mode. BUG=624894 ==========
eranm@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/2348393002/diff/20001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/20001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:92: // log_client.QueryLeafIndex("ct.test", ...); On 2016/09/20 14:31:04, Eran Messeri wrote: > Which message loop should be pumped to guarantee the callback has been invoked? I've added a few more lines to the example usage to show that a MessageLoopWithIO should be used.
On 2016/09/20 16:33:12, Rob Percival wrote: > https://codereview.chromium.org/2348393002/diff/20001/components/certificate_... > File components/certificate_transparency/mock_log_dns_traffic.h (right): > > https://codereview.chromium.org/2348393002/diff/20001/components/certificate_... > components/certificate_transparency/mock_log_dns_traffic.h:92: // > log_client.QueryLeafIndex("ct.test", ...); > On 2016/09/20 14:31:04, Eran Messeri wrote: > > Which message loop should be pumped to guarantee the callback has been > invoked? > > I've added a few more lines to the example usage to show that a > MessageLoopWithIO should be used. Thanks, still lgtm.
https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:24: // A container for all of the data we need to keep alive for a mock socket. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... // A container for the data needed for simulating a mock socket. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:27: // so we have to manage the lifetime of those arguments ourselves. Wrapping all // so it is necessary to manage the lifetime of those arguments. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:30: // vector<unique_ptr<MockSocketData>> member, which requires MockSocketData be Both vector and unique_ptr should allow you to forward declare, so long as the constructors are out-of-line. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:35: MockSocketData(const std::vector<char>& write, const std::vector<char>& read); Isn't base::StringPiece the preferred data structure here? https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:50: static const net::MockRead no_more_data_; class-level private statics do not provide any advantages over file-local statics in an unnamed namespace. That is, no one can reach into it, it's still a global (just by another name), but at least that way, your implementation details don't leak into your header. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:57: net::MockWrite expected_write_; Line breaks between 55-56, 57-58, 64-65 will all help readability. Consider this code review interface, which doesn't support any syntax highlighting. Unless actively focused on the text, the structure is not visible, and the ability to rest the eyes on a particular section requires more effort. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:80: // // Create a mock NetworkChangeNotifier to propogate DNS config. spelling: propagate https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:82: // net::NetworkChangeNotifier::CreateMock(); pedantic nit: ( ) over = here since CreateMock returns a raw pointer https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:87: // MockLogDnsTraffic mock_dns; This is a fake, not a mock. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:89: // // Use the Expect* methods to define expected DNS requests and responses. Naming wise, this feels uncomfortable with //net paradigms, and suggests a pattern that is like GTest/Gmock, but very much isn't. That is, it doesn't setup any test expectations, AFAICT. The use of EXPECT_TRUE in mock_log_dns_traffic.cc is something I would have flagged and pushed back on; I believe that EXPECT/ASSERT should be used in tests, not test helpers. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:95: // LogDnsClient::QueryLeafIndexCallback callback = base::Bind(...); As mentioned to Eran in a recent CL, I'm very much opposed to these sorts of local-callbacks. log_client.QueryLeafIndex("ct.test", ..., base::Bind(...)); would suffice. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:99: // // the result of the leaf index query. You're making an API contract that RunUntilIdle is sufficient, but I don't believe it is. I would instead remove all of 98-100. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:111: // Expect a CT DNS request for the domain |qname|. Linebreak between 110-111 https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:115: // Expect a CT DNS request for the domain |qname|. Linebreak https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:118: void ExpectRequestAndTimeout(base::StringPiece qname); Line break https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:125: // Expect a CT DNS request for the domain |qname|. Line break https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:153: // is to test that LogDnsClient handles both modes in the same way. DESIGN: I would expect that this would be private, with friended for the tests.
Description was changed from ========== Add example usage to documentation for MockLogDnsTraffic Also documents the default socket read mode. BUG=624894 ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 ==========
PTAL https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:24: // A container for all of the data we need to keep alive for a mock socket. On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // A container for the data needed for simulating a mock socket. Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:27: // so we have to manage the lifetime of those arguments ourselves. Wrapping all On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > // so it is necessary to manage the lifetime of those arguments. Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:30: // vector<unique_ptr<MockSocketData>> member, which requires MockSocketData be On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > Both vector and unique_ptr should allow you to forward declare, so long as the > constructors are out-of-line. Both vector and unique_ptr are templates, so they can't possibly have out-of-line constructors. I assume I'm misunderstanding you - could you clarify please? https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:35: MockSocketData(const std::vector<char>& write, const std::vector<char>& read); On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > Isn't base::StringPiece the preferred data structure here? One of the main purposes of this class is to manage the lifetime of these arguments. If I change these to be StringPiece, I'll need to immediately convert them into std::vector<char> or std::string. Since they're built from std::vector<char> in the first place, it seems logical to keep them this way. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:50: static const net::MockRead no_more_data_; On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > class-level private statics do not provide any advantages over file-local > statics in an unnamed namespace. > > That is, no one can reach into it, it's still a global (just by another name), > but at least that way, your implementation details don't leak into your header. Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:57: net::MockWrite expected_write_; On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > Line breaks between 55-56, 57-58, 64-65 will all help readability. > > Consider this code review interface, which doesn't support any syntax > highlighting. Unless actively focused on the text, the structure is not visible, > and the ability to rest the eyes on a particular section requires more effort. Ah I forget that there's so syntax highlighting in Rietveld by default. You might want to install the Rietveld Usability Toolkit (https://chrome.google.com/webstore/detail/rietveld-usability-toolki/nmljjlfbn...), which adds syntax highlighting and some other nice features too. I've added the suggested line breaks anyway though, as it's a good idea. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:80: // // Create a mock NetworkChangeNotifier to propogate DNS config. On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > spelling: propagate Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:82: // net::NetworkChangeNotifier::CreateMock(); On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > pedantic nit: ( ) over = here since CreateMock returns a raw pointer Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:87: // MockLogDnsTraffic mock_dns; On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > This is a fake, not a mock. Based on most definitions I can find, it seems more like a mock. You set expected inputs and it verifies that they are provided, then passes back some predefined output. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:89: // // Use the Expect* methods to define expected DNS requests and responses. On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > Naming wise, this feels uncomfortable with //net paradigms, and suggests a > pattern that is like GTest/Gmock, but very much isn't. That is, it doesn't setup > any test expectations, AFAICT. > > The use of EXPECT_TRUE in mock_log_dns_traffic.cc is something I would have > flagged and pushed back on; I believe that EXPECT/ASSERT should be used in > tests, not test helpers. It actually does setup test expectations, if you look deep inside socket_test_util.cc (https://cs.chromium.org/chromium/src/net/socket/socket_test_util.cc?rcl=14752...). That's probably something you'd have pushed back on equally hard though. I'm happy to change the EXPECT/ASSERTs in mock_log_dns_traffic.cc into DCHECKs, as they're really only things that'll happen as the result of setting up expectations incorrectly (e.g. expected inputs/outputs that can't fit into a packet). https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:95: // LogDnsClient::QueryLeafIndexCallback callback = base::Bind(...); On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > As mentioned to Eran in a recent CL, I'm very much opposed to these sorts of > local-callbacks. > > log_client.QueryLeafIndex("ct.test", ..., base::Bind(...)); > > would suffice. Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:99: // // the result of the leaf index query. On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > You're making an API contract that RunUntilIdle is sufficient, but I don't > believe it is. > > I would instead remove all of 98-100. Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:111: // Expect a CT DNS request for the domain |qname|. On 2016/09/23 21:29:46, Ryan Sleevi (slow) wrote: > Linebreak between 110-111 Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:115: // Expect a CT DNS request for the domain |qname|. On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > Linebreak Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:118: void ExpectRequestAndTimeout(base::StringPiece qname); On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > Line break Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:125: // Expect a CT DNS request for the domain |qname|. On 2016/09/23 21:29:46, Ryan Sleevi (slow) wrote: > Line break Done. https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:153: // is to test that LogDnsClient handles both modes in the same way. On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > DESIGN: I would expect that this would be private, with friended for the tests. Done.
https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/40001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:30: // vector<unique_ptr<MockSocketData>> member, which requires MockSocketData be On 2016/10/03 13:35:45, Rob Percival wrote: > On 2016/09/23 21:29:47, Ryan Sleevi (slow) wrote: > > Both vector and unique_ptr should allow you to forward declare, so long as the > > constructors are out-of-line. > > Both vector and unique_ptr are templates, so they can't possibly have > out-of-line constructors. I assume I'm misunderstanding you - could you clarify > please? Never mind, fixed.
https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.cc (right): https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:24: // Prevents read overruns and makes a socket timeout the default behaviour. How? That's perhaps the subtle part of this code, where the comment can expand :) // Simulates a 'hanging read'. This is used as the last mock response // as a sentinel to prevent reading more data than expected; any caller // who does will ultimately timeout. (However, it's not clear why this is better than, say, just returning an explicit net::Error) https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:47: DCHECK(net::DNSDomainFromDot(qname, &encoded_qname)); Don't DCHECK in tests. Throughout. https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:101: // Forward declaration of a class that encapsulates socket simulation data. I would say this is an unnecessary comment, but I appreciate the *over* commenting :)
https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.cc (right): https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:24: // Prevents read overruns and makes a socket timeout the default behaviour. On 2016/10/03 23:44:52, Ryan Sleevi (slow) wrote: > How? That's perhaps the subtle part of this code, where the comment can expand > :) > > // Simulates a 'hanging read'. This is used as the last mock response > // as a sentinel to prevent reading more data than expected; any caller > // who does will ultimately timeout. > > (However, it's not clear why this is better than, say, just returning an > explicit net::Error) It's consistent with the other code I saw using mock sockets. However, I prefer the idea of just returning an error, so I'll change it to do that. https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.cc:47: DCHECK(net::DNSDomainFromDot(qname, &encoded_qname)); On 2016/10/03 23:44:52, Ryan Sleevi (slow) wrote: > Don't DCHECK in tests. Throughout. Please see https://chromiumcodereview-hr.appspot.com/2389993003/ as a possible resolution. https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/80001/components/certificate_... components/certificate_transparency/mock_log_dns_traffic.h:101: // Forward declaration of a class that encapsulates socket simulation data. On 2016/10/03 23:44:52, Ryan Sleevi (slow) wrote: > I would say this is an unnecessary comment, but I appreciate the *over* > commenting :) 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.
LGTM % comments on the other CL (re: DCHECK vs EXPECT/ASSERT) https://codereview.chromium.org/2348393002/diff/120001/components/certificate... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/120001/components/certificate... components/certificate_transparency/mock_log_dns_traffic.h:33: // base::MessageLoopForIO message_loop; Nit: I think we should move 32-33 before 28-30; just that messageloops are generally the first thing we expect to setup.
https://codereview.chromium.org/2348393002/diff/120001/components/certificate... File components/certificate_transparency/mock_log_dns_traffic.h (right): https://codereview.chromium.org/2348393002/diff/120001/components/certificate... components/certificate_transparency/mock_log_dns_traffic.h:33: // base::MessageLoopForIO message_loop; On 2016/10/07 14:29:18, Ryan Sleevi (slow) wrote: > Nit: I think we should move 32-33 before 28-30; just that messageloops are > generally the first thing we expect to setup. 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 eranm@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2348393002/#ps160001 (title: "Addresses the 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.
Description was changed from ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2415063002/ by msramek@chromium.org. The reason for reverting is: This broke a lot of LogDnsClientTest tests on all platforms: https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/29988 https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/47542 https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui....
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 424984 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
Patchset #10 (id:180001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ==========
On 2016/10/13 09:44:50, findit-for-me wrote: > FYI: Findit identified this CL at revision 424984 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... Oddly, most of the tests fail with net::ERR_UNEXPECTED in a release build with DCHECKs off. A release build with DCHECKs on results in the tests passing.
On 2016/10/13 16:17:06, Rob Percival wrote: > On 2016/10/13 09:44:50, findit-for-me wrote: > > FYI: Findit identified this CL at revision 424984 as the culprit for > > failures in the build cycles as shown on: > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > Oddly, most of the tests fail with net::ERR_UNEXPECTED in a release build with > DCHECKs off. A release build with DCHECKs on results in the tests passing. Ah of course, all of the DCHECK lines aren't being evaluated in release builds with DCHECK off. I'll revert that part of the change.
On 2016/10/13 16:17:06, Rob Percival wrote: > On 2016/10/13 09:44:50, findit-for-me wrote: > > FYI: Findit identified this CL at revision 424984 as the culprit for > > failures in the build cycles as shown on: > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > Oddly, most of the tests fail with net::ERR_UNEXPECTED in a release build with > DCHECKs off. A release build with DCHECKs on results in the tests passing. Ah of course, all of the DCHECK lines aren't being evaluated in release builds with DCHECK off. I'll revert that part of the change.
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...
Description was changed from ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with DCHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with CHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ==========
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 eranm@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2348393002/#ps200001 (title: "Changes DCHECK to CHECK")
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 ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with CHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with CHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with CHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Cr-Commit-Position: refs/heads/master@{#424984} ========== to ========== Improves documentation and formatting. Replaces some test ASSERT/EXPECT calls with CHECK, as they would only be the result of incorrect test setup. Small refactorings to improve encapsulation. BUG=624894 Committed: https://crrev.com/f02172072e3fd83e2296e984f1e5bd86ba40093c Committed: https://crrev.com/3106d3fe1758a68145023d2047ebaf70e1b7cbcd Cr-Original-Commit-Position: refs/heads/master@{#424984} Cr-Commit-Position: refs/heads/master@{#425924} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3106d3fe1758a68145023d2047ebaf70e1b7cbcd Cr-Commit-Position: refs/heads/master@{#425924} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
