Added rlz testcases.

Add rlz test.

BUG=NONE
TEST=NONE

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

[prachij, 2012-09-07 23:59:15]

Hi Roger/Anantha,

I have added following rlz testcases.

-Testing rlz pings for first chrome launch with google as default search engine.
-Testing rlz ping after typing anything in omnibox before ping is sent out.
-Testing rlz ping after typing anything in omnibox after ping is sent out.
 for details follow below link:
https://sites.google.com/a/google.com/chrome-te/home/installer/rlz-test-plan?...

I haven't done code refractoring now so you will see some duplicate code, I will
do once I automate few more testcases.

Thanks,
-Prachi

[Roger Tawa OOO till Jul 10th, 2012-09-11 00:29:42]

Hi Prachi,

Looks good, but seems like there are some unfinished bits.  Also please take a
look at the google python style guide and update the file accordingly.  Thanks.

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py
File rlz_test/rlztest.py (right):

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode33
rlz_test/rlztest.py:33: 'chromium_proxy_server_version3.py',
is this file name correct?

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode41
rlz_test/rlztest.py:41: r'C:\Users\prachij\chromedriver.exe')
path is hardcoded to your own home dir.  Should make this settable via a command
line argument.

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode81
rlz_test/rlztest.py:81: def GetKeyValues(self,key, subkey):
add space before key

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode81
rlz_test/rlztest.py:81: def GetKeyValues(self,key, subkey):
I think it should be GetKeyValueNames().

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode83
rlz_test/rlztest.py:83: list_names =[]
space before []

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode94
rlz_test/rlztest.py:94: #print list_names
remove line

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode103
rlz_test/rlztest.py:103: time.sleep(2)
why sleep?

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode108
rlz_test/rlztest.py:108: 'Software\Google\Common\Rlz\Events :\n',list_key
don't use \ at end.  do this:

  print ('...long string...',
         '...other string...', list key)

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode113
rlz_test/rlztest.py:113: time.sleep(90)
this could ne flaky.  should probably wait a little longer though, since chrome
is also waiting 90 seconds by default.  You can set this to 20 using the master
prefs one you have a real install of chrome.

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode118
rlz_test/rlztest.py:118: contents = open(os.getcwd()+'\ServerXXX.log', 'r',
1).read()
ServerXXX.log ?

[prachij, 2012-09-11 21:04:19]

Hi Roger,

Thanks for reviewing the CL. I have made changes as per you recommended.

Thanks,
-Prachi

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py
File rlz_test/rlztest.py (right):

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:33: 'chromium_proxy_server_version3.py',
As we were making quite lot of changes to proxy server, we started naming the
file with versions. So this was the proxy server file I had on my local. I have
changed this and now also setting this through command line.
On 2012/09/11 00:29:42, Roger Tawa wrote:
> is this file name correct?

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:41: r'C:\Users\prachij\chromedriver.exe')
changed to take it from command line.
On 2012/09/11 00:29:42, Roger Tawa wrote:
> path is hardcoded to your own home dir.  Should make this settable via a
command
> line argument.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:81: def GetKeyValues(self,key, subkey):
On 2012/09/11 00:29:42, Roger Tawa wrote:
> add space before key

Done.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:81: def GetKeyValues(self,key, subkey):
On 2012/09/11 00:29:42, Roger Tawa wrote:
> I think it should be GetKeyValueNames().

Done.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:83: list_names =[]
On 2012/09/11 00:29:42, Roger Tawa wrote:
> space before []

Done.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:94: #print list_names
On 2012/09/11 00:29:42, Roger Tawa wrote:
> remove line

Done.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:108: 'Software\Google\Common\Rlz\Events :\n',list_key
On 2012/09/11 00:29:42, Roger Tawa wrote:
> don't use \ at end.  do this:
> 
>   print ('...long string...',
>          '...other string...', list key)

