Added --test_args parameter that passes its value as -a flag to autoserv.

Added --test_args parameter that passes its value as -a flag to autoserv.

[petkov, 2010-04-27 16:45:52]

Thanks for looking into this. A couple of comments.

http://codereview.chromium.org/1794004/diff/1/2
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/1794004/diff/1/2#newcode25
src/scripts/run_remote_tests.sh:25: DEFINE_string test_args "" "Command line
arguments for test"
Let's rename to args -- no reason to be different than autoserv's long name. You
could also add the short option "a".
Also, keep flag definition sorted.

http://codereview.chromium.org/1794004/diff/1/2#newcode248
src/scripts/run_remote_tests.sh:248: test_args="-a \"${FLAGS_test_args}\""
This script runs the "src/scripts/autotest" script which in turns invokes
"autoserv". Have you confirmed that passing something like:

--args="foo bar"

preserves the quotes down to autoserv, and then you get a two element "args"
variable in the control file? I.e., something like:

args = ['foo', 'bar']

[whywhat, 2010-04-27 17:25:04]

http://codereview.chromium.org/1794004/diff/1/2
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/1794004/diff/1/2#newcode25
src/scripts/run_remote_tests.sh:25: DEFINE_string test_args "" "Command line
arguments for test"
On 2010/04/27 16:45:52, petkov wrote:
> Let's rename to args -- no reason to be different than autoserv's long name.
You
> could also add the short option "a".
> Also, keep flag definition sorted.
> 

Done.

http://codereview.chromium.org/1794004/diff/1/2#newcode248
src/scripts/run_remote_tests.sh:248: test_args="-a \"${FLAGS_test_args}\""
This doesn't work. No matter if I escape the space between args or not, "bar"
becomes part of FLAGS_ARGV and considered as a test name. I guess I'd need to
fix FLAGS for that to work. As a workaround one may pass args comma separated
and split them somewhere later on the path down to the test.

On 2010/04/27 16:45:52, petkov wrote:
> This script runs the "src/scripts/autotest" script which in turns invokes
> "autoserv". Have you confirmed that passing something like:
> 
> --args="foo bar"
> 
> preserves the quotes down to autoserv, and then you get a two element "args"
> variable in the control file? I.e., something like:
> 
> args = ['foo', 'bar']
>

[ericli, 2010-04-27 17:28:55]

I hate to say that, but this is another good example to use
src/script/autotest directly instead of keep adding duplicated args into
run_remote_tests.

Eric

On Tue, Apr 27, 2010 at 10:25 AM, <avayvod@chromium.org> wrote:

>
> http://codereview.chromium.org/1794004/diff/1/2
> File src/scripts/run_remote_tests.sh (right):
>
> http://codereview.chromium.org/1794004/diff/1/2#newcode25
> src/scripts/run_remote_tests.sh:25: DEFINE_string test_args "" "Command
> line arguments for test"
> On 2010/04/27 16:45:52, petkov wrote:
>
>> Let's rename to args -- no reason to be different than autoserv's long
>>
> name. You
>
>> could also add the short option "a".
>> Also, keep flag definition sorted.
>>
>
>
> Done.
>
>
> http://codereview.chromium.org/1794004/diff/1/2#newcode248
> src/scripts/run_remote_tests.sh:248: test_args="-a
> \"${FLAGS_test_args}\""
> This doesn't work. No matter if I escape the space between args or not,
> "bar" becomes part of FLAGS_ARGV and considered as a test name. I guess
> I'd need to fix FLAGS for that to work. As a workaround one may pass
> args comma separated and split them somewhere later on the path down to
> the test.
>
>
> On 2010/04/27 16:45:52, petkov wrote:
>
>> This script runs the "src/scripts/autotest" script which in turns
>>
> invokes
>
>> "autoserv". Have you confirmed that passing something like:
>>
>
>  --args="foo bar"
>>
>
>  preserves the quotes down to autoserv, and then you get a two element
>>
> "args"
>
>> variable in the control file? I.e., something like:
>>
>
>  args = ['foo', 'bar']
>>
>
>
> http://codereview.chromium.org/1794004/show
>



-- 
Eric Li
李咏竹
Google Kirkland

[whywhat, 2010-04-27 18:06:07]

I tried to pass args separated with commas and split them by comma in
third_party/autotest/files/server/autoserv_parser.py - it works. But it would
break other uses of autoserv.

