Test birtday error.

Test birtday error.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94818

[lipalani1, 2011-07-21 21:29:51]

Tim - Please review the test portion.
Nick - Please review the python portion. I believe the code that I have added is
not easily unit testable(and is probably not needed to be tested as the change
is not in the syncdatamodel). or it might be because of my ignorance about
python. Let me know!

[tim (not reviewing), 2011-07-26 19:32:17]

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:319: }
should have a a break; here

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:465: DCHECK(wait_state_ !=
SYNC_DISABLED);
DCHECK_NE

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:468: if (wait_state_ ==
SYNC_DISABLED) {
this would be clearer as return wait_state_ == SYNC_DISABLED

http://codereview.chromium.org/7481009/diff/2002/chrome/test/live_sync/sync_e...
File chrome/test/live_sync/sync_errors_test.cc (right):

http://codereview.chromium.org/7481009/diff/2002/chrome/test/live_sync/sync_e...
chrome/test/live_sync/sync_errors_test.cc:7: 
nit - remove extra newline

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1471: def
ChromiumSyncBirthdayErrorOpHanlder(self):
OpHandler typo

[try_nick_at_chromium_org, 2011-07-26 19:50:22]

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/chromiu...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:959: self.account.store_birthday = '%0.30f'
% random.random()
I'd prefer this be a call into a method of SyncDataModel.  The method should be
like:

def ResetStoreBirthday(self):
  # TODO(nick): uuid.uuid1() is better, but python 2.5 only.
  self.store_birthday = '%0.30f' % random.random()

And it should be called from SyncDataModel's __init__ function as well.

Bonus points for fixing CheckStoreBirthday() so that it doesn't reach into
self.account.store_birthday.

[lipalani1, 2011-07-27 01:15:22]

Please review once again.

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:319: }
On 2011/07/26 19:32:17, timsteele wrote:
> should have a a break; here

Done.

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:465: DCHECK(wait_state_ !=
SYNC_DISABLED);
On 2011/07/26 19:32:17, timsteele wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/7481009/diff/2002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:468: if (wait_state_ ==
SYNC_DISABLED) {
On 2011/07/26 19:32:17, timsteele wrote:
> this would be clearer as return wait_state_ == SYNC_DISABLED

Done.

http://codereview.chromium.org/7481009/diff/2002/chrome/test/live_sync/sync_e...
File chrome/test/live_sync/sync_errors_test.cc (right):

http://codereview.chromium.org/7481009/diff/2002/chrome/test/live_sync/sync_e...
chrome/test/live_sync/sync_errors_test.cc:7: 
On 2011/07/26 19:32:17, timsteele wrote:
> nit - remove extra newline

Done.

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/chromiu...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/chromiu...
net/tools/testserver/chromiumsync.py:959: self.account.store_birthday = '%0.30f'
% random.random()
On 2011/07/26 19:50:22, try_nick_at_chromium_org wrote:
> I'd prefer this be a call into a method of SyncDataModel.  The method should
be
> like:
> 
> def ResetStoreBirthday(self):
>   # TODO(nick): uuid.uuid1() is better, but python 2.5 only.
>   self.store_birthday = '%0.30f' % random.random()
> 
> And it should be called from SyncDataModel's __init__ function as well.
> 
> Bonus points for fixing CheckStoreBirthday() so that it doesn't reach into
> self.account.store_birthday.

Done.

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7481009/diff/2002/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1471: def
ChromiumSyncBirthdayErrorOpHanlder(self):
On 2011/07/26 19:32:17, timsteele wrote:
> OpHandler typo

Done.

[tim (not reviewing), 2011-07-27 18:16:49]

cc changes LGTM.

I'm cc'ing Raghu as an FYI / TBR review for the harness changes.

[ncarter (slow), 2011-07-29 18:40:39]

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:604: """
The closing """ should be on the prev line for a single-line docstring.  This
isn't explicitly mentioned in the google python style guide, but it's a
convention used extensively in the examples there.

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:610: """
Here too.

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1485: if not
self._shouldHandleRequest(test_name):
_shouldHandleRequest << the S should be capitalized, otherwise this code won't
work.

[lipalani1, 2011-07-29 19:33:24]

Please review. I am rebuilding clean(which will take another 20 mts) and will
run the testcase after that.

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:604: """
On 2011/07/29 18:40:40, ncarter wrote:
> The closing """ should be on the prev line for a single-line docstring.  This
> isn't explicitly mentioned in the google python style guide, but it's a
> convention used extensively in the examples there.

Done.

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:610: """
On 2011/07/29 18:40:40, ncarter wrote:
> Here too.

Done.

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7481009/diff/11012/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1485: if not
self._shouldHandleRequest(test_name):
Not sure how this got changed. May I was making some last minute changed and
accidentally did a ctrl-z.
On 2011/07/29 18:40:40, ncarter wrote:
> _shouldHandleRequest << the S should be capitalized, otherwise this code won't
> work.

Done.

[ncarter (slow), 2011-07-29 19:36:33]

LGTM

[commit-bot: I haz the power, 2011-07-29 20:07:46]

Can't apply patch for file chrome/chrome_tests.gypi.
While running patch -p1 --forward --force;
patching file chrome/chrome_tests.gypi
Hunk #1 FAILED at 3532.
1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej

[commit-bot: I haz the power, 2011-07-29 23:09:01]

Try job failure for 7481009-22001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-07-30 02:17:40]

Change committed as 94818