Done.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:113: time.sleep(90)
Till now I haven't seen this flaky, but may be you are right I should increase
it little longer in case. Yes, once the installer script are ready will set that
through master pref.
On 2012/09/11 00:29:42, Roger Tawa wrote:
> this could ne flaky.  should probably wait a little longer though, since
chrome
> is also waiting 90 seconds by default.  You can set this to 20 using the
master
> prefs one you have a real install of chrome.

https://chromiumcodereview.appspot.com/10910145/diff/2001/rlz_test/rlztest.py...
rlz_test/rlztest.py:118: contents = open(os.getcwd()+'\ServerXXX.log', 'r',
1).read()
This is global log file created my proxy server which I am reading in order to
assert pings.
On 2012/09/11 00:29:42, Roger Tawa wrote:
> ServerXXX.log ?

[Roger Tawa OOO till Jul 10th, 2012-09-14 19:43:36]

A few more comments below.  Please also take a look at style guide for style
related issues (http://google-styleguide.googlecode.com/svn/trunk/pyguide.html).

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py
File rlz_test/rlztest.py (right):

http://codereview.chromium.org/10910145/diff/2001/rlz_test/rlztest.py#newcode118
rlz_test/rlztest.py:118: contents = open(os.getcwd()+'\ServerXXX.log', 'r',
1).read()
On 2012/09/11 21:04:19, prachij wrote:
> This is global log file created my proxy server which I am reading in order to
> assert pings.
> On 2012/09/11 00:29:42, Roger Tawa wrote:
> > ServerXXX.log ?
> 

Hmmm, should probably choose a better name :-)

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py
File rlz_test/rlztest.py (right):

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode13
rlz_test/rlztest.py:13: import os
please put all these imports in alpha order.
please use standard chromium source file header.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode18
rlz_test/rlztest.py:18: from chromium_proxy_server_version3 import
ChromiumProxyServer
you can remove the imports at lines 16-18

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode19
rlz_test/rlztest.py:19: from subprocess import Popen, PIPE, STDOUT
remove line 19, not needed since you do it at line 1

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode20
rlz_test/rlztest.py:20: from selenium.webdriver.common.keys import Keys as Keys
why do you need the "as Keys" part?

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode31
rlz_test/rlztest.py:31: "your system(c:\\chrome\\..):")
Is this script meant to be use interactively?

This will get repetitive I think since setUp() is called before each testXXX
function.  I think it would be better to do as command line args, or at least
ask only once before calling unittest.main()

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode39
rlz_test/rlztest.py:39:
'--urls=http://clients1.google.com/tools/pso/ping,www.google.com'])
lines 35-39 should be indented only 4 spaces from line 34.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode84
rlz_test/rlztest.py:84: """Get the values for particular subkey"""
plese document args and return.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode106
rlz_test/rlztest.py:106: 'Software\Google\Common\Rlz\Events\C')
spaces around = in two lines above.  Please also fix indentation.

don't you need to double up the backslash \?  Or use as raw string?  this
applies to all string with backslahes, like line 108 below,

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode108
rlz_test/rlztest.py:108: 'Software\Google\Common\Rlz\Events :\n', list_key)
line up the two starting single quotes.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode110
rlz_test/rlztest.py:110: #self.assertEqual('C2I', list_key[1])
should these be commented out?

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode134
rlz_test/rlztest.py:134: print '\nChrome Launch ping send :\n',
contents[start:end]
send -> sent ?

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode141
rlz_test/rlztest.py:141: 'Software\Google\Common\Rlz\StatefulEvents:', list_key)
line up single quotes.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode157
rlz_test/rlztest.py:157: time.sleep(100)
comment says 90

[prachij, 2012-09-19 19:30:16]

Hi Roger,

Thanks for your review. I have made changes as per you suggested. Also, I have
done code refactoring and also added some more assertion if one of the
testcases. I have also changed name of proxy_server file as well as log file as
per the latest file handed over to me by Navdeep.