I tried to replace , with space in run_remote_tests.sh and I verified that I get
-a "hello hwqual" in environ['AUTOSERV_ARGS'] in src/scripts/autotest. However I
get only "hello in autoserv. I'm not sure where the transformation takes place.
ebuild-$BOARD is called for autotest with clean, unpack and test.

[petkov, 2010-04-27 20:52:50]

On 2010/04/27 18:06:07, whywhat wrote:
> I tried to pass args separated with commas and split them by comma in
> third_party/autotest/files/server/autoserv_parser.py - it works. But it would
> break other uses of autoserv.
> 
> I tried to replace , with space in run_remote_tests.sh and I verified that I
get
> -a "hello hwqual" in environ['AUTOSERV_ARGS'] in src/scripts/autotest. However
I
> get only "hello in autoserv. I'm not sure where the transformation takes
place.
> ebuild-$BOARD is called for autotest with clean, unpack and test.

Look into the chromeos-base/autotest ebuild and see what you pass to autoserv
there?

[petkov, 2010-04-27 20:54:17]

On 2010/04/27 17:28:55, ericli wrote:
> I hate to say that, but this is another good example to use
> src/script/autotest directly instead of keep adding duplicated args into
> run_remote_tests.

We should be able to do that once all run_remote_tests functionality is in
autotest, right? I'm not sure what the status is on that...

> 
> Eric
> 
> On Tue, Apr 27, 2010 at 10:25 AM, <mailto:avayvod@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/1794004/diff/1/2
> > File src/scripts/run_remote_tests.sh (right):
> >
> > http://codereview.chromium.org/1794004/diff/1/2#newcode25
> > src/scripts/run_remote_tests.sh:25: DEFINE_string test_args "" "Command
> > line arguments for test"
> > On 2010/04/27 16:45:52, petkov wrote:
> >
> >> Let's rename to args -- no reason to be different than autoserv's long
> >>
> > name. You
> >
> >> could also add the short option "a".
> >> Also, keep flag definition sorted.
> >>
> >
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/1794004/diff/1/2#newcode248
> > src/scripts/run_remote_tests.sh:248: test_args="-a
> > \"${FLAGS_test_args}\""
> > This doesn't work. No matter if I escape the space between args or not,
> > "bar" becomes part of FLAGS_ARGV and considered as a test name. I guess
> > I'd need to fix FLAGS for that to work. As a workaround one may pass
> > args comma separated and split them somewhere later on the path down to
> > the test.
> >
> >
> > On 2010/04/27 16:45:52, petkov wrote:
> >
> >> This script runs the "src/scripts/autotest" script which in turns
> >>
> > invokes
> >
> >> "autoserv". Have you confirmed that passing something like:
> >>
> >
> >  --args="foo bar"
> >>
> >
> >  preserves the quotes down to autoserv, and then you get a two element
> >>
> > "args"
> >
> >> variable in the control file? I.e., something like:
> >>
> >
> >  args = ['foo', 'bar']
> >>
> >
> >
> > http://codereview.chromium.org/1794004/show
> >
> 
> 
> 
> -- 
> Eric Li
> 李咏竹
> Google Kirkland
>

[ericli, 2010-04-27 21:42:39]

Yeah, this sounds like a bug inside hromeos-base/autotest ebuild. If you
could figure out and fix it, great. Or I will put it in my TODO queue.

Eric

On Tue, Apr 27, 2010 at 1:52 PM, <petkov@chromium.org> wrote:

> On 2010/04/27 18:06:07, whywhat wrote:
>
>> I tried to pass args separated with commas and split them by comma in
>> third_party/autotest/files/server/autoserv_parser.py - it works. But it
>> would
>> break other uses of autoserv.
>>
>
>  I tried to replace , with space in run_remote_tests.sh and I verified that
>> I
>>
> get
>
>> -a "hello hwqual" in environ['AUTOSERV_ARGS'] in src/scripts/autotest.
>> However
>>
> I
>
>> get only "hello in autoserv. I'm not sure where the transformation takes
>>
> place.
>
>> ebuild-$BOARD is called for autotest with clean, unpack and test.
>>
>
> Look into the chromeos-base/autotest ebuild and see what you pass to
> autoserv
> there?
>
>
>
> http://codereview.chromium.org/1794004/show
>



-- 
Eric Li
李咏竹
Google Kirkland

[ericli, 2010-04-27 21:43:56]

