Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8510075:
Fixed and added new tests in SyncIntegrationTest class. (Closed)
|
Created:
9 years, 1 month ago by dyu1 Modified:
9 years, 1 month ago CC:
chromium-reviews, Nirnimesh, John Grabowski, anantha, Paweł Hajdan Jr., dennis_jeffrey Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFixed and added new tests in SyncIntegrationTest class.
Added AwaitSyncCycleCompletion function to all tests after signing into sync to allow for full sync before proceeding.
Added Autofill sync test and Credit card sync test.
Changed test page from google.com to local web file using http server.
Disabled SyncIntegrationTest class in FULL suite due to History backend crash when logging into the sync server.
TEST=none
BUG=104227
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110816
Patch Set 1 #
Total comments: 21
Patch Set 2 : '' #
Total comments: 15
Patch Set 3 : '' #
Total comments: 1
Patch Set 4 : '' #Patch Set 5 : '' #Messages
Total messages: 11 (0 generated)
http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/PYAUTO_T... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/PYAUTO_T... chrome/test/functional/PYAUTO_TESTS:137: #'sync.SyncIntegrationTest', Is this disabled temporarily or permanently? If permanent, we should remove lines 135-137. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:169: # Launch a new instance of the browser with a clean profile (Browser 2) nit: period at end of sentence http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:177: self.FillAutofillProfile(profiles=profile, credit_cards=credit_card) where is "credit_card" defined? http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:180: # Log into the account and sync the browser to the account. '...sync the second browser...' http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:188: msg=('Browser 1 profile %d does not match Browser 2 profile %d' Do you mean '%s' instead of '%d'? I think the values you're trying to print here are dictionaries, not numbers, right? http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:192: """Verify credit card info does not sync between wto browsers.""" 'wto' --> 'two' http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:197: # Launch a new instance of the browser with a clean profile (Browser 2) nit: period at end of sentence http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:206: browser1_profile = self.GetAutofillProfile() This variable is unused. Should you use it to verify that the credit card info exists (in browser 1)? http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:208: # Log into the account and sync the browser to the account. '...sync the second browser...' http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:216: msg='Browser 2 contains credit card info.') 'Browser 2 unexpectedly contains...'
http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/PYAUTO_T... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/PYAUTO_T... chrome/test/functional/PYAUTO_TESTS:137: #'sync.SyncIntegrationTest', On 2011/11/15 02:07:13, dennis_jeffrey wrote: > Is this disabled temporarily or permanently? If permanent, we should remove > lines 135-137. Temp, I added the the disable to lines 160,161. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:169: # Launch a new instance of the browser with a clean profile (Browser 2) On 2011/11/15 02:07:13, dennis_jeffrey wrote: > nit: period at end of sentence Done. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:177: self.FillAutofillProfile(profiles=profile, credit_cards=credit_card) On 2011/11/15 02:07:13, dennis_jeffrey wrote: > where is "credit_card" defined? Removed. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:180: # Log into the account and sync the browser to the account. On 2011/11/15 02:07:13, dennis_jeffrey wrote: > '...sync the second browser...' Done. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:188: msg=('Browser 1 profile %d does not match Browser 2 profile %d' On 2011/11/15 02:07:13, dennis_jeffrey wrote: > Do you mean '%s' instead of '%d'? I think the values you're trying to print > here are dictionaries, not numbers, right? Done. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:192: """Verify credit card info does not sync between wto browsers.""" On 2011/11/15 02:07:13, dennis_jeffrey wrote: > 'wto' --> 'two' Done. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:197: # Launch a new instance of the browser with a clean profile (Browser 2) On 2011/11/15 02:07:13, dennis_jeffrey wrote: > nit: period at end of sentence Done. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:206: browser1_profile = self.GetAutofillProfile() On 2011/11/15 02:07:13, dennis_jeffrey wrote: > This variable is unused. Should you use it to verify that the credit card info > exists (in browser 1)? I added a check. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:208: # Log into the account and sync the browser to the account. On 2011/11/15 02:07:13, dennis_jeffrey wrote: > '...sync the second browser...' Done. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/sync.py#... chrome/test/functional/sync.py:216: msg='Browser 2 contains credit card info.') On 2011/11/15 02:07:13, dennis_jeffrey wrote: > 'Browser 2 unexpectedly contains...' Done.
LGTM, but just one question about a line in the PYAUTO_TESTS file. http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/PYAUTO_T... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/8510075/diff/1/chrome/test/functional/PYAUTO_T... chrome/test/functional/PYAUTO_TESTS:137: #'sync.SyncIntegrationTest', On 2011/11/15 02:56:08, dyu1 wrote: > On 2011/11/15 02:07:13, dennis_jeffrey wrote: > > Is this disabled temporarily or permanently? If permanent, we should remove > > lines 135-137. > > Temp, I added the the disable to lines 160,161. If we're disabling it down below, why do we need to comment out the current line? This is just another change we'll have to make later when we re-enable the test.
http://codereview.chromium.org/8510075/diff/4001/chrome/test/data/sync/basic_... File chrome/test/data/sync/basic_page.html (right): http://codereview.chromium.org/8510075/diff/4001/chrome/test/data/sync/basic_... chrome/test/data/sync/basic_page.html:1: <html> Why not reuse one of the existing html files? http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:147: self.AwaitSyncCycleCompletion() Don't you have to do this in the 2nd browser as well? http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:157: def testAddSingleProfileAndVerifySync(self): Profile is an over-loaded term (autofill profile, multi-profile, regular profile, profile dir). |Verify| in test name is redundant. How about: testAutofillProfileSync http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:158: """Verify a single profile syncs between two browsers. profile -> autofill profile http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:182: self.AwaitSyncCycleCompletion() Wait for sync cycle completion on the 2nd browser as well. http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:191: def testAddCreditCardAndVerifyNoSync(self): testNoCreditCardSync http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:213: self.AwaitSyncCycleCompletion() on browser2 as well
http://codereview.chromium.org/8510075/diff/4001/chrome/test/data/sync/basic_... File chrome/test/data/sync/basic_page.html (right): http://codereview.chromium.org/8510075/diff/4001/chrome/test/data/sync/basic_... chrome/test/data/sync/basic_page.html:1: <html> On 2011/11/15 18:21:16, Nirnimesh wrote: > Why not reuse one of the existing html files? done http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:147: self.AwaitSyncCycleCompletion() On 2011/11/15 18:21:16, Nirnimesh wrote: > Don't you have to do this in the 2nd browser as well? Did you mean for the first browser? This is the 2nd browser. I added this to line 133. http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:157: def testAddSingleProfileAndVerifySync(self): On 2011/11/15 18:21:16, Nirnimesh wrote: > Profile is an over-loaded term (autofill profile, multi-profile, regular > profile, profile dir). |Verify| in test name is redundant. > How about: testAutofillProfileSync Done. http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:158: """Verify a single profile syncs between two browsers. On 2011/11/15 18:21:16, Nirnimesh wrote: > profile -> autofill profile Done. http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:182: self.AwaitSyncCycleCompletion() On 2011/11/15 18:21:16, Nirnimesh wrote: > Wait for sync cycle completion on the 2nd browser as well. Do you mean first browser? http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:191: def testAddCreditCardAndVerifyNoSync(self): On 2011/11/15 18:21:16, Nirnimesh wrote: > testNoCreditCardSync Done. http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:213: self.AwaitSyncCycleCompletion() On 2011/11/15 18:21:16, Nirnimesh wrote: > on browser2 as well browser 1?
http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... chrome/test/functional/sync.py:147: self.AwaitSyncCycleCompletion() On 2011/11/17 20:01:31, dyu1 wrote: > On 2011/11/15 18:21:16, Nirnimesh wrote: > > Don't you have to do this in the 2nd browser as well? > > Did you mean for the first browser? This is the 2nd browser. I added this to > line 133. |self| is the first browser. browser2 is the second browser. Isn't it?
Yes, |self| is the first browser and browser2 is the second browser. I had "self.AwaitSyncCycleCompletion" after logging into browser2. I didn't have this after logging into |self| but I added this in the recent change. On 2011/11/17 20:15:35, Nirnimesh wrote: > http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.py > File chrome/test/functional/sync.py (right): > > http://codereview.chromium.org/8510075/diff/4001/chrome/test/functional/sync.... > chrome/test/functional/sync.py:147: self.AwaitSyncCycleCompletion() > On 2011/11/17 20:01:31, dyu1 wrote: > > On 2011/11/15 18:21:16, Nirnimesh wrote: > > > Don't you have to do this in the 2nd browser as well? > > > > Did you mean for the first browser? This is the 2nd browser. I added this to > > line 133. > > |self| is the first browser. browser2 is the second browser. Isn't it?
http://codereview.chromium.org/8510075/diff/10001/chrome/test/functional/sync.py File chrome/test/functional/sync.py (right): http://codereview.chromium.org/8510075/diff/10001/chrome/test/functional/sync... chrome/test/functional/sync.py:148: self.AwaitSyncCycleCompletion() I don't understand. Where are you ensuring that the 2nd browser has finished syncing from sync server? I'm suggesting: browser2.AwaitSyncCycleCompletion()
Done On 2011/11/17 20:26:04, Nirnimesh wrote: > http://codereview.chromium.org/8510075/diff/10001/chrome/test/functional/sync.py > File chrome/test/functional/sync.py (right): > > http://codereview.chromium.org/8510075/diff/10001/chrome/test/functional/sync... > chrome/test/functional/sync.py:148: self.AwaitSyncCycleCompletion() > I don't understand. Where are you ensuring that the 2nd browser has finished > syncing from sync server? > > I'm suggesting: > browser2.AwaitSyncCycleCompletion()
LGTM |
