Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3277008:
add safebrowsing testserver (Closed)
|
Created:
10 years, 3 months ago by lzheng Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, Paul Godavari, gcasto (DO NOT USE) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd the dependency with safebrowsing's test suite server and make safebrowsing browsertest run end to end test with the test server.
BUG=47318
TEST=this is the test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=63021
Patch Set 1 : '' #
Total comments: 21
Patch Set 2 : '' #
Total comments: 28
Patch Set 3 : '' #
Total comments: 45
Patch Set 4 : address shess's comments #
Total comments: 7
Patch Set 5 : Get rid of pinger, address more comments from shess #
Total comments: 7
Patch Set 6 : '' #
Total comments: 12
Patch Set 7 : '' #
Total comments: 9
Patch Set 8 : '' #Patch Set 9 : 'python_util #
Total comments: 26
Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 31
Patch Set 14 : address Eric's comments #Patch Set 15 : resync before submitting #Patch Set 16 : resync before submitting #
Messages
Total messages: 25 (0 generated)
Add the full end to end test. The code is ready for review. However, before I can submit, I am still waiting for two changes from server: 1. Need to make the test data small, otherwise the test data set is too big, this browser test takes more than 30 seconds to finish. 2. The server need to use 'rb' to read the test data.
Please do not commit without my explicit "LGTM". http://codereview.chromium.org/3277008/diff/13001/14001 File DEPS (right): http://codereview.chromium.org/3277008/diff/13001/14001#newcode52 DEPS:52: "src/third_party/safe_browsing": Have you verified with tools/licenses.py scan that the README.chromium paperwork is correct for this directory? http://codereview.chromium.org/3277008/diff/13001/14002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/13001/14002#newcode88 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:88: L"\"" + python_runtime.ToWStringHack() + L"\" " + Can we share more of this code with net/test/test_server? It's mostly copy-pasted here, which is bad. http://codereview.chromium.org/3277008/diff/13001/14002#newcode226 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:226: LOG(ERROR) << "Unexpected URL format in phishing URL list: " Shouldn't we return false or something? http://codereview.chromium.org/3277008/diff/13001/14002#newcode235 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:235: CHECK_EQ("no", url_parts[2]); To avoid crashiness, please remove this check. You can return false on error. http://codereview.chromium.org/3277008/diff/13001/14002#newcode266 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:266: CHECK(safe_browsing_service_); Similarly here. http://codereview.chromium.org/3277008/diff/13001/14002#newcode406 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:406: void OnForceUpdateDone() { Why not just use StopUILoop directly? http://codereview.chromium.org/3277008/diff/13001/14002#newcode428 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:428: void OnCheckUrlOnIOThreadDone() { Same here. http://codereview.chromium.org/3277008/diff/13001/14002#newcode561 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:561: for (int step = 1; step < kMaxSteps; step++) { Why do we need this loop? http://codereview.chromium.org/3277008/diff/13001/14002#newcode575 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:575: do { Why do we need this loop?
Pawel: Thanks for your good comments. Please see comments inline. http://codereview.chromium.org/3277008/diff/13001/14001 File DEPS (right): http://codereview.chromium.org/3277008/diff/13001/14001#newcode52 DEPS:52: "src/third_party/safe_browsing": On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Have you verified with tools/licenses.py scan that the README.chromium paperwork > is correct for this directory? Done. I added a README.chromium Nice to know there is such a tool. There is nowhere this is metioned, we should update the third_party read me to point this out. http://codereview.chromium.org/3277008/diff/13001/14002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/13001/14002#newcode88 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:88: L"\"" + python_runtime.ToWStringHack() + L"\" " + On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Can we share more of this code with net/test/test_server? It's mostly > copy-pasted here, which is bad. They are actually different servers and happen to all have commandline flags "port" and "datafile", that should not be shared since they could have different parameters later. We can create a python_util.cc file under test_suppport_common that will have "AppendToPythonPath" and "python_runtime(for windows)" though. I can happily do that but hope we can get that in a different change. http://codereview.chromium.org/3277008/diff/13001/14002#newcode226 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:226: LOG(ERROR) << "Unexpected URL format in phishing URL list: " On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Shouldn't we return false or something? Yes. Done. http://codereview.chromium.org/3277008/diff/13001/14002#newcode235 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:235: CHECK_EQ("no", url_parts[2]); On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > To avoid crashiness, please remove this check. You can return false on error. Done. http://codereview.chromium.org/3277008/diff/13001/14002#newcode266 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:266: CHECK(safe_browsing_service_); On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Similarly here. This is just to make sure the safe_browsing_service_ is initiated. It serves as an API guard, not for testing. I am now using ASSERT_TRUE(InitSafeBrowsingService()) to make sure we could get out of this test if safebrowsing service is not initialized correctly. This should address your concern. http://codereview.chromium.org/3277008/diff/13001/14002#newcode406 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:406: void OnForceUpdateDone() { On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Why not just use StopUILoop directly? I was directly call "StopUILoop()" but I changed to what we have now to make the code more readible. E.g.: ForceUpdate will finish with OnForceUpdateDone, so we know when and where async call finishes. http://codereview.chromium.org/3277008/diff/13001/14002#newcode561 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:561: for (int step = 1; step < kMaxSteps; step++) { On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Why do we need this loop? Good question. I updated the beginning of this file to give an overview of this test and its flow. Basically, we will start several update requests to the test server and in each step, we verify if the safebrowsing database is correct. http://codereview.chromium.org/3277008/diff/13001/14002#newcode575 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:575: do { On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > Why do we need this loop? I updated the comment here too. We need to wait till the update finishes getting all chunks.
http://codereview.chromium.org/3277008/diff/13001/14001 File DEPS (right): http://codereview.chromium.org/3277008/diff/13001/14001#newcode52 DEPS:52: "src/third_party/safe_browsing": On 2010/09/10 22:52:59, lzheng wrote: > On 2010/09/10 17:53:49, Paweł Hajdan Jr. wrote: > > Have you verified with tools/licenses.py scan that the README.chromium > paperwork > > is correct for this directory? > Done. I added a README.chromium > Nice to know there is such a tool. There is nowhere this is metioned, we should > update the third_party read me to point this out. > My plan is to get this tool as one of our build steps once we fix all the existing third_party directories. See http://code.google.com/p/chromium/issues/detail?id=28291 http://codereview.chromium.org/3277008/diff/13001/14002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/13001/14002#newcode88 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:88: L"\"" + python_runtime.ToWStringHack() + L"\" " + On 2010/09/10 22:52:59, lzheng wrote: > We can create a python_util.cc file under test_suppport_common that will have > "AppendToPythonPath" and "python_runtime(for windows)" though. I can happily do > that but hope we can get that in a different change. Please find some way to share the code. I'd be very reluctant to deferring this to a separate CL. In case the dedup effort is delayed, we'd end up with two diverging parts of the code. http://codereview.chromium.org/3277008/diff/13001/14002#newcode406 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:406: void OnForceUpdateDone() { On 2010/09/10 22:52:59, lzheng wrote: > I was directly call "StopUILoop()" but I changed to what we have now to make the > code more readible. E.g.: ForceUpdate will finish with OnForceUpdateDone, so we > know when and where async call finishes. I'm fine with that. http://codereview.chromium.org/3277008/diff/18001/17003#newcode10 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:10: #include "base/process_util.h" nit: typo - summary http://codereview.chromium.org/3277008/diff/18001/17003#newcode11 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:11: #include "base/string_number_conversions.h" nit: Let's not include the port number here. We might want to start changing it at some point (for example to run more tests in parallel). http://codereview.chromium.org/3277008/diff/18001/17003#newcode48 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:48: EXPECT_EQ(server_handle_, base::kNullProcessHandle); Can we extract the test server to a separate file? http://codereview.chromium.org/3277008/diff/18001/17003#newcode354 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:354: Lock update_status_mutex_; We should ask the test server to construct URL. This will allow us more flexibility with the test server. http://codereview.chromium.org/3277008/diff/18001/17003#newcode451 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:451: StopUILoop(); nit: CHECK/DCHECK on CurrentlyOn... please make those consistent. I suggest using DCHECK, because that's what we do in other parts of the codebase. http://codereview.chromium.org/3277008/diff/18001/17003#newcode559 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:559: int last_step = 0; If you need timeouts, please refer to chrome/test/test_timeouts (added today). Never hardcode timeouts in a test file. http://codereview.chromium.org/3277008/diff/18001/17003#newcode583 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:583: if (wait_loops > kMaxWaitSec * 1000 / kWaitMsec) { Is each step somehow different, or are those multiple iterations of the same thing? http://codereview.chromium.org/3277008/diff/18001/17003#newcode598 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:598: Such kind of loop is a bad method to wait. Determine an event or condition you need to wait for, and wait for it. Do not spin the message loop multiple times until things are happy. The rationale here is that there is a correlation between waiting in a bad way and unreliable tests. http://codereview.chromium.org/3277008/diff/18001/17003#newcode646 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:646: I think it could be more intuitive if the test helper would run the loop if needed, instead of doing so by the caller. Imagine what happens if the caller forgets to spin the message loop... Weird things may happen. http://codereview.chromium.org/3277008/diff/18001/17003#newcode648 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:648: nit: Please do not use C-style casts. http://codereview.chromium.org/3277008/diff/18001/17003#newcode653 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:653: You should ask test server for these, and do not rely on the constants. This gives us more flexibility to change the test server. http://codereview.chromium.org/3277008/diff/18001/17005 File third_party/safe_browsing/README.chromium (right): http://codereview.chromium.org/3277008/diff/18001/17005#newcode4 third_party/safe_browsing/README.chromium:4: License File: LICENSE nit: This line shouldn't be needed, LICENSE is the default value. Up to you, I'm fine if it's still here.
Updated with latest change. Please take another look. http://codereview.chromium.org/3277008/diff/13001/14002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/13001/14002#newcode88 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:88: L"\"" + python_runtime.ToWStringHack() + L"\" " + On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > On 2010/09/10 22:52:59, lzheng wrote: > > We can create a python_util.cc file under test_suppport_common that will have > > "AppendToPythonPath" and "python_runtime(for windows)" though. I can happily > do > > that but hope we can get that in a different change. > > Please find some way to share the code. I'd be very reluctant to deferring this > to a separate CL. In case the dedup effort is delayed, we'd end up with two > diverging parts of the code. I am sending http://codereview.chromium.org/3366026 to you to review. To avoid bloating this cl, we can either submit that CL first, rebase this Cl with that change. For now, to make this CL compile and work, I am going to keep the redundant code here. http://codereview.chromium.org/3277008/diff/18001/17003#newcode10 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:10: #include "base/process_util.h" On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > nit: typo - summary Done. http://codereview.chromium.org/3277008/diff/18001/17003#newcode11 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:11: #include "base/string_number_conversions.h" On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > nit: Let's not include the port number here. We might want to start changing it > at some point (for example to run more tests in parallel). Done. http://codereview.chromium.org/3277008/diff/18001/17003#newcode48 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:48: EXPECT_EQ(server_handle_, base::kNullProcessHandle); On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > Can we extract the test server to a separate file? I don't foresee any other test will use this test server. So, I will keep it in this file for now. http://codereview.chromium.org/3277008/diff/18001/17003#newcode354 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:354: Lock update_status_mutex_; On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > We should ask the test server to construct URL. This will allow us more > flexibility with the test server. These urls are part of the hard coded data set in "testing_input.data" which we ask server to load. It should be composed in the test so we can change it based on test data. Even we have different servers, the url could be different depending on which data we want to load. http://codereview.chromium.org/3277008/diff/18001/17003#newcode451 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:451: StopUILoop(); On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > nit: CHECK/DCHECK on CurrentlyOn... please make those consistent. I suggest > using DCHECK, because that's what we do in other parts of the codebase. Good idea. In stead, I swiched everything to ASSERT_TRUE(), which is proper for tests. http://codereview.chromium.org/3277008/diff/18001/17003#newcode559 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:559: int last_step = 0; On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > If you need timeouts, please refer to chrome/test/test_timeouts (added today). > Never hardcode timeouts in a test file. Good to know. I have some question regarding that class. There are different reasons for timeouts, and I don't see several timeouts in that class will cover all cases. Once a lot of different places picked up a given timeout in that class, it will be hard to adjust the time value since one value will affect many places and it might be hard to pick a value that is optimal for everywhere. http://codereview.chromium.org/3277008/diff/18001/17003#newcode583 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:583: if (wait_loops > kMaxWaitSec * 1000 / kWaitMsec) { On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > Is each step somehow different, or are those multiple iterations of the same > thing? They will be different. http://codereview.chromium.org/3277008/diff/18001/17003#newcode598 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:598: On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > Such kind of loop is a bad method to wait. Determine an event or condition you > need to wait for, and wait for it. Do not spin the message loop multiple times > until things are happy. The rationale here is that there is a correlation > between waiting in a bad way and unreliable tests. You have valid concerns. However, we can't simply wait for a event here. This while loop allows us to pull safebrowsing server and update several states. It simulates the real life working flow to make sure our test is as valid as possible. To make a wait for event (such as condition variable), we have to change the safebrowsing server a lot to send notifications when each of the state changes. It will introduce more complications to the server which are not necessary. In this case, if the test failed due to timeout, it should be a valid failure since it means either the server is not responding or the update flow is stopped/jammed. http://codereview.chromium.org/3277008/diff/18001/17003#newcode646 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:646: On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > I think it could be more intuitive if the test helper would run the loop if > needed, instead of doing so by the caller. Imagine what happens if the caller > forgets to spin the message loop... Weird things may happen. Good suggestion. I moved the RunMessageLoop() to each proper function. http://codereview.chromium.org/3277008/diff/18001/17003#newcode648 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:648: On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > nit: Please do not use C-style casts. Done. http://codereview.chromium.org/3277008/diff/18001/17003#newcode653 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:653: On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > You should ask test server for these, and do not rely on the constants. This > gives us more flexibility to change the test server. This is one of the hard coded state in the test data. Depending on the test data we want to load, this step could be unnecessary.
http://codereview.chromium.org/3277008/diff/18001/17003 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/18001/17003#newcode48 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:48: class SafeBrowsingTestServer { On 2010/09/14 06:39:20, lzheng wrote: > On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > > Can we extract the test server to a separate file? > > I don't foresee any other test will use this test server. So, I will keep it in > this file for now. Ok, that's fine. http://codereview.chromium.org/3277008/diff/18001/17003#newcode354 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:354: switches::kSbInfoURLPrefix, "http://localhost:40101/safebrowsing"); On 2010/09/14 06:39:20, lzheng wrote: > On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > > We should ask the test server to construct URL. This will allow us more > > flexibility with the test server. > > These urls are part of the hard coded data set in "testing_input.data" which we > ask server to load. It should be composed in the test so we can change it based > on test data. Even we have different servers, the url could be different > depending on which data we want to load. Can we generate the test data then? Hardcoding those URLs and port numbers is relying on implementation details of TestServer, which may change and are going to change. http://codereview.chromium.org/3277008/diff/18001/17003#newcode559 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:559: const int kMaxWaitSec = 30; On 2010/09/14 06:39:20, lzheng wrote: > On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > > If you need timeouts, please refer to chrome/test/test_timeouts (added today). > > Never hardcode timeouts in a test file. > Good to know. I have some question regarding that class. There are different > reasons for timeouts, and I don't see several timeouts in that class will cover > all cases. Once a lot of different places picked up a given timeout in that > class, it will be hard to adjust the time value since one value will affect many > places and it might be hard to pick a value that is optimal for everywhere. Well, I probably don't care too much whether the timeout is 2 seconds or 3 seconds. Or 300 ms or 150 ms... I think that unifying those and making it possible to adjust those timeouts from command-line (e.g. for Valgrind) gives us more benefits than hand-tuning all timeouts to be super-optimal for a particular test. And then you still may need to adjust them for Valgrind... http://codereview.chromium.org/3277008/diff/18001/17003#newcode583 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:583: for (int step = 1; step < kMaxSteps; step++) { On 2010/09/14 06:39:20, lzheng wrote: > On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > > Is each step somehow different, or are those multiple iterations of the same > > thing? > > They will be different. Could you add a comment to make it super-clear? http://codereview.chromium.org/3277008/diff/18001/17003#newcode598 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:598: do { // Wait for the update to finish. On 2010/09/14 06:39:20, lzheng wrote: > On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: > > Such kind of loop is a bad method to wait. Determine an event or condition you > > need to wait for, and wait for it. Do not spin the message loop multiple times > > until things are happy. The rationale here is that there is a correlation > > between waiting in a bad way and unreliable tests. > You have valid concerns. However, we can't simply wait for a event here. This > while loop allows us to pull safebrowsing server and update several states. It > simulates the real life working flow to make sure our test is as valid as > possible. To make a wait for event (such as condition variable), we have to > change the safebrowsing server a lot to send notifications when each of the > state changes. It will introduce more complications to the server which are not > necessary. I don't think so. My experience so far is that it's less complicated than people believe initially. Could you experiment with waiting for an event, so that we can compare the code? > In this case, if the test failed due to timeout, it should be a valid failure > since it means either the server is not responding or the update flow is > stopped/jammed. Well, and the timeout is suspicious. We should not hardcode things like kMaxWaitSec. http://codereview.chromium.org/3277008/diff/37001/24014#newcode190 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:190: } No, no, no. You can't duplicate this code. For example, David Benjamin has a CL in review that makes it much more reliable, and also faster. Please find a way to share more code with net::TestServer. Sharing the pythonpath-related code is good, but I'd like to see more.
On Tue, Sep 14, 2010 at 10:30 AM, <phajdan.jr@chromium.org> wrote: > > http://codereview.chromium.org/3277008/diff/18001/17003 > File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): > > http://codereview.chromium.org/3277008/diff/18001/17003#newcode48 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:48: class > SafeBrowsingTestServer { > > On 2010/09/14 06:39:20, lzheng wrote: > >> On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: >> > Can we extract the test server to a separate file? >> > > I don't foresee any other test will use this test server. So, I will >> > keep it in > >> this file for now. >> > > Ok, that's fine. > > > http://codereview.chromium.org/3277008/diff/18001/17003#newcode354 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:354: > switches::kSbInfoURLPrefix, "http://localhost:40101/safebrowsing"); > > On 2010/09/14 06:39:20, lzheng wrote: > >> On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: >> > We should ask the test server to construct URL. This will allow us >> > more > >> > flexibility with the test server. >> > > These urls are part of the hard coded data set in "testing_input.data" >> > which we > >> ask server to load. It should be composed in the test so we can change >> > it based > >> on test data. Even we have different servers, the url could be >> > different > >> depending on which data we want to load. >> > > Can we generate the test data then? Hardcoding those URLs and port > numbers is relying on implementation details of TestServer, which may change and are going to change. This data is published by safebrowsing team to an open source repository. When we launch the test server, we can let it load different data and we can specify different url prefixes for that data. That is what we do now. This should not bounded to the server, the server here only reads the data. Instead, the data should be bounded to tests, which is what we do now. > > > http://codereview.chromium.org/3277008/diff/18001/17003#newcode559 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:559: const int > kMaxWaitSec = 30; > > On 2010/09/14 06:39:20, lzheng wrote: > >> On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: >> > If you need timeouts, please refer to chrome/test/test_timeouts >> > (added today). > >> > Never hardcode timeouts in a test file. >> Good to know. I have some question regarding that class. There are >> > different > >> reasons for timeouts, and I don't see several timeouts in that class >> > will cover > >> all cases. Once a lot of different places picked up a given timeout in >> > that > >> class, it will be hard to adjust the time value since one value will >> > affect many > >> places and it might be hard to pick a value that is optimal for >> > everywhere. > > Well, I probably don't care too much whether the timeout is 2 seconds or > 3 seconds. Or 300 ms or 150 ms... I think that unifying those and making > it possible to adjust those timeouts from command-line (e.g. for > Valgrind) gives us more benefits than hand-tuning all timeouts to be > super-optimal for a particular test. And then you still may need to > adjust them for Valgrind... I got rid of the kMaxWaitSec and uses BrowserTest's default time out now. Hope that will make your effort of unifying timeout easier. > > > http://codereview.chromium.org/3277008/diff/18001/17003#newcode583 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:583: for (int > > step = 1; step < kMaxSteps; step++) { > On 2010/09/14 06:39:20, lzheng wrote: > >> On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: >> > Is each step somehow different, or are those multiple iterations of >> > the same > >> > thing? >> > > They will be different. >> > > Could you add a comment to make it super-clear? Done. > > > http://codereview.chromium.org/3277008/diff/18001/17003#newcode598 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:598: do { // > Wait for the update to finish. > > On 2010/09/14 06:39:20, lzheng wrote: > >> On 2010/09/10 23:11:26, Paweł Hajdan Jr. wrote: >> > Such kind of loop is a bad method to wait. Determine an event or >> > condition you > >> > need to wait for, and wait for it. Do not spin the message loop >> > multiple times > >> > until things are happy. The rationale here is that there is a >> > correlation > >> > between waiting in a bad way and unreliable tests. >> You have valid concerns. However, we can't simply wait for a event >> > here. This > >> while loop allows us to pull safebrowsing server and update several >> > states. It > >> simulates the real life working flow to make sure our test is as valid >> > as > >> possible. To make a wait for event (such as condition variable), we >> > have to > >> change the safebrowsing server a lot to send notifications when each >> > of the > >> state changes. It will introduce more complications to the server >> > which are not > >> necessary. >> > > I don't think so. My experience so far is that it's less complicated > than people believe initially. Could you experiment with waiting for an > event, so that we can compare the code? As we chatted off line, we will leave this as what have here. > > > In this case, if the test failed due to timeout, it should be a valid >> > failure > >> since it means either the server is not responding or the update flow >> > is > >> stopped/jammed. >> > > Well, and the timeout is suspicious. We should not hardcode things like > kMaxWaitSec. > I removed it so we could rely on BrowserTest's timeout. > > http://codereview.chromium.org/3277008/diff/37001/24014#newcode190 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:190: } > No, no, no. You can't duplicate this code. For example, David Benjamin > has a CL in review that makes it much more reliable, and also faster. > Please find a way to share more code with net::TestServer. Sharing the > pythonpath-related code is good, but I'd like to see more. > > I updated the python_util CLs and made the WaitServerStart a static function. Please take a look so I can update this CL (I was thinking to create another file for this but feel there is not enough materials/functions could be added there. So, I settled for a static function for now). > > http://codereview.chromium.org/3277008/show >
codereview site is giving me issues, so getting this posted even though I'm not really done digging. http://codereview.chromium.org/3277008/diff/37001/24014 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/37001/24014#newcode49 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:49: const char kTestCompletePath[] = "/test_complete"; Push these into an anonymous namespace. http://codereview.chromium.org/3277008/diff/37001/24014#newcode90 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:90: } " // namespace" And a line of whitespace before "class". http://codereview.chromium.org/3277008/diff/37001/24014#newcode115 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:115: .Append(FILE_PATH_LITERAL("testing")); Might as well use AppendASCII(), IMHO. http://codereview.chromium.org/3277008/diff/37001/24014#newcode187 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:187: } Whitespace here - though I'd say if you aren't going to use Port() and Hostname(), maybe just omit them. http://codereview.chromium.org/3277008/diff/37001/24014#newcode203 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:203: } whitespace here. http://codereview.chromium.org/3277008/diff/37001/24014#newcode208 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:208: static const int kRetryAttempts = 3; private: constants before private: functions. http://codereview.chromium.org/3277008/diff/37001/24014#newcode224 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:224: std::vector<PhishingUrl>* phishing_urls) { Can you push this function (and the typedef) into SafeBrowsingServiceTest class? Or the anonymous namespace? http://codereview.chromium.org/3277008/diff/37001/24014#newcode232 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:232: continue; whitespace before variables. http://codereview.chromium.org/3277008/diff/37001/24014#newcode234 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:234: std::vector<std::string> url_parts; Maybe record_parts, as only one of them is an URL. http://codereview.chromium.org/3277008/diff/37001/24014#newcode236 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:236: if (url_parts.size() < 3) { Should this be !=? http://codereview.chromium.org/3277008/diff/37001/24014#newcode298 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:298: } Could the next line be in the else, and the RunMessageLoop() outside the if()? Maybe even is_check_url_in_db_ = !sbs_->CheckUrl() outside of if(). Style does encourage the chain-of-if()-statements style, but I think once you drop a return; in there you should break it. Just MHO, though. http://codereview.chromium.org/3277008/diff/37001/24014#newcode361 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:361: StringPrintf("http://%s:%s/safebrowsing", kHost, kPort)); Lift the shared StringPrintf() to a variable. Not for performance, just to make sure they really are using the same server. Though I think it would also be reasonable to lift it out to a constant next to kHost/kPort. I also notice that kURLVerifyPath and kDBVerifyPath are kind of similar. Maybe it would be even more reasonable to have a helper function to generate safe-browsing URLs. http://codereview.chromium.org/3277008/diff/37001/24014#newcode404 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:404: ASSERT_TRUE(ChromeThread::CurrentlyOn(ChromeThread::IO)); I am not entirely certain that ASSERT_TRUE() works as you might expect on other threads. Did you check into that? http://codereview.chromium.org/3277008/diff/37001/24014#newcode405 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:405: CHECK(safe_browsing_test_->is_checked_url_in_db()); Should not CHECK() in tests, I think. Might be a good time to fix the other occurrences, also. http://codereview.chromium.org/3277008/diff/37001/24014#newcode428 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:428: void OnForceUpdateDone() { I think it would be reasonable for these cases to call StopUILoop directly, rather than having this indirection. Unless you strongly expect to add changes. http://codereview.chromium.org/3277008/diff/37001/24014#newcode532 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:532: } Suggest abstracting out the shared code from those three methods. I am not entirely clear on why these are fetching from the test server, is it to stay in sync with the parallel SafeBrowsingService fetches? http://codereview.chromium.org/3277008/diff/37001/24014#newcode586 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:586: Whitespace above the comment, no space between comment and variable. I am somewhat befuddled by the comment, though, as it doesn't appear to limit the number of tests at all. http://codereview.chromium.org/3277008/diff/37001/24014#newcode623 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:623: LOG(INFO) << safe_browsing_helper->response_data(); I'm kind of thinking these LOG(INFO) lines might get annoying when someone is trying to debug a build break (and not a safe-browsing problem). http://codereview.chromium.org/3277008/diff/37001/24014#newcode628 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:628: EXPECT_GT(phishing_urls.size(), static_cast<size_t>(0)); Use 0U instead of the static_cast. Apply that elsewhere, too. http://codereview.chromium.org/3277008/diff/37001/24014#newcode639 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:639: << " test step: " << step; I like this!
Scott: I updated the code and your comments are addressed. Please take another look. Thanks! Lei http://codereview.chromium.org/3277008/diff/37001/24014 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/37001/24014#newcode49 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:49: const char kTestCompletePath[] = "/test_complete"; On 2010/09/15 01:09:17, shess wrote: > Push these into an anonymous namespace. Done. http://codereview.chromium.org/3277008/diff/37001/24014#newcode90 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:90: } On 2010/09/15 01:09:17, shess wrote: > " // namespace" > > And a line of whitespace before "class". Done. http://codereview.chromium.org/3277008/diff/37001/24014#newcode115 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:115: .Append(FILE_PATH_LITERAL("testing")); On 2010/09/15 01:09:17, shess wrote: > Might as well use AppendASCII(), IMHO. Yes, that works too. I will keep it as is if that is okay with you. http://codereview.chromium.org/3277008/diff/37001/24014#newcode187 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:187: } On 2010/09/15 01:09:17, shess wrote: > Whitespace here - though I'd say if you aren't going to use Port() and > Hostname(), maybe just omit them. Removed. http://codereview.chromium.org/3277008/diff/37001/24014#newcode203 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:203: } On 2010/09/15 01:09:17, shess wrote: > whitespace here. Done. http://codereview.chromium.org/3277008/diff/37001/24014#newcode208 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:208: static const int kRetryAttempts = 3; On 2010/09/15 01:09:17, shess wrote: > private: constants before private: functions. I removed those timeouts and use Pinger's default ping behavior. http://codereview.chromium.org/3277008/diff/37001/24014#newcode224 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:224: std::vector<PhishingUrl>* phishing_urls) { On 2010/09/15 01:09:17, shess wrote: > Can you push this function (and the typedef) into SafeBrowsingServiceTest class? > Or the anonymous namespace? I moved them to anonymous namespace. http://codereview.chromium.org/3277008/diff/37001/24014#newcode232 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:232: continue; On 2010/09/15 01:09:17, shess wrote: > whitespace before variables. Done. http://codereview.chromium.org/3277008/diff/37001/24014#newcode234 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:234: std::vector<std::string> url_parts; On 2010/09/15 01:09:17, shess wrote: > Maybe record_parts, as only one of them is an URL. Done. http://codereview.chromium.org/3277008/diff/37001/24014#newcode236 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:236: if (url_parts.size() < 3) { On 2010/09/15 01:09:17, shess wrote: > Should this be !=? Done. http://codereview.chromium.org/3277008/diff/37001/24014#newcode298 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:298: } On 2010/09/15 01:09:17, shess wrote: > Could the next line be in the else, and the RunMessageLoop() outside the if()? > Maybe even is_check_url_in_db_ = !sbs_->CheckUrl() outside of if(). > > Style does encourage the chain-of-if()-statements style, but I think once you > drop a return; in there you should break it. Just MHO, though. I changed a little bit. Hope it is more clear. http://codereview.chromium.org/3277008/diff/37001/24014#newcode361 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:361: StringPrintf("http://%s:%s/safebrowsing", kHost, kPort)); On 2010/09/15 01:09:17, shess wrote: > Lift the shared StringPrintf() to a variable. Not for performance, just to make > sure they really are using the same server. Though I think it would also be > reasonable to lift it out to a constant next to kHost/kPort. > > I also notice that kURLVerifyPath and kDBVerifyPath are kind of similar. Maybe > it would be even more reasonable to have a helper function to generate > safe-browsing URLs. For this test, the prefixes will just be a url points to local server. They will be the same unless test data changes. So, I moved the creation of this prefix at the beginning of this function. In production, the prefixes points to different servers (and they use default values). http://codereview.chromium.org/3277008/diff/37001/24014#newcode404 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:404: ASSERT_TRUE(ChromeThread::CurrentlyOn(ChromeThread::IO)); On 2010/09/15 01:09:17, shess wrote: > I am not entirely certain that ASSERT_TRUE() works as you might expect on other > threads. Did you check into that? Yes, it should be on IO thread based on my understanding. And the test shows this way too. http://codereview.chromium.org/3277008/diff/37001/24014#newcode405 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:405: CHECK(safe_browsing_test_->is_checked_url_in_db()); On 2010/09/15 01:09:17, shess wrote: > Should not CHECK() in tests, I think. Might be a good time to fix the other > occurrences, also. This was meant to be a programing CHECK, to indicate that the state here is what expected. I changed this to be EXPECT_TRUE though. There is several more CHECKs to check if a pointer is null as a programming/api guard. I think it is okay to keep them as they are now, unless you strongly disagree. http://codereview.chromium.org/3277008/diff/37001/24014#newcode428 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:428: void OnForceUpdateDone() { On 2010/09/15 01:09:17, shess wrote: > I think it would be reasonable for these cases to call StopUILoop directly, > rather than having this indirection. Unless you strongly expect to add changes. Pawel brought this too. The reason I created a a redirection is that when I want to comment when and where the program will continue after "ui_test_utils::RunMessageLoop()" is called, I found it is hard to just say "It will be continue at StopUILoop" and later you will find a lot of places call "StopUILoop" and hard to figure out which one is triggered by current async call. So, I would suggest to keep this indirections here although it seems to be not too useful in programming level. http://codereview.chromium.org/3277008/diff/37001/24014#newcode532 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:532: } On 2010/09/15 01:09:17, shess wrote: > Suggest abstracting out the shared code from those three methods. Good catch. Done. > > I am not entirely clear on why these are fetching from the test server, is it to > stay in sync with the parallel SafeBrowsingService fetches? The testserver expects the test to keep fetching data until it runs out of data so it will return nothing. The VerifyTestComplete step makes sure our test is not stopped in the middle while server still have data left. Also, if there is a bad url that server expects us to fetch full hash but we did not, this verification will fail too. I am updating the comment here to explain. http://codereview.chromium.org/3277008/diff/37001/24014#newcode586 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:586: On 2010/09/15 01:09:17, shess wrote: > Whitespace above the comment, no space between comment and variable. I am > somewhat befuddled by the comment, though, as it doesn't appear to limit the > number of tests at all. You are right. I noticed this too and moved the comment upwards. last_step was moved to the beginning of this function too. http://codereview.chromium.org/3277008/diff/37001/24014#newcode623 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:623: LOG(INFO) << safe_browsing_helper->response_data(); On 2010/09/15 01:09:17, shess wrote: > I'm kind of thinking these LOG(INFO) lines might get annoying when someone is > trying to debug a build break (and not a safe-browsing problem). I removed this log. I wish there is something like VLOG(x) though. http://codereview.chromium.org/3277008/diff/37001/24014#newcode628 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:628: EXPECT_GT(phishing_urls.size(), static_cast<size_t>(0)); On 2010/09/15 01:09:17, shess wrote: > Use 0U instead of the static_cast. Apply that elsewhere, too. Good idea. http://codereview.chromium.org/3277008/diff/41001/42002#newcode43 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:43: I'll get rid of one empty line here.
Starting to LGTM. Actually, looks like my comments are all completely advisory. So LGTM whether you decide to make changes according to my comments or not. http://codereview.chromium.org/3277008/diff/37001/24014 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/37001/24014#newcode115 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:115: .Append(FILE_PATH_LITERAL("testing")); On 2010/09/15 19:21:49, lzheng wrote: > On 2010/09/15 01:09:17, shess wrote: > > Might as well use AppendASCII(), IMHO. > Yes, that works too. I will keep it as is if that is okay with you. OK. Mainly suggested because you don't seem to need anything beyond ASCII, I really don't recall the suggested style for tests. http://codereview.chromium.org/3277008/diff/37001/24014#newcode404 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:404: ASSERT_TRUE(ChromeThread::CurrentlyOn(ChromeThread::IO)); On 2010/09/15 19:21:49, lzheng wrote: > On 2010/09/15 01:09:17, shess wrote: > > I am not entirely certain that ASSERT_TRUE() works as you might expect on > other > > threads. Did you check into that? > Yes, it should be on IO thread based on my understanding. And the test shows > this way too. Sorry, I wasn't wondering if this should be on the I/O thread, it should. I was wondering if ASSERT_TRUE() fails the containing test when run on a different thread from the test body. I don't know if that is the case, or not. A minimal test would be to make sure that changing it to ASSERT_FALSE() throws, and doesn't crash things or something, but probably even better to find someone to ask. http://codereview.chromium.org/3277008/diff/37001/24014#newcode405 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:405: CHECK(safe_browsing_test_->is_checked_url_in_db()); On 2010/09/15 19:21:49, lzheng wrote: > On 2010/09/15 01:09:17, shess wrote: > > Should not CHECK() in tests, I think. Might be a good time to fix the other > > occurrences, also. > This was meant to be a programing CHECK, to indicate that the state here is what > expected. I changed this to be EXPECT_TRUE though. > There is several more CHECKs to check if a pointer is null as a programming/api > guard. I think it is okay to keep them as they are now, unless you strongly > disagree. The problem with CHECK in tests is that it crashes the tests entirely. So if a bot is running a bunch of unit tests and your's fails, any tests after that don't run. It can't be entirely avoided, but it's generally good to not use CHECK() in tests if you can help it. http://codereview.chromium.org/3277008/diff/37001/24014#newcode428 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:428: void OnForceUpdateDone() { On 2010/09/15 19:21:49, lzheng wrote: > On 2010/09/15 01:09:17, shess wrote: > > I think it would be reasonable for these cases to call StopUILoop directly, > > rather than having this indirection. Unless you strongly expect to add > changes. > > Pawel brought this too. The reason I created a a redirection is that when I want > to comment when and where the program will continue after > "ui_test_utils::RunMessageLoop()" is called, I found it is hard to just say "It > will be continue at StopUILoop" and later you will find a lot of places call > "StopUILoop" and hard to figure out which one is triggered by current async > call. So, I would suggest to keep this indirections here although it seems to be > not too useful in programming level. OK, seems reasonable. http://codereview.chromium.org/3277008/diff/37001/24014#newcode623 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:623: LOG(INFO) << safe_browsing_helper->response_data(); On 2010/09/15 19:21:49, lzheng wrote: > On 2010/09/15 01:09:17, shess wrote: > > I'm kind of thinking these LOG(INFO) lines might get annoying when someone is > > trying to debug a build break (and not a safe-browsing problem). > I removed this log. I wish there is something like VLOG(x) though. Yeah, I'm just being grumpy. I hate being sheriff and having a build break, and I go look at the logs and it looks like thousands of things are going wrong. Which one is relevant? Who knows! I think that when people are writing/debugging code, they over-estimate the value of their log lines to future debugging sessions. http://codereview.chromium.org/3277008/diff/41001/42002#newcode161 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:161: return true; If you're going to log the failure for DIR_EXE below, might as well have consistent style up here. http://codereview.chromium.org/3277008/diff/41001/42002#newcode299 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:299: is_checked_url_in_db_ = true; no need for return;. http://codereview.chromium.org/3277008/diff/41001/42002#newcode581 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:581: EXPECT_TRUE(last_update().is_null()); Could pass FilePath(kDataFile). else, const on FilePath, so I don't look for where it's updated :-).
Scott: a. I switched CHECK to either ASSERT_XX or EXPECT_XXX. b. As you suspected, ASSERT_xxx only stops the current function. I modified some of them to EXPECT_xxx since it makes sense to let the callback continue in some cases. Thanks for bringing this up. c. I also changed several places following your comments. Pawel: I send out http://codereview.chromium.org/3383006/show to create a separate binary for this. I will merge this change with that once that cl is submitted. I also got rid of the pinger. Could you take another look? Thanks! http://codereview.chromium.org/3277008/diff/41001/42002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/41001/42002#newcode161 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:161: return false; On 2010/09/16 23:57:50, shess wrote: > If you're going to log the failure for DIR_EXE below, might as well have > consistent style up here. Done. http://codereview.chromium.org/3277008/diff/41001/42002#newcode299 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:299: return; On 2010/09/16 23:57:50, shess wrote: > no need for return;. Done. http://codereview.chromium.org/3277008/diff/41001/42002#newcode581 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:581: SafeBrowsingTestServer test_server(kHost, kPort, datafile_path); On 2010/09/16 23:57:50, shess wrote: > Could pass FilePath(kDataFile). else, const on FilePath, so I don't look for > where it's updated :-). Done.
still lgtm. On 2010/09/17 17:36:47, lzheng wrote: > Scott: > > a. I switched CHECK to either ASSERT_XX or EXPECT_XXX. > b. As you suspected, ASSERT_xxx only stops the current function. I modified some > of them to EXPECT_xxx since it makes sense to let the callback continue in some > cases. Thanks for bringing this up. > c. I also changed several places following your comments. > > > Pawel: > > I send out http://codereview.chromium.org/3383006/show to create a separate > binary for this. I will merge this change with that once that cl is submitted. I > also got rid of the pinger. Could you take another look? > > Thanks! > > http://codereview.chromium.org/3277008/diff/41001/42002 > File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): > > http://codereview.chromium.org/3277008/diff/41001/42002#newcode161 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:161: return false; > On 2010/09/16 23:57:50, shess wrote: > > If you're going to log the failure for DIR_EXE below, might as well have > > consistent style up here. > > Done. > > http://codereview.chromium.org/3277008/diff/41001/42002#newcode299 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:299: return; > On 2010/09/16 23:57:50, shess wrote: > > no need for return;. > > Done. > > http://codereview.chromium.org/3277008/diff/41001/42002#newcode581 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:581: > SafeBrowsingTestServer test_server(kHost, kPort, datafile_path); > On 2010/09/16 23:57:50, shess wrote: > > Could pass FilePath(kDataFile). else, const on FilePath, so I don't look for > > where it's updated :-). > > Done.
Sorry, I still have some comments. I'm not going to complain about the inner loop after you moved it to a separate test binary, but I am going to make sure all the other things are right. This is really close now by the way. http://codereview.chromium.org/3277008/diff/66001/67002 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/66001/67002#newcode46 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:46: const char kHost[] = "localhost"; Could you make those constants private to the test server class, and make the caller ask the test server for those? We're not going to make this as flexible as I want yet, but let's at least make the design more future-proof. http://codereview.chromium.org/3277008/diff/66001/67002#newcode425 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:425: int wait_time_msec = time_out_msec / 5; Just get rid of it, yuck. http://codereview.chromium.org/3277008/diff/66001/67002#newcode516 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:516: if (start_message_loop) { Let the caller start the loop instead of adding a bool parameter. http://codereview.chromium.org/3277008/diff/66001/67002#newcode535 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:535: const int kStatusCheckMsec = 50; Do not hardcode such things! If you need to, chrome/test/test_timeouts can be adjusted. http://codereview.chromium.org/3277008/diff/66001/67002#newcode536 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:536: // Limits max test steps so the test won't run wild. With the browser_tests launcher, you're protected against that. Is this still needed? And it seems that exceeding this number does not generate a test failure, just breaks out of the loop. http://codereview.chromium.org/3277008/diff/66001/67002#newcode553 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:553: kHost, kPort, TestTimeouts::command_execution_timeout_ms())); You rather want to use action_timeout_ms or action_max_timeout. Please note the comment near commend_execution_timeout btw: "Timeout to use for AutomationProxy. Do not use un other places." and a TODO that it's going to be removed... http://codereview.chromium.org/3277008/diff/66001/67004 File net/test/python_utils.cc (right): http://codereview.chromium.org/3277008/diff/66001/67004#newcode43 net/test/python_utils.cc:43: *dir = dir->Append(FILE_PATH_LITERAL("third_party")) This is accidental, right?
I think I addressed all the comments. Another look? The net/test/python_utils.cc change was intended, since the "dir" was set in PathService::Get, we can not over-write it. I could put this change in another CL if you prefer. Also, since safe_browsing_browsertest.cc was renamed to safe_browsing_test.cc in another cl when I try to create a separate test for safebrowsing, the diff between safe_browsing_test and safe_browsing_browsertest could not show the changes I made between patches. To make your review easier, I still uploaded a copy of safe_browser_browsertest.cc. I will need to remove it before I commit though. Lei On Mon, Sep 20, 2010 at 3:16 AM, <phajdan.jr@chromium.org> wrote: > Sorry, I still have some comments. I'm not going to complain about the > inner > loop after you moved it to a separate test binary, but I am going to make > sure > all the other things are right. This is really close now by the way. > > > http://codereview.chromium.org/3277008/diff/66001/67002 > > File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): > > http://codereview.chromium.org/3277008/diff/66001/67002#newcode46 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:46: const char > kHost[] = "localhost"; > Could you make those constants private to the test server class, and > make the caller ask the test server for those? We're not going to make > this as flexible as I want yet, but let's at least make the design more > future-proof. > > http://codereview.chromium.org/3277008/diff/66001/67002#newcode425 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:425: int > wait_time_msec = time_out_msec / 5; > Just get rid of it, yuck. > > http://codereview.chromium.org/3277008/diff/66001/67002#newcode516 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:516: if > (start_message_loop) { > Let the caller start the loop instead of adding a bool parameter. > > http://codereview.chromium.org/3277008/diff/66001/67002#newcode535 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:535: const int > kStatusCheckMsec = 50; > Do not hardcode such things! If you need to, chrome/test/test_timeouts > can be adjusted. > > http://codereview.chromium.org/3277008/diff/66001/67002#newcode536 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:536: // Limits > max test steps so the test won't run wild. > With the browser_tests launcher, you're protected against that. Is this > still needed? And it seems that exceeding this number does not generate > a test failure, just breaks out of the loop. > > http://codereview.chromium.org/3277008/diff/66001/67002#newcode553 > chrome/browser/safe_browsing/safe_browsing_browsertest.cc:553: kHost, > kPort, TestTimeouts::command_execution_timeout_ms())); > You rather want to use action_timeout_ms or action_max_timeout. > > Please note the comment near commend_execution_timeout btw: "Timeout to > use for AutomationProxy. Do not use un other places." and a TODO that > it's going to be removed... > > http://codereview.chromium.org/3277008/diff/66001/67004 > File net/test/python_utils.cc (right): > > http://codereview.chromium.org/3277008/diff/66001/67004#newcode43 > net/test/python_utils.cc:43: *dir = > dir->Append(FILE_PATH_LITERAL("third_party")) > This is accidental, right? > > http://codereview.chromium.org/3277008/show >
By "get rid of it" I have really meant it. :) It's also less code to maintain for you. http://codereview.chromium.org/3277008/diff/89001/87003 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/89001/87003#newcode191 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:191: const char SafeBrowsingTestServer::k_host_[] = "localhost"; nit: Why not kHost as the style guide says? http://codereview.chromium.org/3277008/diff/89001/87003#newcode441 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:441: int max_retries = TestTimeouts::action_max_timeout_ms() / time_out_ms; This uses browser_tests protection against infinite loops, please get rid of this code. http://codereview.chromium.org/3277008/diff/89001/87003#newcode444 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:444: ChromeThread::PostDelayedTask( It's weird that we post a delayed task and spin up the message loop. Why don't we post directly to the loop? You should rely on the test launcher, and not invent your own timeout code. http://codereview.chromium.org/3277008/diff/89001/87003#newcode575 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:575: // Periodically pull the status nit: dot at the end of comment. http://codereview.chromium.org/3277008/diff/89001/87005 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3277008/diff/89001/87005#newcode1635 chrome/chrome_tests.gypi:1635: '../third_party/protobuf2/protobuf.gyp:py_proto', Why this started to be needed for browser_tests? It's not obvious to me why this change is being made. http://codereview.chromium.org/3277008/diff/89001/87006 File net/test/python_utils.cc (right): http://codereview.chromium.org/3277008/diff/89001/87006#newcode43 net/test/python_utils.cc:43: *dir = dir->Append(FILE_PATH_LITERAL("third_party")) Okay, we want to append to DIR_SOURCE_ROOT. That's fine.
I will find a time to chat with you sometime today, so we don't need to exchange emails back and force. http://codereview.chromium.org/3277008/diff/89001/87003 File chrome/browser/safe_browsing/safe_browsing_browsertest.cc (right): http://codereview.chromium.org/3277008/diff/89001/87003#newcode191 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:191: const char SafeBrowsingTestServer::k_host_[] = "localhost"; On 2010/09/21 08:42:07, Paweł Hajdan Jr. wrote: > nit: Why not kHost as the style guide says? I know we usually use kHost, but that is mostly for something out of the class. I could not find the guide on this, do you have any pointer? I changed it to k_host_ just to distinguish with the regular static char case where it is defined out side. http://codereview.chromium.org/3277008/diff/89001/87003#newcode441 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:441: int max_retries = TestTimeouts::action_max_timeout_ms() / time_out_ms; On 2010/09/21 08:42:07, Paweł Hajdan Jr. wrote: > This uses browser_tests protection against infinite loops, please get rid of > this code. I don't feel very comfortable to totally rely on the browser_tests time out to guard this. It will take longer for the test to fail on any error case. The loops here could fail the test early when something is obviously wrong. http://codereview.chromium.org/3277008/diff/89001/87003#newcode444 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:444: ChromeThread::PostDelayedTask( On 2010/09/21 08:42:07, Paweł Hajdan Jr. wrote: > It's weird that we post a delayed task and spin up the message loop. Why don't > we post directly to the loop? You should rely on the test launcher, and not > invent your own timeout code. Not sure if I understand what you meant. This gives time for the server to load the data file and get into ready mode. We can't keep pulling from the server. I added a comment to explain this. http://codereview.chromium.org/3277008/diff/89001/87003#newcode575 chrome/browser/safe_browsing/safe_browsing_browsertest.cc:575: // Periodically pull the status On 2010/09/21 08:42:07, Paweł Hajdan Jr. wrote: > nit: dot at the end of comment. Done. http://codereview.chromium.org/3277008/diff/89001/87005 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3277008/diff/89001/87005#newcode1635 chrome/chrome_tests.gypi:1635: '../third_party/protobuf2/protobuf.gyp:py_proto', On 2010/09/21 08:42:07, Paweł Hajdan Jr. wrote: > Why this started to be needed for browser_tests? It's not obvious to me why this > change is being made. Since I moved the test to safe_browsing_test, this should be dropped after the integration. Done. http://codereview.chromium.org/3277008/diff/89001/87005#newcode1945 chrome/chrome_tests.gypi:1945: 'browser', Ignore these too. There should be no change in this file.
http://codereview.chromium.org/3277008/diff/71002/59006 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/71002/59006#newcode441 chrome/browser/safe_browsing/safe_browsing_test.cc:441: // TODO(lzheng): We should have a way to reliablely tell when a server is I will correct this typo "reliably".
Code I commented in the drive-by lg with some comments. http://codereview.chromium.org/3277008/diff/71002/59006 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/71002/59006#newcode185 chrome/browser/safe_browsing/safe_browsing_test.cc:185: static const char k_host_[]; nit: Please make it kHost, we do a similar thing in other cases. I have never seen k_hacker_style_ being used. http://codereview.chromium.org/3277008/diff/71002/59006#newcode210 chrome/browser/safe_browsing/safe_browsing_test.cc:210: ASSERT_FALSE(!safe_browsing_service_); nit: ASSERT_TRUE(safe_browsing_service_)? Double-negation is just weird. http://codereview.chromium.org/3277008/diff/71002/59006#newcode220 chrome/browser/safe_browsing/safe_browsing_test.cc:220: ASSERT_FALSE(!safe_browsing_service_); Same here. And in all other cases. http://codereview.chromium.org/3277008/diff/71002/59006#newcode446 chrome/browser/safe_browsing/safe_browsing_test.cc:446: Sleep(TestTimeouts::action_timeout_ms()); Fetch, then Sleep.
http://codereview.chromium.org/3277008/diff/71002/59006 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/71002/59006#newcode185 chrome/browser/safe_browsing/safe_browsing_test.cc:185: static const char k_host_[]; On 2010/09/21 17:35:07, Paweł Hajdan Jr. wrote: > nit: Please make it kHost, we do a similar thing in other cases. I have never > seen k_hacker_style_ being used. Done. http://codereview.chromium.org/3277008/diff/71002/59006#newcode210 chrome/browser/safe_browsing/safe_browsing_test.cc:210: ASSERT_FALSE(!safe_browsing_service_); On 2010/09/21 17:35:07, Paweł Hajdan Jr. wrote: > nit: ASSERT_TRUE(safe_browsing_service_)? > > Double-negation is just weird. Done. http://codereview.chromium.org/3277008/diff/71002/59006#newcode220 chrome/browser/safe_browsing/safe_browsing_test.cc:220: ASSERT_FALSE(!safe_browsing_service_); On 2010/09/21 17:35:07, Paweł Hajdan Jr. wrote: > Same here. And in all other cases. Done. http://codereview.chromium.org/3277008/diff/71002/59006#newcode446 chrome/browser/safe_browsing/safe_browsing_test.cc:446: Sleep(TestTimeouts::action_timeout_ms()); On 2010/09/21 17:35:07, Paweł Hajdan Jr. wrote: > Fetch, then Sleep. I moved the first fetch inside the loop. Hopefully, this makes the flow more straight-forward.
http://codereview.chromium.org/3277008/diff/104001/100008 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/104001/100008#newcode5 chrome/browser/safe_browsing/safe_browsing_test.cc:5: // This test uses safebrowsing test server published at "the safebrowsing test server" http://codereview.chromium.org/3277008/diff/104001/100008#newcode6 chrome/browser/safe_browsing/safe_browsing_test.cc:6: // http://code.google.com/p/google-safe-browsing/ to test safebrowsing protocol "test the safebrowsing protocol" http://codereview.chromium.org/3277008/diff/104001/100008#newcode7 chrome/browser/safe_browsing/safe_browsing_test.cc:7: // implemetation. Details of safebrowsing testing flow is docummented "of the safebrowsing" "documented" http://codereview.chromium.org/3277008/diff/104001/100008#newcode10 chrome/browser/safe_browsing/safe_browsing_test.cc:10: // In a short summary, this test launches safebrowsing test server and nit: Remove "In a short summary". http://codereview.chromium.org/3277008/diff/104001/100008#newcode13 chrome/browser/safe_browsing/safe_browsing_test.cc:13: // urls from test server to verify its repository. The test will succeed only urls --> URLs http://codereview.chromium.org/3277008/diff/104001/100008#newcode14 chrome/browser/safe_browsing/safe_browsing_test.cc:14: // if all updates are performed and urls are match what the server expected. urls --> URLs "are match what" --> "match what" http://codereview.chromium.org/3277008/diff/104001/100008#newcode52 chrome/browser/safe_browsing/safe_browsing_test.cc:52: typedef struct { Why don't typically use this style in chrome. please just do: struct PhishingUrl { ... }; http://codereview.chromium.org/3277008/diff/104001/100008#newcode63 chrome/browser/safe_browsing/safe_browsing_test.cc:63: static bool ParsePhishingUrls(const std::string& data, No real need for this to be static, as it is already in anonymous namespace. http://codereview.chromium.org/3277008/diff/104001/100008#newcode109 chrome/browser/safe_browsing/safe_browsing_test.cc:109: EXPECT_EQ(server_handle_, base::kNullProcessHandle); nit: reverse so the expectation is first. http://codereview.chromium.org/3277008/diff/104001/100008#newcode147 chrome/browser/safe_browsing/safe_browsing_test.cc:147: LOG(ERROR) << "Failed to launch server" << cmd_line.command_line_string(); nit: add a space at the end of "server". http://codereview.chromium.org/3277008/diff/104001/100008#newcode180 chrome/browser/safe_browsing/safe_browsing_test.cc:180: static const char* Port() { why not return an int? http://codereview.chromium.org/3277008/diff/104001/100008#newcode220 chrome/browser/safe_browsing/safe_browsing_test.cc:220: ASSERT_TRUE(safe_browsing_service_); Isn't this function called from the IO thread? I don't believe gtest is thread-safe, so ASSERT_* and EXPECT_* are only valid to call on the test runner thread. Not sure, worth investigating. http://codereview.chromium.org/3277008/diff/104001/100008#newcode438 chrome/browser/safe_browsing/safe_browsing_test.cc:438: // ready so we could get rid of the Sleep and retry loop. This is how it is done for the chrome test server: http://codereview.chromium.org/3368012/diff/27001
Eric: Thanks for the review. Your comments are addressed. Please see comments inline. http://codereview.chromium.org/3277008/diff/104001/100008 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/104001/100008#newcode5 chrome/browser/safe_browsing/safe_browsing_test.cc:5: // This test uses safebrowsing test server published at On 2010/09/29 01:32:11, eroman wrote: > "the safebrowsing test server" Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode6 chrome/browser/safe_browsing/safe_browsing_test.cc:6: // http://code.google.com/p/google-safe-browsing/ to test safebrowsing protocol On 2010/09/29 01:32:11, eroman wrote: > "test the safebrowsing protocol" Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode7 chrome/browser/safe_browsing/safe_browsing_test.cc:7: // implemetation. Details of safebrowsing testing flow is docummented On 2010/09/29 01:32:11, eroman wrote: > "of the safebrowsing" > > "documented" Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode10 chrome/browser/safe_browsing/safe_browsing_test.cc:10: // In a short summary, this test launches safebrowsing test server and On 2010/09/29 01:32:11, eroman wrote: > nit: Remove "In a short summary". Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode13 chrome/browser/safe_browsing/safe_browsing_test.cc:13: // urls from test server to verify its repository. The test will succeed only On 2010/09/29 01:32:11, eroman wrote: > urls --> URLs Done. I hope I got all url/urls in the comments too. http://codereview.chromium.org/3277008/diff/104001/100008#newcode14 chrome/browser/safe_browsing/safe_browsing_test.cc:14: // if all updates are performed and urls are match what the server expected. On 2010/09/29 01:32:11, eroman wrote: > urls --> URLs > > "are match what" --> "match what" Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode52 chrome/browser/safe_browsing/safe_browsing_test.cc:52: typedef struct { On 2010/09/29 01:32:11, eroman wrote: > Why don't typically use this style in chrome. > please just do: > > struct PhishingUrl { > ... > }; Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode63 chrome/browser/safe_browsing/safe_browsing_test.cc:63: static bool ParsePhishingUrls(const std::string& data, On 2010/09/29 01:32:11, eroman wrote: > No real need for this to be static, as it is already in anonymous namespace. true. http://codereview.chromium.org/3277008/diff/104001/100008#newcode109 chrome/browser/safe_browsing/safe_browsing_test.cc:109: EXPECT_EQ(server_handle_, base::kNullProcessHandle); On 2010/09/29 01:32:11, eroman wrote: > nit: reverse so the expectation is first. Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode147 chrome/browser/safe_browsing/safe_browsing_test.cc:147: LOG(ERROR) << "Failed to launch server" << cmd_line.command_line_string(); On 2010/09/29 01:32:11, eroman wrote: > nit: add a space at the end of "server". Done. http://codereview.chromium.org/3277008/diff/104001/100008#newcode220 chrome/browser/safe_browsing/safe_browsing_test.cc:220: ASSERT_TRUE(safe_browsing_service_); On 2010/09/29 01:32:11, eroman wrote: > Isn't this function called from the IO thread? > > I don't believe gtest is thread-safe, so ASSERT_* and EXPECT_* are only valid to > call on the test runner thread. Not sure, worth investigating. I don't see anywhere ASSERT_* and EXPECT_* mentioned threadsafe and I don't see that from the code too. There are "threadsafe" discussions on death test EXPECT_DEATH and ASSERT_DEATH though: https://wiki.corp.google.com/twiki/bin/view/Main/GUnitAdvanced http://codereview.chromium.org/3277008/diff/104001/100008#newcode438 chrome/browser/safe_browsing/safe_browsing_test.cc:438: // ready so we could get rid of the Sleep and retry loop. On 2010/09/29 01:32:11, eroman wrote: > This is how it is done for the chrome test server: > > http://codereview.chromium.org/3368012/diff/27001 They are not exactly the same. The test server does this using a blocking action, here I am using a sleep/retry loop. I personally don't think it is too bad to use a sleep retry loop here, but what we have in test server could be more decisive and efficient.
http://codereview.chromium.org/3277008/diff/104001/100008 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/104001/100008#newcode220 chrome/browser/safe_browsing/safe_browsing_test.cc:220: ASSERT_TRUE(safe_browsing_service_); On 2010/09/29 18:31:06, lzheng wrote: > On 2010/09/29 01:32:11, eroman wrote: > > Isn't this function called from the IO thread? > > > > I don't believe gtest is thread-safe, so ASSERT_* and EXPECT_* are only valid > to > > call on the test runner thread. Not sure, worth investigating. > I don't see anywhere ASSERT_* and EXPECT_* mentioned threadsafe and I don't see > that from the code too. There are "threadsafe" discussions on death test > EXPECT_DEATH and ASSERT_DEATH though: > https://wiki.corp.google.com/twiki/bin/view/Main/GUnitAdvanced > You are right, it looks like gtest is in fact thread safe. I guess I was thinking of _gmock_, since we have had data races in the past (i.e. http://code.google.com/p/chromium/issues/detail?id=52457)
lgtm. please run it a number of times locally to make sure it runs non-flakily. I am primarily concerned about the spin loops. http://codereview.chromium.org/3277008/diff/134001/135002 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/134001/135002#newcode67 chrome/browser/safe_browsing/safe_browsing_test.cc:67: std::vector<PhishingUrl>* phishing_urls) { nit: please line this up with the earlier parameter (or move each to their own line). http://codereview.chromium.org/3277008/diff/134001/135002#newcode158 chrome/browser/safe_browsing/safe_browsing_test.cc:158: if (!server_handle_) { nit: I wander if you should test against base::kNullProcessHandle here for consistency (since that is what you initialize it to). http://codereview.chromium.org/3277008/diff/134001/135002#newcode192 chrome/browser/safe_browsing/safe_browsing_test.cc:192: }; Can you add DISALLOW_COPY_AND_ASSIGN ? http://codereview.chromium.org/3277008/diff/134001/135002#newcode240 chrome/browser/safe_browsing/safe_browsing_test.cc:240: // In this case, Safebrowsing service will fetch full hash nit: "fetch full hash" --> "fetch the full hash". http://codereview.chromium.org/3277008/diff/134001/135002#newcode241 chrome/browser/safe_browsing/safe_browsing_test.cc:241: // from the server and exam that. Once it is done, nit: exam --> examine. http://codereview.chromium.org/3277008/diff/134001/135002#newcode436 chrome/browser/safe_browsing/safe_browsing_test.cc:436: void ServerReady(const char* host, int port) { Can you rename this to like WaitUntilServerReady(), or InitServer() ? http://codereview.chromium.org/3277008/diff/134001/135002#newcode442 chrome/browser/safe_browsing/safe_browsing_test.cc:442: while (true) { Do you have a sense of how long this runs ordinarily? http://codereview.chromium.org/3277008/diff/134001/135002#newcode444 chrome/browser/safe_browsing/safe_browsing_test.cc:444: if (response_status_ == URLRequestStatus::SUCCESS) how about having FetchUrl return the status/data ? http://codereview.chromium.org/3277008/diff/134001/135002#newcode537 chrome/browser/safe_browsing/safe_browsing_test.cc:537: safe_browsing_helper->ServerReady(SafeBrowsingTestServer::Host(), nit: prefer lining up arguments (i.e. move this to net line, or indent the following). http://codereview.chromium.org/3277008/diff/134001/135002#newcode554 chrome/browser/safe_browsing/safe_browsing_test.cc:554: for (int step = 1; step < 3; step++) { nit: This is going to be tricky to debug when there are failures. How about adding like: SCOPED_TRACE(StringPrintf("step=%d", step)); So if there are errors you can see what step it was on? http://codereview.chromium.org/3277008/diff/134001/135002#newcode570 chrome/browser/safe_browsing/safe_browsing_test.cc:570: } while (is_update_scheduled() || is_initial_request() || this is a tight loop. Do you not want any sleeps in here? http://codereview.chromium.org/3277008/diff/134001/135002#newcode594 chrome/browser/safe_browsing/safe_browsing_test.cc:594: EXPECT_TRUE(is_checked_url_in_db()) << phishing_urls[j].url please move the << XXX to next line so the params line up. http://codereview.chromium.org/3277008/diff/134001/135002#newcode595 chrome/browser/safe_browsing/safe_browsing_test.cc:595: << " is_phishing: " << phishing_urls[j].is_phishing nit: index by 4 http://codereview.chromium.org/3277008/diff/134001/135002#newcode597 chrome/browser/safe_browsing/safe_browsing_test.cc:597: EXPECT_FALSE(is_checked_url_safe()) << phishing_urls[j].url please move the << to next line so it lines up with other params. http://codereview.chromium.org/3277008/diff/134001/135002#newcode601 chrome/browser/safe_browsing/safe_browsing_test.cc:601: EXPECT_TRUE(is_checked_url_safe()) << phishing_urls[j].url ditto. http://codereview.chromium.org/3277008/diff/134001/135002#newcode623 chrome/browser/safe_browsing/safe_browsing_test.cc:623: // EXPECT_EQ("yes", safe_browsing_helper->response_data()); Be sure to add a TODO about this...
Eric: All comments addressed. I was meant to wait for the bot timeout change to happen before I submit this change, however, since it will take several days, I added a "#if defined" in the code to limit how much the test can run on windows. I will remove that after the bot is restarted. Please feel free take another quick look. I will submit it once the bots are green. http://codereview.chromium.org/3277008/diff/134001/135002 File chrome/browser/safe_browsing/safe_browsing_test.cc (right): http://codereview.chromium.org/3277008/diff/134001/135002#newcode67 chrome/browser/safe_browsing/safe_browsing_test.cc:67: std::vector<PhishingUrl>* phishing_urls) { On 2010/10/08 22:28:07, eroman wrote: > nit: please line this up with the earlier parameter (or move each to their own > line). Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode158 chrome/browser/safe_browsing/safe_browsing_test.cc:158: if (!server_handle_) { On 2010/10/08 22:28:07, eroman wrote: > nit: I wander if you should test against base::kNullProcessHandle here for > consistency (since that is what you initialize it to). Good point. http://codereview.chromium.org/3277008/diff/134001/135002#newcode192 chrome/browser/safe_browsing/safe_browsing_test.cc:192: }; On 2010/10/08 22:28:07, eroman wrote: > Can you add DISALLOW_COPY_AND_ASSIGN ? Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode240 chrome/browser/safe_browsing/safe_browsing_test.cc:240: // In this case, Safebrowsing service will fetch full hash On 2010/10/08 22:28:07, eroman wrote: > nit: "fetch full hash" --> "fetch the full hash". Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode241 chrome/browser/safe_browsing/safe_browsing_test.cc:241: // from the server and exam that. Once it is done, On 2010/10/08 22:28:07, eroman wrote: > nit: exam --> examine. Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode436 chrome/browser/safe_browsing/safe_browsing_test.cc:436: void ServerReady(const char* host, int port) { On 2010/10/08 22:28:07, eroman wrote: > Can you rename this to like WaitUntilServerReady(), or InitServer() ? Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode442 chrome/browser/safe_browsing/safe_browsing_test.cc:442: while (true) { On 2010/10/08 22:28:07, eroman wrote: > Do you have a sense of how long this runs ordinarily? According to my tests, it almost never need to go to that sleep. Even needed, it just uses the Sleep once. http://codereview.chromium.org/3277008/diff/134001/135002#newcode537 chrome/browser/safe_browsing/safe_browsing_test.cc:537: safe_browsing_helper->ServerReady(SafeBrowsingTestServer::Host(), On 2010/10/08 22:28:07, eroman wrote: > nit: prefer lining up arguments (i.e. move this to net line, or indent the > following). Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode554 chrome/browser/safe_browsing/safe_browsing_test.cc:554: for (int step = 1; step < 3; step++) { Sorry, what you saw meant for debug. The step < 3 should not be there. On 2010/10/08 22:28:07, eroman wrote: > nit: This is going to be tricky to debug when there are failures. > > How about adding like: > > SCOPED_TRACE(StringPrintf("step=%d", step)); > > So if there are errors you can see what step it was on? I removed he step < 3 since when the server runs out of data, the loop will break from the "if (last_update() < now)". The < 3 was there just for testing purse. The SCOPED_TRACE was a good idea. I am using it. http://codereview.chromium.org/3277008/diff/134001/135002#newcode570 chrome/browser/safe_browsing/safe_browsing_test.cc:570: } while (is_update_scheduled() || is_initial_request() || On 2010/10/08 22:28:07, eroman wrote: > this is a tight loop. Do you not want any sleeps in here? CheckStatusAfterDelay actually blocks in message loop for about TestTimeouts::action_timeout_ms till call back happens. I renamed the function to WaitForStatusUpdate() to be more descriptive. http://codereview.chromium.org/3277008/diff/134001/135002#newcode594 chrome/browser/safe_browsing/safe_browsing_test.cc:594: EXPECT_TRUE(is_checked_url_in_db()) << phishing_urls[j].url On 2010/10/08 22:28:07, eroman wrote: > please move the << XXX to next line so the params line up. Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode595 chrome/browser/safe_browsing/safe_browsing_test.cc:595: << " is_phishing: " << phishing_urls[j].is_phishing On 2010/10/08 22:28:07, eroman wrote: > nit: index by 4 Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode597 chrome/browser/safe_browsing/safe_browsing_test.cc:597: EXPECT_FALSE(is_checked_url_safe()) << phishing_urls[j].url On 2010/10/08 22:28:07, eroman wrote: > please move the << to next line so it lines up with other params. Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode601 chrome/browser/safe_browsing/safe_browsing_test.cc:601: EXPECT_TRUE(is_checked_url_safe()) << phishing_urls[j].url On 2010/10/08 22:28:07, eroman wrote: > ditto. Done. http://codereview.chromium.org/3277008/diff/134001/135002#newcode623 chrome/browser/safe_browsing/safe_browsing_test.cc:623: // EXPECT_EQ("yes", safe_browsing_helper->response_data()); On 2010/10/08 22:28:07, eroman wrote: > Be sure to add a TODO about this... Done.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