I didn't actively look to port all run_remote_tests.sh functions into
src/script/autotest. I assume you guys still like to stick with it.

Eric.

On Tue, Apr 27, 2010 at 1:54 PM, <petkov@chromium.org> wrote:

> On 2010/04/27 17:28:55, ericli wrote:
>
>> I hate to say that, but this is another good example to use
>> src/script/autotest directly instead of keep adding duplicated args into
>> run_remote_tests.
>>
>
> We should be able to do that once all run_remote_tests functionality is in
> autotest, right? I'm not sure what the status is on that...
>
>
>  Eric
>>
>
>  On Tue, Apr 27, 2010 at 10:25 AM, <mailto:avayvod@chromium.org> wrote:
>>
>
>  >
>> > http://codereview.chromium.org/1794004/diff/1/2
>> > File src/scripts/run_remote_tests.sh (right):
>> >
>> > http://codereview.chromium.org/1794004/diff/1/2#newcode25
>> > src/scripts/run_remote_tests.sh:25: DEFINE_string test_args "" "Command
>> > line arguments for test"
>> > On 2010/04/27 16:45:52, petkov wrote:
>> >
>> >> Let's rename to args -- no reason to be different than autoserv's long
>> >>
>> > name. You
>> >
>> >> could also add the short option "a".
>> >> Also, keep flag definition sorted.
>> >>
>> >
>> >
>> > Done.
>> >
>> >
>> > http://codereview.chromium.org/1794004/diff/1/2#newcode248
>> > src/scripts/run_remote_tests.sh:248: test_args="-a
>> > \"${FLAGS_test_args}\""
>> > This doesn't work. No matter if I escape the space between args or not,
>> > "bar" becomes part of FLAGS_ARGV and considered as a test name. I guess
>> > I'd need to fix FLAGS for that to work. As a workaround one may pass
>> > args comma separated and split them somewhere later on the path down to
>> > the test.
>> >
>> >
>> > On 2010/04/27 16:45:52, petkov wrote:
>> >
>> >> This script runs the "src/scripts/autotest" script which in turns
>> >>
>> > invokes
>> >
>> >> "autoserv". Have you confirmed that passing something like:
>> >>
>> >
>> >  --args="foo bar"
>> >>
>> >
>> >  preserves the quotes down to autoserv, and then you get a two element
>> >>
>> > "args"
>> >
>> >> variable in the control file? I.e., something like:
>> >>
>> >
>> >  args = ['foo', 'bar']
>> >>
>> >
>> >
>> > http://codereview.chromium.org/1794004/show
>> >
>>
>
>
>
>  --
>>
>> Eric Li
>> 李咏竹
>> Google Kirkland
>>
>
>
>
>
> http://codereview.chromium.org/1794004/show
>



-- 
Eric Li
李咏竹
Google Kirkland

[whywhat, 2010-04-28 12:43:50]

Please, take another look.
It works now, with one change in autotest ebuild.
See http://codereview.chromium.org/1787008

[petkov, 2010-04-28 16:57:15]

http://codereview.chromium.org/1794004/diff/12001/8003
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/1794004/diff/12001/8003#newcode17
src/scripts/run_remote_tests.sh:17: DEFINE_string args "" "Command line
arguments for test, separated with comma" a
Any reason for doing comma separated here? It seems from the code that you
should be able to just take a quoted string of space-separated arguments.

I'd rather keep the format between run_remote_tests and autoserv the same --
this way if we get rid of run_remote_tests one day, we won't need to update any
scripts.

http://codereview.chromium.org/1794004/diff/12001/8003#newcode264
src/scripts/run_remote_tests.sh:264: "${args[@]}"
Same concern about passing an empty argument "" down...

[ericli, 2010-04-28 17:03:55]

http://codereview.chromium.org/1794004/diff/12001/8002
File src/scripts/autotest (right):

http://codereview.chromium.org/1794004/diff/12001/8002#newcode195
src/scripts/autotest:195: if '-a' in args:
You should define args into parser.add_options so --args option (-a) could be
accessible from options, and it will be removed from args list for free.

[whywhat, 2010-04-28 17:05:31]

http://codereview.chromium.org/1794004/diff/12001/8003
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/1794004/diff/12001/8003#newcode17
src/scripts/run_remote_tests.sh:17: DEFINE_string args "" "Command line
arguments for test, separated with comma" a
Me to. But if you pass --args "hello world", ${FLAGS_ARGV} contain '"hello' and
'world"' and world" is considered a test target which is not found.

