devserver: introduce --datadir option to override where devserver puts static

devserver: introduce --datadir option to override where devserver puts static

BUG=chromium-os:5246
TEST=create new unit test, make sure it fails before and passes after

Change-Id: I21270f261eb067ace334a6414ce546f6235b1966

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=5d21a2a

[davidjames, 2011-02-11 23:31:53]

I don't know this file that well so I'll just put style comments.

http://codereview.chromium.org/6508001/diff/2002/devserver.py
File devserver.py (right):

http://codereview.chromium.org/6508001/diff/2002/devserver.py#newcode191
devserver.py:191: cherrypy.log('Data dir is %s' %options.datadir, 'DEVSERVER')
Need a space after the % sign.

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py
File devserver_test.py (right):

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode94
devserver_test.py:94: def _StartServer(self,datadir=''):
space after comma

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode116
devserver_test.py:116: response = connection.read()
connection.close()

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode132
devserver_test.py:132: contents = connection.read()
connection.close()

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode142
devserver_test.py:142: time.sleep(1)
I see you copied this from above. This sleep seems a bit flaky though...

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode146
devserver_test.py:146: STATIC_DIR + '/' + TEST_IMAGE_NAME)
continued lines have 4 space indent. Also, why not just do
os.path.join(TEST_DATA_PATH, STATIC_DIR, TEST_IMAGE_NAME)

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode153
devserver_test.py:153: response = connection.read()
connection.close()

http://codereview.chromium.org/6508001/diff/2002/devserver_test.py#newcode169
devserver_test.py:169: contents = connection.read()
connection.close()

[zbehan, 2011-02-11 23:43:49]

Fixed all of them. PTAL

On Sat, Feb 12, 2011 at 12:41 AM, <zbehan@chromium.org> wrote:

> http://codereview.chromium.org/6508001/
>

[sosa, 2011-02-11 23:59:19]

Cpl Nits  ... LGTM

http://codereview.chromium.org/6508001/diff/3003/devserver.py
File devserver.py (right):

http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode124
devserver.py:124: parser.add_option('--datadir', dest='datadir',
data_dir

http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode191
devserver.py:191: cherrypy.log('Data dir is %s' % options.datadir, 'DEVSERVER')
This is already logged when we print out the static dir

[zbehan, 2011-02-12 00:09:53]

On Sat, Feb 12, 2011 at 12:59 AM, <sosa@chromium.org> wrote:

> Cpl Nits  ... LGTM
>
>
> http://codereview.chromium.org/6508001/diff/3003/devserver.py
> File devserver.py (right):
>
> http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode124
> devserver.py:124: parser.add_option('--datadir', dest='datadir',
> data_dir
>
> http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode191
>
> devserver.py:191: cherrypy.log('Data dir is %s' % options.datadir,
> 'DEVSERVER')
> This is already logged when we print out the static dir


I guess. I wanted to explicitly print it since it's newly a commandline
option.


>
>
> http://codereview.chromium.org/6508001/
>

[sosa, 2011-02-12 00:24:19]

You could also just remove the other one :)

On Fri, Feb 11, 2011 at 4:09 PM, Zdenek Behan <zbehan@chromium.org> wrote:
> On Sat, Feb 12, 2011 at 12:59 AM, <sosa@chromium.org> wrote:
>>
>> Cpl Nits  ... LGTM
>>
>>
>> http://codereview.chromium.org/6508001/diff/3003/devserver.py
>> File devserver.py (right):
>>
>> http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode124
>> devserver.py:124: parser.add_option('--datadir', dest='datadir',
>> data_dir
>>
>> http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode191
>> devserver.py:191: cherrypy.log('Data dir is %s' % options.datadir,
>> 'DEVSERVER')
>> This is already logged when we print out the static dir
>
> I guess. I wanted to explicitly print it since it's newly a commandline
> option.
>
>>
>> http://codereview.chromium.org/6508001/
>
>

[zbehan, 2011-02-12 00:24:40]

Meh, like this, the new test is actually flaky. The file served by devserver
has to be pre-existing... Now it fails once and succeeds once.

On Sat, Feb 12, 2011 at 1:09 AM, Zdenek Behan <zbehan@chromium.org> wrote:

> On Sat, Feb 12, 2011 at 12:59 AM, <sosa@chromium.org> wrote:
>
>> Cpl Nits  ... LGTM
>>
>>
>> http://codereview.chromium.org/6508001/diff/3003/devserver.py
>> File devserver.py (right):
>>
>> http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode124
>> devserver.py:124: parser.add_option('--datadir', dest='datadir',
>> data_dir
>>
>> http://codereview.chromium.org/6508001/diff/3003/devserver.py#newcode191
>>
>> devserver.py:191: cherrypy.log('Data dir is %s' % options.datadir,
>> 'DEVSERVER')
>> This is already logged when we print out the static dir
>
>
> I guess. I wanted to explicitly print it since it's newly a commandline
> option.
>
>
>>
>>
>> http://codereview.chromium.org/6508001/
>>
>
>

[zbehan, 2011-02-12 00:37:36]

Ok. Fixed datadir -> data_dir, and moved file copy before server start,
which eliminates flakiness.

On Sat, Feb 12, 2011 at 1:36 AM, <zbehan@chromium.org> wrote:

> http://codereview.chromium.org/6508001/
>

[sosa, 2011-02-12 00:52:21]

LGTM