Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716413003:
Initial clear server data impl (Closed)
DescriptionInitial clear server data impl.
When we run integration tests, we need accounts with a clean slate.
This CL clears out the data on sync servers for the given test account,
and restarts sync. This operation doesn't require read/writing any data
to the account, and thus passphrase decryption is by-passed (Some tests
have the side-effect of setting a decryption passphrase that requires
clear server data to be run without decryption).
R=pavely@chromium.org,skym@chromium.org
TEST=run this command
ninja -C out/Debug sync_integration_tests -j 200 && \
xvfb-run --server-args="-screen 0 1920x1080x24" \
out/Debug/sync_integration_tests \
--gtest_also_run_disabled_tests \
--gtest_filter=*E2ETest* \
--sync-url=https://clients4.google.com/chrome-sync/dev \
--sync-user-for-test=testuser@gmail.com\
--sync-password-for-test=testpassword
--vmodule=*sync*=1
BUG=
Review-Url: https://codereview.chromium.org/2716413003
Cr-Commit-Position: refs/heads/master@{#456199}
Committed: https://chromium.googlesource.com/chromium/src/+/0b596d6acab4b78d658879a7584c6743cf63d7cf
Patch Set 1 #Patch Set 2 : Clear server data command #
Total comments: 40
Patch Set 3 : Responding to CL comments. Some Logic changes, but mostly spelling/comments. #
Total comments: 50
Patch Set 4 : Fixed various CL comments #
Total comments: 16
Patch Set 5 : CL changes #
Total comments: 11
Patch Set 6 : Responding to Pavel's comments. #
Total comments: 10
Patch Set 7 : Adding confirmationuiclosed to prevent timeouts, addressing some naming concerns, and removing logg… #
Total comments: 5
Patch Set 8 : Addressing newlines, and unused arguments #
Messages
Total messages: 37 (14 generated)
Description was changed from ========== Initial clear server data impl BUG= ========== to ========== Initial clear server data impl Running this from my machine: 41 pass 26 tests timeout 5 tests fail BUG=b/35406961 ==========
wylieb@chromium.org changed reviewers: + pavely@chromium.org, sky@chromium.org
PTAL at this CL!
wylieb@chromium.org changed reviewers: + skym@chromium.org - sky@chromium.org
Changed 'sky@chromium' to 'skym@chromium', sorry Scott!
b/35406961 is Google internal bug, not accessible for external committers. You can either not specify bug at all or specify bug number from crbug.com. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:235: bool ProfileSyncServiceHarness::SetupSyncForClear( This function is awfully similar to SetupSync(). Imagine someone else decides to change one of them, will they remember to change the other as well? Is it possible to refactor functions to reduce code duplication? https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.h:104: // Same as above, but without checks for passphrase decryption. Could you explain in CL description/comment why code shouldn't check for passphrase decryption? Also put some info in description about what problem you are solving and how you are doing it. https://codereview.chromium.org/2716413003/diff/20001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/20001/components/browser_sync... components/browser_sync/profile_sync_service.cc:24: #include "base/run_loop.h" You shouldn't need RunLoop in this file. https://codereview.chromium.org/2716413003/diff/20001/components/browser_sync... components/browser_sync/profile_sync_service.cc:1390: Doe extra line make function more readable? Coding style suggests not to add unnecessary vertical space, but it is often question of taste. https://codereview.chromium.org/2716413003/diff/20001/components/browser_sync... components/browser_sync/profile_sync_service.cc:2431: void ProfileSyncService::ClearServerDataForTest(const base::Closure& callback) { I would put this function after RefreshTypesForTest to match order of declaration in .h file.
Description was changed from ========== Initial clear server data impl Running this from my machine: 41 pass 26 tests timeout 5 tests fail BUG=b/35406961 ========== to ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). BUG= ==========
Sorry for all the obnoxious nits. Also, CL description doesn't have line breaks. Someone pointed me to use https://chrome.google.com/webstore/detail/reflow-cl-description/dilabllmnakbk... which makes it easy to conform to best practices for CL summary line lengths. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:235: bool ProfileSyncServiceHarness::SetupSyncForClear( On 2017/03/07 07:26:15, pavely wrote: > This function is awfully similar to SetupSync(). Imagine someone else decides to > change one of them, will they remember to change the other as well? Is it > possible to refactor functions to reduce code duplication? +1 https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:303: std::unique_ptr<syncer::SyncSetupInProgressHandle> blocker = Is there a reason that |sync_blocker_| isn't used? https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:305: So https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:307: many https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:309: new https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:311: lines https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:313: if (!AwaitEngineInitialization()) { We should also always require a passphrase, right? Should we verify as such? https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:398: bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() { Do you understand why this method even exists? As far as I can tell, it's AwaitEngineInitialization() without the engine check. Looking at all of its call sites, it seems like replacing it with AwaitEngineInitialization might even just work... Would love it if we could avoid the other two flavors of this function being added in this CL as well. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.h:63: // Use this method when you need to setup a client that you're going to cal cal should be call https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.h:112: // Saem as above, but without checks for passphrase decryption. Saem should be Same https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. Can you add more context here, why is this useful? https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:355: LOG(WARNING) << "PROFILE PATH " << profile_path.value(); Should this be DVLOG? Did you mean to remove this? Also, the ALL CAPS is kind of odd if you want to keep this logging as DVLOG. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:624: DVLOG(1) << "Done Setting up first client for clear"; Setting should be setting Also, end with punctuation to be consistent? https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:638: for (int i = 0; i < num_clients_; ++i) { This loop and logic seems extremely similar to just a few lines above. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1168: // start config, at this point out bday is good Sorry for all the obnoxious nits, but, https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... Also, out should be our, I think. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:373: // Setup and clear server data Comment should end with a . to be consistent with other comments. https://codereview.chromium.org/2716413003/diff/20001/components/browser_sync... File components/browser_sync/profile_sync_service.h (right): https://codereview.chromium.org/2716413003/diff/20001/components/browser_sync... components/browser_sync/profile_sync_service.h:573: // for end to end testing End with punctuation.
https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:398: bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() { On 2017/03/07 18:38:22, skym wrote: > Do you understand why this method even exists? As far as I can tell, it's > AwaitEngineInitialization() without the engine check. Looking at all of its call > sites, it seems like replacing it with AwaitEngineInitialization might even just > work... > > Would love it if we could avoid the other two flavors of this function being > added in this CL as well. Oooh, I missed that the first check was significantly different. Hmm, never mind I guess.
Addressed CL concerns. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:235: bool ProfileSyncServiceHarness::SetupSyncForClear( On 2017/03/07 07:26:15, pavely wrote: > This function is awfully similar to SetupSync(). Imagine someone else decides to > change one of them, will they remember to change the other as well? Is it > possible to refactor functions to reduce code duplication? Well that depends. If they want to change the the behavior of setting up sync for an account to clear the server data, then yes I would expect them to. But if someone decides to change SetupSync without understanding the context from which it's called, then I'd say it's prone to what you're pointing out here. I generally favor _some_ code duplication over a function doing more than one thing. You have a good point, though. LMK about the change I made here: I passed a bool forClear, and keyed off of that for tasks that were problematic within the function. It satisfies the your concern, as the only duplicate method that there is now is one that calls into SetupSync (ie another parameter-less function like the one that exists for SetupSync already. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:303: std::unique_ptr<syncer::SyncSetupInProgressHandle> blocker = On 2017/03/07 18:38:22, skym wrote: > Is there a reason that |sync_blocker_| isn't used? Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:305: On 2017/03/07 18:38:22, skym wrote: > So Ok https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:307: On 2017/03/07 18:38:22, skym wrote: > many I https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:309: On 2017/03/07 18:38:22, skym wrote: > new Fixed https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:311: On 2017/03/07 18:38:22, skym wrote: > lines It https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:313: if (!AwaitEngineInitialization()) { On 2017/03/07 18:38:22, skym wrote: > We should also always require a passphrase, right? Should we verify as such? Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:398: bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() { On 2017/03/07 18:47:44, skym wrote: > On 2017/03/07 18:38:22, skym wrote: > > Do you understand why this method even exists? As far as I can tell, it's > > AwaitEngineInitialization() without the engine check. Looking at all of its > call > > sites, it seems like replacing it with AwaitEngineInitialization might even > just > > work... > > > > Would love it if we could avoid the other two flavors of this function being > > added in this CL as well. > > Oooh, I missed that the first check was significantly different. Hmm, never mind > I guess. Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:398: bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() { On 2017/03/07 18:38:22, skym wrote: > Do you understand why this method even exists? As far as I can tell, it's > AwaitEngineInitialization() without the engine check. Looking at all of its call > sites, it seems like replacing it with AwaitEngineInitialization might even just > work... > > Would love it if we could avoid the other two flavors of this function being > added in this CL as well. Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.h:63: // Use this method when you need to setup a client that you're going to cal On 2017/03/07 18:38:22, skym wrote: > cal should be call Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.h:104: // Same as above, but without checks for passphrase decryption. On 2017/03/07 07:26:16, pavely wrote: > Could you explain in CL description/comment why code shouldn't check for > passphrase decryption? Also put some info in description about what problem you > are solving and how you are doing it. Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.h:112: // Saem as above, but without checks for passphrase decryption. On 2017/03/07 18:38:22, skym wrote: > Saem should be Same Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. On 2017/03/07 18:38:22, skym wrote: > Can you add more context here, why is this useful? Consecutive runs fail because of an assertion on a static variable here. Added context to it. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:355: LOG(WARNING) << "PROFILE PATH " << profile_path.value(); On 2017/03/07 18:38:22, skym wrote: > Should this be DVLOG? Did you mean to remove this? Also, the ALL CAPS is kind of > odd if you want to keep this logging as DVLOG. Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:624: DVLOG(1) << "Done Setting up first client for clear"; On 2017/03/07 18:38:22, skym wrote: > Setting should be setting > > Also, end with punctuation to be consistent? Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1168: // start config, at this point out bday is good On 2017/03/07 18:38:22, skym wrote: > Sorry for all the obnoxious nits, but, > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... > > Also, out should be our, I think. Done. https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2716413003/diff/20001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:373: // Setup and clear server data On 2017/03/07 18:38:23, skym wrote: > Comment should end with a . to be consistent with other comments. Done.
https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:134: if (result == false) { Ahh, you were copying from here. Can you fix this while you're in here? :) https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:146: if (result == false) { The result == false is a bit verbose. What do you think of if (!result) { /*failure*/ } else { /*success*/ } or even better if (result) { /*success*/ } else { /*failure*/ } https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:147: std::string status = GetServiceStatus(); What do you think of in-lining this? https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:196: LOG(ERROR) << "AwaitEngineInitializationForClear failed."; These logs are kind of redundant, the await call already logs messages, right? I'm okay if you think they're adding value and you want to keep them, I just want us to think about it. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:202: Why do you need to go any farther than waiting for the engine to init to perform a server clear? I could believe a service()->RequestStart(); is required as well. But do you really need things like AwaitSyncSetupCompletionForClear()? https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:211: if ((signin_type_ == SigninType::UI_SIGNIN) && What kind of sign in are E2E tests by the way? FAKE_SIGNIN? https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:224: LOG(WARNING) << "Not checking passphrase decryption"; Punctuation! https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:246: sync_blocker_ = service()->GetSetupInProgressHandle(); Sorry if it feels like I'm flip-flopping here, but I don't think you should use the member blocker. I didn't understand what was going on here previously, and now that I looked into it, I'm not convinced it's actually useful in the general case. I'm running try bots against https://codereview.chromium.org/2735253002 right now which tries to remove it. If you do still need it for this restart case, then perhaps it should be scoped to this function. This is nicer anyways because then we don't have to rely on FinishSyncSetup() releasing this blocker which just seems really fragile. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:257: // Check passphrase now, should be implicit This comment should end with a period as well. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:267: Do you need to call AwaitSyncSetupCompletion() ? https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. When running tests consecutively, the tests Interesting... So are you calling these tests differently now? https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:617: if (UsingExternalServers()) { Would be good to explain why you clear for external servers. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:618: DVLOG(1) << "Setting up first client for clear."; Would be good to explain why we need to setup before calling ClearServerData. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:619: if (!GetClient(clientIndex++)->SetupSyncForClear()) { This is clunky how you're using clientIndex++ here, and then hard coding 0 in the ClearServerData(...) call. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:625: ClearServerData(GetClient(0)); Probably need to give this function a return value and check it as well. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:629: for (int i = clientIndex; i < num_clients_; ++i) { Could reuse clientIndex instead of making a new variable for (; clientIndex < num_clients_; ++clientIndex) { https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:631: I'd remove this newline to keep it consistent with the above lines. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1160: // Start configuration, at this point our birthday is out of date. Errm, is that really true? Shouldn't our birthday only be out of date after we call clear server data and the callback is invoked below? https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1166: harness->RestartSyncService(); It doesn't look like the success of this method is being verified. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:373: // Setup profile, clear server data, and restart sync. It doesn't seem like the implementation actually sets up the profile, contrary to this comment. https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... components/browser_sync/profile_sync_service.cc:2433: engine_->StartConfiguration(); I was always confused about this, why do you call StartConfiguration there again? Can you add a comment explaining the reasoning here? https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... File components/browser_sync/profile_sync_service.h (right): https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... components/browser_sync/profile_sync_service.h:573: // for end to end testing Punctuation.
https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:134: if (result == false) { On 2017/03/07 21:32:24, skym wrote: > Ahh, you were copying from here. Can you fix this while you're in here? :) Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:146: if (result == false) { On 2017/03/07 21:32:24, skym wrote: > The result == false is a bit verbose. What do you think of > > if (!result) { /*failure*/ } else { /*success*/ } > > or even better > > if (result) { /*success*/ } else { /*failure*/ } Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:147: std::string status = GetServiceStatus(); On 2017/03/07 21:32:24, skym wrote: > What do you think of in-lining this? Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:196: LOG(ERROR) << "AwaitEngineInitializationForClear failed."; On 2017/03/07 21:32:25, skym wrote: > These logs are kind of redundant, the await call already logs messages, right? > I'm okay if you think they're adding value and you want to keep them, I just > want us to think about it. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:202: On 2017/03/07 21:32:24, skym wrote: > Why do you need to go any farther than waiting for the engine to init to perform > a server clear? I could believe a service()->RequestStart(); is required as > well. But do you really need things like AwaitSyncSetupCompletionForClear()? Good insight, thanks! Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:211: if ((signin_type_ == SigninType::UI_SIGNIN) && On 2017/03/07 21:32:24, skym wrote: > What kind of sign in are E2E tests by the way? FAKE_SIGNIN? UI_SIGNIN https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:224: LOG(WARNING) << "Not checking passphrase decryption"; On 2017/03/07 21:32:25, skym wrote: > Punctuation! Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:246: sync_blocker_ = service()->GetSetupInProgressHandle(); On 2017/03/07 21:32:25, skym wrote: > Sorry if it feels like I'm flip-flopping here, but I don't think you should use > the member blocker. I didn't understand what was going on here previously, and > now that I looked into it, I'm not convinced it's actually useful in the general > case. I'm running try bots against https://codereview.chromium.org/2735253002 > right now which tries to remove it. > > If you do still need it for this restart case, then perhaps it should be scoped > to this function. This is nicer anyways because then we don't have to rely on > FinishSyncSetup() releasing this blocker which just seems really fragile. Yeah that's why I had it that way to begin with. Could have some leaks here if something calls SetupSync() while this happens. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:257: // Check passphrase now, should be implicit On 2017/03/07 21:32:24, skym wrote: > This comment should end with a period as well. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:267: On 2017/03/07 21:32:24, skym wrote: > Do you need to call AwaitSyncSetupCompletion() ? Good point. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. When running tests consecutively, the tests On 2017/03/07 21:32:25, skym wrote: > Interesting... So are you calling these tests differently now? No, I think that this was introduced and nobody thought about the case where we run tests locally. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:617: if (UsingExternalServers()) { On 2017/03/07 21:32:25, skym wrote: > Would be good to explain why you clear for external servers. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:618: DVLOG(1) << "Setting up first client for clear."; On 2017/03/07 21:32:25, skym wrote: > Would be good to explain why we need to setup before calling ClearServerData. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:619: if (!GetClient(clientIndex++)->SetupSyncForClear()) { On 2017/03/07 21:32:25, skym wrote: > This is clunky how you're using clientIndex++ here, and then hard coding 0 in > the ClearServerData(...) call. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:625: ClearServerData(GetClient(0)); On 2017/03/07 21:32:25, skym wrote: > Probably need to give this function a return value and check it as well. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:629: for (int i = clientIndex; i < num_clients_; ++i) { On 2017/03/07 21:32:25, skym wrote: > Could reuse clientIndex instead of making a new variable > > for (; clientIndex < num_clients_; ++clientIndex) { Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:631: On 2017/03/07 21:32:25, skym wrote: > I'd remove this newline to keep it consistent with the above lines. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1160: // Start configuration, at this point our birthday is out of date. On 2017/03/07 21:32:25, skym wrote: > Errm, is that really true? Shouldn't our birthday only be out of date after we > call clear server data and the callback is invoked below? Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1166: harness->RestartSyncService(); On 2017/03/07 21:32:25, skym wrote: > It doesn't look like the success of this method is being verified. Done. https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:373: // Setup profile, clear server data, and restart sync. On 2017/03/07 21:32:25, skym wrote: > It doesn't seem like the implementation actually sets up the profile, contrary > to this comment. Done. https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... components/browser_sync/profile_sync_service.cc:2433: engine_->StartConfiguration(); This is the only place I call StartConfiguration. I do it because the engine needs to be in configuration mode in order to call ClearServerData! https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... File components/browser_sync/profile_sync_service.h (right): https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... components/browser_sync/profile_sync_service.h:573: // for end to end testing On 2017/03/07 21:32:25, skym wrote: > Punctuation. Done.
Sorry for all the churn, things are looking almost all good! https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. When running tests consecutively, the tests On 2017/03/07 23:04:40, wylieb wrote: > On 2017/03/07 21:32:25, skym wrote: > > Interesting... So are you calling these tests differently now? > > No, I think that this was introduced and nobody thought about the case where we > run tests locally. I don't understand. If I run a vanilla > out/Default/sync_integration_tests It runs multiple tests and doesn't hit the ASSERTs in AssociateWithTest(). What exactly is the case no one thought about? https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/40001/components/browser_sync... components/browser_sync/profile_sync_service.cc:2433: engine_->StartConfiguration(); On 2017/03/07 23:04:40, wylieb wrote: > This is the only place I call StartConfiguration. I do it because the engine > needs to be in configuration mode in order to call ClearServerData! Heh, sorry, I said "again" because I've asked you this question before, but don't remember the reason. Digging into the code, it looks like the SyncScheduler has a concept of modes, and requires, as you were indicating, a CONFIGURATION_MODE to be set to transition to CLEAR_SERVER_DATA_MODE. https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_schedul... https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_schedul... https://codereview.chromium.org/1251203002/ I still don't quite follow the reason for this restriction. Perhaps it's because the only place we currently call this is after a catch up configuration sync cycle https://cs.chromium.org/chromium/src/components/browser_sync/profile_sync_ser... . Pavel, do you have better understanding for this restriction? Regardless, Brandon, can you slap in a comment here explaining this? Your logic doesn't care about "configuration", whatever that means in this context. But the engine/scheduler seem to have this restriction and calling engine_->StartConfiguration() essentially hacks around this confusing restriction. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:156: bool forClear) { should be named for_clear. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:216: // decryption or setup completion. I'd just drop the "passphrase description or", I'm not sure that's what I'd categorize the passphrase logic below this as. It seems like it's setting the passphrase. It bails if it needs to passphrase to decrypt anything. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:241: auto blocker = service()->GetSetupInProgressHandle(); I think auto is bad here. You're obscuring if this is a smart or raw pointer. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:252: // This passphrase should be implicit because ClearServerData should be called Erm, doesn't implicit mean that it is the GAIA password being reused as a passphrase? https://cs.chromium.org/chromium/src/components/sync/driver/sync_service.h?l=78 Rather, it is IsUsingSecondaryPassphrase() that should be false because ClearServerData() was just called, right? Although I don't get why the integ tests need to do this themselves, I thought there was logic to automatically upgrade clients to an implict passphrase itself.. /shrug https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:262: blocker.reset(); You shouldn't need this, it'll be destructed when it leaves scope for you, right? https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:263: service()->SetFirstSetupComplete(); I'm confused why you need both the blocker and this call. You've already called this method from FinishSyncSetup(), from SetupSync(), right? It isn't clear to me if that has been cleared out or not as part of this restarting of sync. It seems like once you know which it does, you can remove one of these two. Wait, actually, I don't see any reason to block at all, regardless of above thinking. It seems like in the old SyncSetup, the blocking was so that we could set the enabled data types. But they're already set before you get into this function, right? https://codereview.chromium.org/2716413003/diff/60001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/60001/components/browser_sync... components/browser_sync/profile_sync_service.cc:24: #include "base/run_loop.h" This include still needs to be removed.
https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. When running tests consecutively, the tests On 2017/03/08 00:23:48, skym wrote: > On 2017/03/07 23:04:40, wylieb wrote: > > On 2017/03/07 21:32:25, skym wrote: > > > Interesting... So are you calling these tests differently now? > > > > No, I think that this was introduced and nobody thought about the case where > we > > run tests locally. > > I don't understand. If I run a vanilla > > > out/Default/sync_integration_tests > > It runs multiple tests and doesn't hit the ASSERTs in AssociateWithTest(). What > exactly is the case no one thought about? I think I was mistaken, I removed it. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:156: bool forClear) { On 2017/03/08 00:23:48, skym wrote: > should be named for_clear. > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:216: // decryption or setup completion. On 2017/03/08 00:23:48, skym wrote: > I'd just drop the "passphrase description or", I'm not sure that's what I'd > categorize the passphrase logic below this as. It seems like it's setting the > passphrase. It bails if it needs to passphrase to decrypt anything. Done. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:241: auto blocker = service()->GetSetupInProgressHandle(); On 2017/03/08 00:23:48, skym wrote: > I think auto is bad here. You're obscuring if this is a smart or raw pointer. Done. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:252: // This passphrase should be implicit because ClearServerData should be called On 2017/03/08 00:23:48, skym wrote: > Erm, doesn't implicit mean that it is the GAIA password being reused as a > passphrase? > https://cs.chromium.org/chromium/src/components/sync/driver/sync_service.h?l=78 > > Rather, it is IsUsingSecondaryPassphrase() that should be false because > ClearServerData() was just called, right? > > Although I don't get why the integ tests need to do this themselves, I thought > there was logic to automatically upgrade clients to an implict passphrase > itself.. /shrug Done. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:262: blocker.reset(); On 2017/03/08 00:23:48, skym wrote: > You shouldn't need this, it'll be destructed when it leaves scope for you, > right? Done. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:263: service()->SetFirstSetupComplete(); On 2017/03/08 00:23:48, skym wrote: > I'm confused why you need both the blocker and this call. > > You've already called this method from FinishSyncSetup(), from SetupSync(), > right? It isn't clear to me if that has been cleared out or not as part of this > restarting of sync. It seems like once you know which it does, you can remove > one of these two. > > Wait, actually, I don't see any reason to block at all, regardless of above > thinking. It seems like in the old SyncSetup, the blocking was so that we could > set the enabled data types. But they're already set before you get into this > function, right? Seems like something to be addressed in another CL. A change that addresses the general question of when and why should we use a blocker, and is SetFirstSetupComplete enough? https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_datatype_helper.h (right): https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_datatype_helper.h:18: // Dissociates the test from us. When running tests consecutively, the tests I think I was mistaken on this one! Removing https://codereview.chromium.org/2716413003/diff/60001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/60001/components/browser_sync... components/browser_sync/profile_sync_service.cc:24: #include "base/run_loop.h" On 2017/03/08 00:23:48, skym wrote: > This include still needs to be removed. Done.
lgtm https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:267: On 2017/03/07 23:04:40, wylieb wrote: > On 2017/03/07 21:32:24, skym wrote: > > Do you need to call AwaitSyncSetupCompletion() ? > > Good point. > > Done. This never happened as far as I can tell. https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:263: service()->SetFirstSetupComplete(); On 2017/03/08 00:58:51, wylieb wrote: > On 2017/03/08 00:23:48, skym wrote: > > I'm confused why you need both the blocker and this call. > > > > You've already called this method from FinishSyncSetup(), from SetupSync(), > > right? It isn't clear to me if that has been cleared out or not as part of > this > > restarting of sync. It seems like once you know which it does, you can remove > > one of these two. > > > > Wait, actually, I don't see any reason to block at all, regardless of above > > thinking. It seems like in the old SyncSetup, the blocking was so that we > could > > set the enabled data types. But they're already set before you get into this > > function, right? > > Seems like something to be addressed in another CL. A change that addresses the > general question of when and why should we use a blocker, and is > SetFirstSetupComplete enough? Okay... I'm okay with you having both the SetFirstSetupComplete and blocker in here for now. But you need to 1. Understand why they're here and what they're adding 2. Document that reason with comments
Could you format description according to https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... Otherwise "git branch -vv" (and potentially other tools) looks weird. https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:143: bool ProfileSyncServiceHarness::SetupSyncForClear() { This function is only used in sync_test when setting up tests. It only contains call to SetupSync with the right parameters and logging on results. Does it make sense to move this code to sync_test? https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:156: bool for_clear) { "for_clear" name is not very clear. It denotes scenario where this function is used, not the purpose of it. You could name this parameter "bool perform_passphrase_verification" or "bool skip_passphrase_verification". https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:320: bool ProfileSyncServiceHarness::AwaitEngineInitializationForClear() { Could you add "bool ensure_valid_passphrase" parameter to AwaitEngineInitialization() and eliminate this copy of the function completely. What if in the future someone needed a flavor that doesn't check for AuthError? https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:614: int clientIndex = 0; SyncTest::SetupSync contains sequence of calls to other helper functions with small amount of code around each call (SetupClients, GetClient()->SetupSync. Does it make sense to extract this block of code into separate helper function to maintain readability of SetupSync? https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:1169: // Our birthday is bad here so restart sync to get the new birthday from the "Our birthday is bad" => "Our birthday is invalidated on the server" https://codereview.chromium.org/2716413003/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/80001/components/browser_sync... components/browser_sync/profile_sync_service.cc:2430: void ProfileSyncService::ClearServerDataForTest(const base::Closure& callback) { Why is this function placed at the end of file?
Description was changed from ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). BUG= ========== to ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST= run ninja -C out/Debug sync_integration_tests -j 200 &&\ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= ==========
Description was changed from ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST= run ninja -C out/Debug sync_integration_tests -j 200 &&\ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= ========== to ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST= run ninja -C out/Debug sync_integration_tests -j 200 && \ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= ==========
Description was changed from ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST= run ninja -C out/Debug sync_integration_tests -j 200 && \ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= ========== to ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST=run this command ninja -C out/Debug sync_integration_tests -j 200 && \ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= ==========
Good comments all around. I fixed the description of the CL, so hopefully it's acceptable in the console/git commands. I chose not to factor out the SetupSyncForClear function in profile_sync_service_harness because it servers the same purpose as the SetupSync function with no arguments. Doesn't make sense to me to move one to SyncTest. Keep an eye out for that comment, and let me know what you think! https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:267: On 2017/03/08 19:15:02, skym wrote: > On 2017/03/07 23:04:40, wylieb wrote: > > On 2017/03/07 21:32:24, skym wrote: > > > Do you need to call AwaitSyncSetupCompletion() ? > > > > Good point. > > > > Done. > > This never happened as far as I can tell. Sorry. I tried to implement this, but it times out when waiting. The condition that fails is IsSyncActive(). Not sure what's going on there -- got stuck debugging. The tests pass without this AwaitSyncSetup() call. Doing this wait in the SetupSyncForClear() method seems to be enough. Thoughts? https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:143: bool ProfileSyncServiceHarness::SetupSyncForClear() { On 2017/03/08 21:15:14, pavely wrote: > This function is only used in sync_test when setting up tests. It only contains > call to SetupSync with the right parameters and logging on results. Does it make > sense to move this code to sync_test? This is a helper to call into SetupSync with the correct arguments. ProfileSyncServiceHarness::SetupSync() serves the same purpose. I think this function makes much more sense in this file. https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:156: bool for_clear) { On 2017/03/08 21:15:13, pavely wrote: > "for_clear" name is not very clear. It denotes scenario where this function is > used, not the purpose of it. You could name this parameter "bool > perform_passphrase_verification" or "bool skip_passphrase_verification". It makes sense to rename this variable to skip_passphrase_verification here. For consistency, I also changed the name of the bool to AwaitEngineInitialization to the same. https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:320: bool ProfileSyncServiceHarness::AwaitEngineInitializationForClear() { On 2017/03/08 21:15:13, pavely wrote: > Could you add "bool ensure_valid_passphrase" parameter to > AwaitEngineInitialization() and eliminate this copy of the function completely. > What if in the future someone needed a flavor that doesn't check for AuthError? Done. https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/2716413003/diff/80001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:614: int clientIndex = 0; On 2017/03/08 21:15:14, pavely wrote: > SyncTest::SetupSync contains sequence of calls to other helper functions with > small amount of code around each call (SetupClients, GetClient()->SetupSync. > > Does it make sense to extract this block of code into separate helper function > to maintain readability of SetupSync? Good point. The readability is bad here. I factored out the code that setup/cleared data into a method. I kept the index and increment around, but I think it's easily readable. https://codereview.chromium.org/2716413003/diff/80001/components/browser_sync... File components/browser_sync/profile_sync_service.cc (right): https://codereview.chromium.org/2716413003/diff/80001/components/browser_sync... components/browser_sync/profile_sync_service.cc:2430: void ProfileSyncService::ClearServerDataForTest(const base::Closure& callback) { On 2017/03/08 21:15:14, pavely wrote: > Why is this function placed at the end of file? Good point. I moved it under ClearAndRestart...
lgtm % my comments https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:79: LOG(WARNING) << "ISEXIT"; Cleanup. https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:143: bool result = SetupSync(syncer::UserSelectableTypes(), false); Yeah... That's the issue with negative parameters. To make function do something you pass false, to make function not do something you pass true. https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:203: if (!AwaitEngineInitialization(!skip_passphrase_verification)) { I think "!skip_..." should be "skip_..." https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:225: if (!skip_passphrase_verification && Just wrap this if/else in an outer if(!skip_....). It'll be clear that skip_... controls this whole piece of logic. Also will be easier to figure out where to put code if you need to add some more logic that only needs to run for passphrase verification. https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/profile_sync_service_harness.h (right): https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.h:61: bool SetupSyncForClear(); What do you think about renaming function to SetupSyncForClearingServerData? With the current name it is not clear what "clear" clears.
https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:267: On 2017/03/09 18:42:26, wylieb wrote: > On 2017/03/08 19:15:02, skym wrote: > > On 2017/03/07 23:04:40, wylieb wrote: > > > On 2017/03/07 21:32:24, skym wrote: > > > > Do you need to call AwaitSyncSetupCompletion() ? > > > > > > Good point. > > > > > > Done. > > > > This never happened as far as I can tell. > > Sorry. I tried to implement this, but it times out when waiting. The condition > that fails is IsSyncActive(). Not sure what's going on there -- got stuck > debugging. The tests pass without this AwaitSyncSetup() call. Doing this wait in > the SetupSyncForClear() method seems to be enough. Thoughts? Was it possible this was while the blocker was still active? What if you manually reset the blocker, call SetFirstSetupComplete(), and then await. I think this needs to be figured out, otherwise we're setting ourselves up for some flakiness.
On 2017/03/10 17:36:31, skym wrote: > https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... > File chrome/browser/sync/test/integration/profile_sync_service_harness.cc > (right): > > https://codereview.chromium.org/2716413003/diff/40001/chrome/browser/sync/tes... > chrome/browser/sync/test/integration/profile_sync_service_harness.cc:267: > On 2017/03/09 18:42:26, wylieb wrote: > > On 2017/03/08 19:15:02, skym wrote: > > > On 2017/03/07 23:04:40, wylieb wrote: > > > > On 2017/03/07 21:32:24, skym wrote: > > > > > Do you need to call AwaitSyncSetupCompletion() ? > > > > > > > > Good point. > > > > > > > > Done. > > > > > > This never happened as far as I can tell. > > > > Sorry. I tried to implement this, but it times out when waiting. The condition > > that fails is IsSyncActive(). Not sure what's going on there -- got stuck > > debugging. The tests pass without this AwaitSyncSetup() call. Doing this wait > in > > the SetupSyncForClear() method seems to be enough. Thoughts? > > Was it possible this was while the blocker was still active? What if you > manually reset the blocker, call SetFirstSetupComplete(), and then await. I > think this needs to be figured out, otherwise we're setting ourselves up for > some flakiness. We spoke about this offline, I made some changes according to what we spoke about. It worked, and it's pretty stable now.
https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:79: LOG(WARNING) << "ISEXIT"; On 2017/03/10 06:14:41, pavely wrote: > Cleanup. Done. https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:143: bool result = SetupSync(syncer::UserSelectableTypes(), false); On 2017/03/10 06:14:42, pavely wrote: > Yeah... That's the issue with negative parameters. To make function do something > you pass false, to make function not do something you pass true. Done. https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:203: if (!AwaitEngineInitialization(!skip_passphrase_verification)) { On 2017/03/10 06:14:41, pavely wrote: > I think "!skip_..." should be "skip_..." Done. https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:225: if (!skip_passphrase_verification && On 2017/03/10 06:14:41, pavely wrote: > Just wrap this if/else in an outer if(!skip_....). It'll be clear that skip_... > controls this whole piece of logic. Also will be easier to figure out where to > put code if you need to add some more logic that only needs to run for > passphrase verification. I actually just passed this block entirely! https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/profile_sync_service_harness.h (right): https://codereview.chromium.org/2716413003/diff/100001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.h:61: bool SetupSyncForClear(); On 2017/03/10 06:14:42, pavely wrote: > What do you think about renaming function to SetupSyncForClearingServerData? > With the current name it is not clear what "clear" clears. I think this is a good idea.
The CQ bit was checked by wylieb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2716413003/#ps120001 (title: "Adding confirmationuiclosed to prevent timeouts, addressing some naming concerns, and removing logg…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:15: No new line here. base is part of chromium project. https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:225: LoginUIServiceFactory::GetForProfile(profile_)->SyncConfirmationUIClosed( This call is copy/pasted from SyncTest::SetupSync. Is it still needed in there as well? https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:229: if (skip_passphrase_verification) { From this line till the end of the function references to skip_passphrase_verification are redundant, skip_passphrase_verification is always false. Could you remove the references. Also... does adding parameter to AwaitSyncSetupCompletion still make sense considering its value will always be "false"?
The CQ bit was unchecked by wylieb@chromium.org
https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/profile_sync_service_harness.cc (right): https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:225: LoginUIServiceFactory::GetForProfile(profile_)->SyncConfirmationUIClosed( On 2017/03/10 21:08:20, pavely wrote: > This call is copy/pasted from SyncTest::SetupSync. Is it still needed in there > as well? No, I moved it here because it was causing timeouts. If you look at the proceeding line, it makes sense to be here. SetupSync() is called on all clients, and thus this will be executed for all clients. The difference being that it's executed on a per-client basis rather than all-at-once. https://codereview.chromium.org/2716413003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/profile_sync_service_harness.cc:229: if (skip_passphrase_verification) { On 2017/03/10 21:08:20, pavely wrote: > From this line till the end of the function references to > skip_passphrase_verification are redundant, skip_passphrase_verification is > always false. Could you remove the references. > Also... does adding parameter to AwaitSyncSetupCompletion still make sense > considering its value will always be "false"? I think you're right. I removed the argument, and references to it in the function after the return statement.
lgtm
The CQ bit was checked by wylieb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2716413003/#ps140001 (title: "Addressing newlines, and unused arguments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489183563549490,
"parent_rev": "18de5022a2a0a25b86c2919783426a9276728e56", "commit_rev":
"0b596d6acab4b78d658879a7584c6743cf63d7cf"}
Message was sent while issue was closed.
Description was changed from ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST=run this command ninja -C out/Debug sync_integration_tests -j 200 && \ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= ========== to ========== Initial clear server data impl. When we run integration tests, we need accounts with a clean slate. This CL clears out the data on sync servers for the given test account, and restarts sync. This operation doesn't require read/writing any data to the account, and thus passphrase decryption is by-passed (Some tests have the side-effect of setting a decryption passphrase that requires clear server data to be run without decryption). R=pavely@chromium.org,skym@chromium.org TEST=run this command ninja -C out/Debug sync_integration_tests -j 200 && \ xvfb-run --server-args="-screen 0 1920x1080x24" \ out/Debug/sync_integration_tests \ --gtest_also_run_disabled_tests \ --gtest_filter=*E2ETest* \ --sync-url=https://clients4.google.com/chrome-sync/dev \ --sync-user-for-test=testuser@gmail.com\ --sync-password-for-test=testpassword --vmodule=*sync*=1 BUG= Review-Url: https://codereview.chromium.org/2716413003 Cr-Commit-Position: refs/heads/master@{#456199} Committed: https://chromium.googlesource.com/chromium/src/+/0b596d6acab4b78d658879a7584c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0b596d6acab4b78d658879a7584c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