On 2010/04/28 16:57:15, petkov wrote:
> Any reason for doing comma separated here? It seems from the code that you
> should be able to just take a quoted string of space-separated arguments.
> 
> I'd rather keep the format between run_remote_tests and autoserv the same --
> this way if we get rid of run_remote_tests one day, we won't need to update
any
> scripts.
>

http://codereview.chromium.org/1794004/diff/12001/8003#newcode264
src/scripts/run_remote_tests.sh:264: "${args[@]}"
On 2010/04/28 16:57:15, petkov wrote:
> Same concern about passing an empty argument "" down...
> 
> 
I tried running remote tests with no arguments, it went fine.

[petkov, 2010-04-28 17:22:56]

lgtm once ericli lgtm's

On 2010/04/28 17:05:31, whywhat wrote:
> http://codereview.chromium.org/1794004/diff/12001/8003
> File src/scripts/run_remote_tests.sh (right):
> 
> http://codereview.chromium.org/1794004/diff/12001/8003#newcode17
> src/scripts/run_remote_tests.sh:17: DEFINE_string args "" "Command line
> arguments for test, separated with comma" a
> Me to. But if you pass --args "hello world", ${FLAGS_ARGV} contain '"hello'
and
> 'world"' and world" is considered a test target which is not found.

shflags documentation says that this should work but you'd need an enhanced
version of getopt. I guess we don't have that version of getopt?

> 
> On 2010/04/28 16:57:15, petkov wrote:
> > Any reason for doing comma separated here? It seems from the code that you
> > should be able to just take a quoted string of space-separated arguments.
> > 
> > I'd rather keep the format between run_remote_tests and autoserv the same --
> > this way if we get rid of run_remote_tests one day, we won't need to update
> any
> > scripts.
> >
> 
> http://codereview.chromium.org/1794004/diff/12001/8003#newcode264
> src/scripts/run_remote_tests.sh:264: "${args[@]}"
> On 2010/04/28 16:57:15, petkov wrote:
> > Same concern about passing an empty argument "" down...
> > 
> > 
> I tried running remote tests with no arguments, it went fine.

You're getting lucky (in the other CL) because autoserv doesn't expect any
arguments without options (or, if there're options, it ignores it arguments). If
you try running: autoserv "", you'll see that the argument is taken into
account. Probably it doesn't matter in this context...

[whywhat, 2010-04-28 17:45:09]

http://codereview.chromium.org/1794004/diff/12001/8002
File src/scripts/autotest (right):

http://codereview.chromium.org/1794004/diff/12001/8002#newcode195
src/scripts/autotest:195: if '-a' in args:
On 2010/04/28 17:03:55, ericli wrote:
> You should define args into parser.add_options so --args option (-a) could be
> accessible from options, and it will be removed from args list for free.

Done.

http://codereview.chromium.org/1794004/diff/12001/8003
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/1794004/diff/12001/8003#newcode264
src/scripts/run_remote_tests.sh:264: "${args[@]}"
On 2010/04/28 16:57:15, petkov wrote:
> Same concern about passing an empty argument "" down...
> 
> 
As I was explained by local shell scripting guru, this "" syntax is only a
script syntax for not splitting its contents by spaces, "" doesn't get to
command line of the command running.

[ericli, 2010-04-28 17:49:36]

LGTM.

On 2010/04/28 17:45:09, whywhat wrote:
> http://codereview.chromium.org/1794004/diff/12001/8002
> File src/scripts/autotest (right):
> 
> http://codereview.chromium.org/1794004/diff/12001/8002#newcode195
> src/scripts/autotest:195: if '-a' in args:
> On 2010/04/28 17:03:55, ericli wrote:
> > You should define args into parser.add_options so --args option (-a) could
be
> > accessible from options, and it will be removed from args list for free.
> 
> Done.
> 
> http://codereview.chromium.org/1794004/diff/12001/8003
> File src/scripts/run_remote_tests.sh (right):
> 
> http://codereview.chromium.org/1794004/diff/12001/8003#newcode264
> src/scripts/run_remote_tests.sh:264: "${args[@]}"
> On 2010/04/28 16:57:15, petkov wrote:
> > Same concern about passing an empty argument "" down...
> > 
> > 
> As I was explained by local shell scripting guru, this "" syntax is only a
> script syntax for not splitting its contents by spaces, "" doesn't get to
> command line of the command running.