Thanks,
-Prachi

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py
File rlz_test/rlztest.py (right):

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode13
rlz_test/rlztest.py:13: import os
On 2012/09/14 19:43:36, Roger Tawa wrote:
> please put all these imports in alpha order.
> please use standard chromium source file header.

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode18
rlz_test/rlztest.py:18: from chromium_proxy_server_version3 import
ChromiumProxyServer
On 2012/09/14 19:43:36, Roger Tawa wrote:
> you can remove the imports at lines 16-18

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode19
rlz_test/rlztest.py:19: from subprocess import Popen, PIPE, STDOUT
On 2012/09/14 19:43:36, Roger Tawa wrote:
> remove line 19, not needed since you do it at line 1

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode20
rlz_test/rlztest.py:20: from selenium.webdriver.common.keys import Keys as Keys
I am using native events like ctrl, enter in line 167 where I am adding this to
Keys. If I don't use "as Keys" I end up getting some exception though not sure
why.
On 2012/09/14 19:43:36, Roger Tawa wrote:
> why do you need the "as Keys" part?

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode31
rlz_test/rlztest.py:31: "your system(c:\\chrome\\..):")
My intent was to ask only once. I was planning to move this in
'setupbeforeclass()' method, but its added in python2.7. Most of people use
python2.6 so I didn't went forward with it. Another alternate was to use command
line arg, but since this script might be used my manual tester in future I
wanted to make sure they add correct data before running. So now I have created
a separate method to handle this, which I am going to call before main.So, now
this will only be called once.
On 2012/09/14 19:43:36, Roger Tawa wrote:
> Is this script meant to be use interactively?
> 
> This will get repetitive I think since setUp() is called before each testXXX
> function.  I think it would be better to do as command line args, or at least
> ask only once before calling unittest.main()

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode84
rlz_test/rlztest.py:84: """Get the values for particular subkey"""
On 2012/09/14 19:43:36, Roger Tawa wrote:
> plese document args and return.

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode106
rlz_test/rlztest.py:106: 'Software\Google\Common\Rlz\Events\C')
This was working fine with single \, but just to be on safe side I have changed
it to double.
On 2012/09/14 19:43:36, Roger Tawa wrote:
> spaces around = in two lines above.  Please also fix indentation.
> 
> don't you need to double up the backslash \?  Or use as raw string?  this
> applies to all string with backslahes, like line 108 below,

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode108
rlz_test/rlztest.py:108: 'Software\Google\Common\Rlz\Events :\n', list_key)
On 2012/09/14 19:43:36, Roger Tawa wrote:
> line up the two starting single quotes.

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode110
rlz_test/rlztest.py:110: #self.assertEqual('C2I', list_key[1])
This should not be commented out, but for some reason these assertions are
failing now, they were passing before. Till I  investigate what the problem is I
have temporary commented out.
On 2012/09/14 19:43:36, Roger Tawa wrote:
> should these be commented out?

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode134
rlz_test/rlztest.py:134: print '\nChrome Launch ping send :\n',
contents[start:end]
On 2012/09/14 19:43:36, Roger Tawa wrote:
> send -> sent ?

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode141
rlz_test/rlztest.py:141: 'Software\Google\Common\Rlz\StatefulEvents:', list_key)
On 2012/09/14 19:43:36, Roger Tawa wrote:
> line up single quotes.

Done.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode157
rlz_test/rlztest.py:157: time.sleep(100)
On 2012/09/14 19:43:36, Roger Tawa wrote:
> comment says 90

Done.

[Roger Tawa OOO till Jul 10th, 2012-09-20 19:13:19]

lgtm

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py
File rlz_test/rlztest.py (right):

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode20
rlz_test/rlztest.py:20: from selenium.webdriver.common.keys import Keys as Keys
On 2012/09/19 19:30:16, prachij wrote:
> I am using native events like ctrl, enter in line 167 where I am adding this
to
> Keys. If I don't use "as Keys" I end up getting some exception though not sure
> why.
> On 2012/09/14 19:43:36, Roger Tawa wrote:
> > why do you need the "as Keys" part?
> 

