|
|
Created:
9 years, 3 months ago by lipalani1 Modified:
9 years, 3 months ago CC:
chromium-reviews, ncarter (slow), idana, Raghu Simha, cbentzel+watch_chromium.org, pam+watch_chromium.org, darin-cc_chromium.org, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntegration test cases for the server directed error handling feature.
BUG=97671
TEST=sync_integration_tests.exe
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102426
Patch Set 1 #Patch Set 2 : For review. #
Total comments: 40
Patch Set 3 : For review. #
Total comments: 23
Patch Set 4 : For review. #Patch Set 5 : for try bots. #Patch Set 6 : For trybots. #
Total comments: 2
Patch Set 7 : Upload before commit. #Patch Set 8 : Upload before commit. #Patch Set 9 : Upload before commit. #
Messages
Total messages: 14 (0 generated)
Please review. Tim - CCed for fyi.
Thanks for adding these tests. I have a bunch of comments, some of which are "while you're at it" comments that should help clean up minor nits left behind from previous CLs. (Hope you don't mind.) Also, please update the description of this patch and populate the BUG= and TEST= fields. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:366: if (retry_verifier_.done()) While you're at it: Add a comment here that briefly explains why we are signaling that the wait is complete. See states above for an example. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:373: SignalStateCompleteWithNextState(WAITING_FOR_MIGRATION_TO_FINISH); While you're at it: Add a comment here that briefly explains why we are signaling that the wait is complete. See states above for an example. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:379: if (!HasPendingBackendMigration()) { While you're at it: Add a comment here that briefly explains why we are signaling that the wait is complete. See states above for an example. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:388: service_->unrecoverable_error_detected() == true) { Add a comment here that briefly explains why we are signaling that the wait is complete. See states above for an example. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:596: DCHECK(status.sync_protocol_error.action == browser_sync::UNKNOWN_ACTION); DCHECKs won't get executed on the main waterfall, because they run in release mode. If this is important, consider using CHECK. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:603: browser_sync::SyncBackendHost::Status::OFFLINE_UNUSABLE); Curious: This success conditions here seem to be different from the ones on which you are waiting in RunStateChangeMachine. Should you also be waiting for this condition in RunStateChangeMachine? Or change this to unrecoverable_error_detected() == true? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:83: // Blocks the caller until the sync has been disabled for this client. Returns While you're at it: s/the sync/sync/ http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:90: // Blocks the caller until the syncer receives a actionable error. nit: s/a actionable/an actionable/ http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:90: // Blocks the caller until the syncer receives a actionable error. Add "Returns true when so-and-so happens" to this comment. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:226: // The sync client is waiting for an actionable error from server. nit: s/from server/from the server/ http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_errors_test.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_errors_test.cc:106: Remove extra blank line. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:614: namespace { Style guide seems to recommend a blank line here. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:616: SyncProtocolErrorTypeToClientToServerResponseErrorType( Holy long name, Batman! How about GetClientToServerResponseErrorType()? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:640: ClientActionToClientToServerResponseAction( indent += 1. In this case, indent -= 3 will also work. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:640: ClientActionToClientToServerResponseAction( How about renaming this to GetServerResponseAction()? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:659: } Style guide seems to recommend a blank line here. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:680: ASSERT_TRUE(output.find("SetError: 200") != string16::npos); Isn't output an std::string? Shouldn't you be comparing the result of find() with std::string::npos? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:680: ASSERT_TRUE(output.find("SetError: 200") != string16::npos); From chromiumsync.py, isn't the error code 400? Or am I reading it wrong? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.h (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.h:20: #include "chrome/browser/sync/protocol/sync_protocol_error.h" Alphabetize includes.
Please review. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:366: if (retry_verifier_.done()) On 2011/09/16 04:01:02, rsimha wrote: > While you're at it: Add a comment here that briefly explains why we are > signaling that the wait is complete. See states above for an example. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:373: SignalStateCompleteWithNextState(WAITING_FOR_MIGRATION_TO_FINISH); On 2011/09/16 04:01:02, rsimha wrote: > While you're at it: Add a comment here that briefly explains why we are > signaling that the wait is complete. See states above for an example. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:379: if (!HasPendingBackendMigration()) { On 2011/09/16 04:01:02, rsimha wrote: > While you're at it: Add a comment here that briefly explains why we are > signaling that the wait is complete. See states above for an example. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:388: service_->unrecoverable_error_detected() == true) { On 2011/09/16 04:01:02, rsimha wrote: > Add a comment here that briefly explains why we are signaling that the wait is > complete. See states above for an example. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:596: DCHECK(status.sync_protocol_error.action == browser_sync::UNKNOWN_ACTION); On 2011/09/16 04:01:02, rsimha wrote: > DCHECKs won't get executed on the main waterfall, because they run in release > mode. If this is important, consider using CHECK. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:603: browser_sync::SyncBackendHost::Status::OFFLINE_UNUSABLE); Checking for unrecoverable error is the right thing. thanks for catching this. On 2011/09/16 04:01:02, rsimha wrote: > Curious: This success conditions here seem to be different from the ones on > which you are waiting in RunStateChangeMachine. Should you also be waiting for > this condition in RunStateChangeMachine? Or change this to > unrecoverable_error_detected() == true? Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:83: // Blocks the caller until the sync has been disabled for this client. Returns On 2011/09/16 04:01:02, rsimha wrote: > While you're at it: s/the sync/sync/ Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:90: // Blocks the caller until the syncer receives a actionable error. On 2011/09/16 04:01:02, rsimha wrote: > nit: s/a actionable/an actionable/ Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:90: // Blocks the caller until the syncer receives a actionable error. On 2011/09/16 04:01:02, rsimha wrote: > Add "Returns true when so-and-so happens" to this comment. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:226: // The sync client is waiting for an actionable error from server. On 2011/09/16 04:01:02, rsimha wrote: > nit: s/from server/from the server/ Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_errors_test.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_errors_test.cc:106: On 2011/09/16 04:01:02, rsimha wrote: > Remove extra blank line. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:614: namespace { On 2011/09/16 04:01:02, rsimha wrote: > Style guide seems to recommend a blank line here. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:616: SyncProtocolErrorTypeToClientToServerResponseErrorType( On 2011/09/16 04:01:02, rsimha wrote: > Holy long name, Batman! How about GetClientToServerResponseErrorType()? Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:640: ClientActionToClientToServerResponseAction( On 2011/09/16 04:01:02, rsimha wrote: > indent += 1. In this case, indent -= 3 will also work. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:640: ClientActionToClientToServerResponseAction( On 2011/09/16 04:01:02, rsimha wrote: > How about renaming this to GetServerResponseAction()? Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:659: } On 2011/09/16 04:01:02, rsimha wrote: > Style guide seems to recommend a blank line here. Done. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:680: ASSERT_TRUE(output.find("SetError: 200") != string16::npos); No the output is size_t. So we need to compare against npos. On 2011/09/16 04:01:02, rsimha wrote: > Isn't output an std::string? Shouldn't you be comparing the result of find() > with std::string::npos? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:680: ASSERT_TRUE(output.find("SetError: 200") != string16::npos); 200 is for success. I thing the .py file says the same... On 2011/09/16 04:01:02, rsimha wrote: > From chromiumsync.py, isn't the error code 400? Or am I reading it wrong? http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.h (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.h:20: #include "chrome/browser/sync/protocol/sync_protocol_error.h" On 2011/09/16 04:01:02, rsimha wrote: > Alphabetize includes. Done.
A few more fixes and this should be good to go. Also, I'm not sure if you saw my earlier comment re: updating the description of this patch and populating the BUG= and TEST= fields. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.cc (right): http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:680: ASSERT_TRUE(output.find("SetError: 200") != string16::npos); On 2011/09/19 18:59:13, lipalani1 wrote: > No the output is size_t. So we need to compare against npos. Yeah, what I meant was that for consistency, we should be using std::string::npos and not string16::npos, since |output| is an std::string and not a string16 variable. http://codereview.chromium.org/7919001/diff/2001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:680: ASSERT_TRUE(output.find("SetError: 200") != string16::npos); On 2011/09/19 18:59:13, lipalani1 wrote: > 200 is for success. I thing the .py file says the same... You're right. 400 is returned only if chromiumsync.py hits a python exception. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.cc (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:366: if (retry_verifier_.done()) This if block needs to be within braces. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:374: // No pending migrations. Now on to waiting for migrations. How does HasPendingBackendMigration() == true indicate that there are no pending migrations? http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:70: // Blocks the caller until sync backend host associated with this harness Undo this change. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:91: // return true if sync client had received an actionable error. s/And return true if sync client had received/Returns true if the sync client received/ http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:97: // Blocks the caller until this harness has observed that sync engine Undo this change. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:132: // Returns the ProfileSyncService member of sync client. s/of sync client/of the sync client/ http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:135: // Returns the status of the ProfileSyncService member of sync client. s/of sync client/of the sync client/ http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:157: // Encrypt the datatype |type|. This method will block while sync backend Undo this change. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.cc (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:618: browser_sync::SyncProtocolErrorType error) { Indent +=1. http://codereview.chromium.org/7919001/diff/9001/net/tools/testserver/chromiu... File net/tools/testserver/chromiumsync.py (right): http://codereview.chromium.org/7919001/diff/9001/net/tools/testserver/chromiu... net/tools/testserver/chromiumsync.py:1078: ### This is deprecated now. Would be remvoved once test cases are removed. I think the Python style guide recommends a single-#-plus-space for comments.
Please review. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.cc (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:366: if (retry_verifier_.done()) On 2011/09/19 22:25:02, rsimha wrote: > This if block needs to be within braces. Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.cc:374: // No pending migrations. Now on to waiting for migrations. On 2011/09/19 22:25:02, rsimha wrote: > How does HasPendingBackendMigration() == true indicate that there are no pending > migrations? Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:70: // Blocks the caller until sync backend host associated with this harness On 2011/09/19 22:25:02, rsimha wrote: > Undo this change. Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:91: // return true if sync client had received an actionable error. On 2011/09/19 22:25:02, rsimha wrote: > s/And return true if sync client had received/Returns true if the sync client > received/ Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:97: // Blocks the caller until this harness has observed that sync engine On 2011/09/19 22:25:02, rsimha wrote: > Undo this change. Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:132: // Returns the ProfileSyncService member of sync client. On 2011/09/19 22:25:02, rsimha wrote: > s/of sync client/of the sync client/ Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:135: // Returns the status of the ProfileSyncService member of sync client. On 2011/09/19 22:25:02, rsimha wrote: > s/of sync client/of the sync client/ Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:157: // Encrypt the datatype |type|. This method will block while sync backend On 2011/09/19 22:25:02, rsimha wrote: > Undo this change. Done. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/test/int... File chrome/browser/sync/test/integration/sync_test.cc (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/test/int... chrome/browser/sync/test/integration/sync_test.cc:618: browser_sync::SyncProtocolErrorType error) { On 2011/09/19 22:25:02, rsimha wrote: > Indent +=1. Done. http://codereview.chromium.org/7919001/diff/9001/net/tools/testserver/chromiu... File net/tools/testserver/chromiumsync.py (right): http://codereview.chromium.org/7919001/diff/9001/net/tools/testserver/chromiu... net/tools/testserver/chromiumsync.py:1078: ### This is deprecated now. Would be remvoved once test cases are removed. Incorrectly ended up in this cl. removed. On 2011/09/19 22:25:02, rsimha wrote: > I think the Python style guide recommends a single-#-plus-space for comments.
Please review.
A couple more fixes remaining, after which this is good to send to the trybots. LGTM after that. Thanks for patiently going through all these iterations :) http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:91: // return true if sync client had received an actionable error. On 2011/09/22 20:42:46, lipalani1 wrote: > Done. This isn't done yet. s/And returns true if sync client received/Returns true if the sync client received/ http://codereview.chromium.org/7919001/diff/9001/net/tools/testserver/chromiu... File net/tools/testserver/chromiumsync.py (right): http://codereview.chromium.org/7919001/diff/9001/net/tools/testserver/chromiu... net/tools/testserver/chromiumsync.py:1078: ### This is deprecated now. Would be remvoved once test cases are removed. On 2011/09/22 20:42:46, lipalani1 wrote: > Incorrectly ended up in this cl. removed. My bad -- looks like this came in from a different CL that went in between your patch sets. It'd still be good to fix the comment, though.
Running try bots. Thanks for the LGTM. the remaining comment is fixed. Misunderstood the comment. http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/9001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service_harness.h:91: // return true if sync client had received an actionable error. On 2011/09/22 21:15:10, rsimha wrote: > On 2011/09/22 20:42:46, lipalani1 wrote: > > Done. > > This isn't done yet. > s/And returns true if sync client received/Returns true if the sync client > received/ Done.
http://codereview.chromium.org/7919001/diff/18004/chrome/browser/sync/profile... File chrome/browser/sync/profile_sync_service_harness.h (right): http://codereview.chromium.org/7919001/diff/18004/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service_harness.h:91: // Returns true if the ync client received an actionable error. Not done yet! s/ync/sync/ http://codereview.chromium.org/7919001/diff/18004/net/tools/testserver/chromi... File net/tools/testserver/chromiumsync.py (right): http://codereview.chromium.org/7919001/diff/18004/net/tools/testserver/chromi... net/tools/testserver/chromiumsync.py:1085: ### This is deprecated now. Would be remvoved once test cases are removed. This comment still doesn't follow the style guide.
fixed the mistake in the sync spelling..
you can review now. the python server file is not here.
LGTM pending green trybots.
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/lipalani@chromium.org/7919001/19019
Change committed as 102426 |