Can you tell me what exceptions you are getting?

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode31
rlz_test/rlztest.py:31: "your system(c:\\chrome\\..):")
On 2012/09/19 19:30:16, prachij wrote:
> My intent was to ask only once. I was planning to move this in
> 'setupbeforeclass()' method, but its added in python2.7. Most of people use
> python2.6 so I didn't went forward with it. Another alternate was to use
command
> line arg, but since this script might be used my manual tester in future I
> wanted to make sure they add correct data before running. So now I have
created
> a separate method to handle this, which I am going to call before main.So, now
> this will only be called once.
> On 2012/09/14 19:43:36, Roger Tawa wrote:
> > Is this script meant to be use interactively?
> > 
> > This will get repetitive I think since setUp() is called before each testXXX
> > function.  I think it would be better to do as command line args, or at
least
> > ask only once before calling unittest.main()
> 

Looks good.  Better to stick with 2.6 since that is what is installed by default
everywhere.

http://codereview.chromium.org/10910145/diff/7001/rlz_test/rlztest.py#newcode106
rlz_test/rlztest.py:106: 'Software\Google\Common\Rlz\Events\C')
On 2012/09/19 19:30:16, prachij wrote:
> This was working fine with single \, but just to be on safe side I have
changed
> it to double.
> On 2012/09/14 19:43:36, Roger Tawa wrote:
> > spaces around = in two lines above.  Please also fix indentation.
> > 
> > don't you need to double up the backslash \?  Or use as raw string?  this
> > applies to all string with backslahes, like line 108 below,
> 

Single \ works when the following letter happens to not be a special escape
sequence.  So better to double up.  Thanks.

[commit-bot: I haz the power, 2012-09-21 23:22:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prachij@chromium.org/10910145/20001

[commit-bot: I haz the power, 2012-09-21 23:22:56]

Presubmit check for 10910145-20001 failed and returned exit status 1.

/b/commit-queue/workdir/chromium/chrome/test/rlz_test/rlztest.py: Has shebang
but not executable bit
Running presubmit commit checks ...

** Presubmit Warnings **
Found line ending with white spaces in:

***************
chrome/test/rlz_test/rlztest.py, line 259
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/test

[commit-bot: I haz the power, 2012-09-21 23:26:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prachij@chromium.org/10910145/12001

[commit-bot: I haz the power, 2012-09-21 23:26:47]

Presubmit check for 10910145-12001 failed and returned exit status 1.

/b/commit-queue/workdir/chromium/chrome/test/rlz_test/rlztest.py: Has shebang
but not executable bit
Running presubmit commit checks ...

** Presubmit Warnings **
Found line ending with white spaces in:

***************
chrome/test/rlz_test/rlztest.py, line 146
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/test

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[prachij, 2012-09-25 21:25:41]

Hi Ken,

I have changed location of this CL from chrome/test to chrome/test/functional. I
would appreciate if you can approve the same so that I can commit the CL.

Thanks,
-Prachi

[kkania, 2012-09-25 21:43:30]

On 2012/09/25 21:25:41, prachij wrote:
> Hi Ken,
> 
> I have changed location of this CL from chrome/test to chrome/test/functional.
I
> would appreciate if you can approve the same so that I can commit the CL.
> 
> Thanks,
> -Prachi

I am not an owner for chrome/test/functional.

[prachij, 2012-09-25 22:16:11]

Hi Nirnimesh,

I have changed location of this CL from chrome/test to chrome/test/functional.
I would appreciate if you can approve the same so that I can commit the CL.

Thanks,
-Prachi

[Nirnimesh, 2012-09-25 22:33:28]

Rubberstamp LGTM

[commit-bot: I haz the power, 2012-09-25 22:48:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prachij@chromium.org/10910145/26001

[commit-bot: I haz the power, 2012-09-26 00:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prachij@chromium.org/10910145/26001

[commit-bot: I haz the power, 2012-09-26 01:57:21]

Change committed as 158712