Add new try job test filter specification format to chrome_tests.sh

Add new try job test filter specification format to chrome_tests.sh

BUG=none
TEST=-t with and without :filter works as expected

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

[M-A Ruel, 2010-05-04 00:32:55]

http://codereview.chromium.org/1899001/diff/1/2
File tools/valgrind/chrome_tests.py (right):

http://codereview.chromium.org/1899001/diff/1/2#newcode65
tools/valgrind/chrome_tests.py:65: self._test_list = {
Can you move that at class level? It doesn't need to be a object member.

http://codereview.chromium.org/1899001/diff/1/2#newcode82
tools/valgrind/chrome_tests.py:82: if test.find(':') > 0:
if ':' in test:

http://codereview.chromium.org/1899001/diff/1/2#newcode83
tools/valgrind/chrome_tests.py:83: (self._test, self._gtest_filter) =
test.split(':',1)
... test.split(':', 1)

http://codereview.chromium.org/1899001/diff/1/2#newcode394
tools/valgrind/chrome_tests.py:394: parser.add_option("-t", "--test",
action="append",
default=[],

otherwise it's messy.

http://codereview.chromium.org/1899001/diff/1/2#newcode400
tools/valgrind/chrome_tests.py:400: help="additional arguments to
--gtest_filter")
Shouldn't it be marked as deprecated?

http://codereview.chromium.org/1899001/diff/1/2#newcode423
tools/valgrind/chrome_tests.py:423: if not options.test or not
len(options.test):
if not options.test:

python is that cool.

http://codereview.chromium.org/1899001/diff/1/2#newcode425
tools/valgrind/chrome_tests.py:425: 
If len(options.test) != 1 and options.gtest_filter:
  parser.error("You FAIL")

[Timur Iskhodzhanov, 2010-05-04 05:24:12]

http://codereview.chromium.org/1899001/diff/1/2
File tools/valgrind/chrome_tests.py (right):

http://codereview.chromium.org/1899001/diff/1/2#newcode93
tools/valgrind/chrome_tests.py:93: "and --test %s" % test)
I think the exception string is confusing.
We CAN specify both -t/--test and --gtest_filter with the current version of the
script. I think what you've meant was
"you can't specify both --gtest_filter and --test %s:%s" % (test, gtest_filter)
?

[Timur Iskhodzhanov, 2010-05-04 13:03:01]

Marc-Antoine,
Maybe it makes sense to do the flags re-write on the server side?

On 2010/05/04 05:24:12, Timur Iskhodzhanov wrote:
> http://codereview.chromium.org/1899001/diff/1/2
> File tools/valgrind/chrome_tests.py (right):
> 
> http://codereview.chromium.org/1899001/diff/1/2#newcode93
> tools/valgrind/chrome_tests.py:93: "and --test %s" % test)
> I think the exception string is confusing.
> We CAN specify both -t/--test and --gtest_filter with the current version of
the
> script. I think what you've meant was
> "you can't specify both --gtest_filter and --test %s:%s" % (test,
gtest_filter)
> ?

[M-A Ruel, 2010-05-04 13:07:43]

On 2010/05/04 13:03:01, Timur Iskhodzhanov wrote:
> Marc-Antoine,
> Maybe it makes sense to do the flags re-write on the server side?

Maybe, I'd deprecate --gtest_filter ASAP but before the master is restarted, it
makes sense to keep both in the meantime.

[M-A Ruel, 2010-05-04 13:07:50]

http://codereview.chromium.org/1899001/diff/1/2
File tools/valgrind/chrome_tests.py (right):

http://codereview.chromium.org/1899001/diff/1/2#newcode93
tools/valgrind/chrome_tests.py:93: "and --test %s" % test)
On 2010/05/04 05:24:12, Timur Iskhodzhanov wrote:
> I think the exception string is confusing.
> We CAN specify both -t/--test and --gtest_filter with the current version of
the
> script. I think what you've meant was
> "you can't specify both --gtest_filter and --test %s:%s" % (test,
gtest_filter)
> ?

You meant:
"You can't specify both --gtest_filter %s and --test %s" %
(options.gtest_filter, test)

[Timur Iskhodzhanov, 2010-05-04 13:58:54]

No :-)

Maybe I don't fully understand the CL yet.
Currently, you can run
a) tools/valgrind/chrome_tests.sh -t base
b) tools/valgrind/chrome_tests.sh -t base --gtest_filter="SomeTest.*"

AFAIU, you suggest yet another syntax:
c) tools/valgrind/chrome_tests.sh -t "base:SomeTest.*"

The error message I'm discussing is confusing given the b) usage

On 2010/05/04 13:07:50, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/1899001/diff/1/2
> File tools/valgrind/chrome_tests.py (right):
> 
> http://codereview.chromium.org/1899001/diff/1/2#newcode93
> tools/valgrind/chrome_tests.py:93: "and --test %s" % test)
> On 2010/05/04 05:24:12, Timur Iskhodzhanov wrote:
> > I think the exception string is confusing.
> > We CAN specify both -t/--test and --gtest_filter with the current version of
> the
> > script. I think what you've meant was
> > "you can't specify both --gtest_filter and --test %s:%s" % (test,
> gtest_filter)
> > ?
> 
> You meant:
> "You can't specify both --gtest_filter %s and --test %s" %
> (options.gtest_filter, test)

[Scott Hess - ex-Googler, 2010-05-05 00:18:22]

I'm not qualified to review Python code, so don't rely on my for a LGTM on this
one.

That said, I'm kind of surprised that the response to my concern about
chrome_tests.sh using a different syntax from the new try syntax was to change
chrome_tests.sh.  I'd have expected the reverse (make the new guy match existing
usage).  I can see how specifying multiple --test and --gtest_filter would be
hard in that case - but probably no harder than allowing multiple --test over
here.

-scott


On 2010/05/04 13:58:54, Timur Iskhodzhanov wrote:
> No :-)
> 
> Maybe I don't fully understand the CL yet.
> Currently, you can run
> a) tools/valgrind/chrome_tests.sh -t base
> b) tools/valgrind/chrome_tests.sh -t base --gtest_filter="SomeTest.*"
> 
> AFAIU, you suggest yet another syntax:
> c) tools/valgrind/chrome_tests.sh -t "base:SomeTest.*"
> 
> The error message I'm discussing is confusing given the b) usage
> 
> On 2010/05/04 13:07:50, Marc-Antoine Ruel wrote:
> > http://codereview.chromium.org/1899001/diff/1/2
> > File tools/valgrind/chrome_tests.py (right):
> > 
> > http://codereview.chromium.org/1899001/diff/1/2#newcode93
> > tools/valgrind/chrome_tests.py:93: "and --test %s" % test)
> > On 2010/05/04 05:24:12, Timur Iskhodzhanov wrote:
> > > I think the exception string is confusing.
> > > We CAN specify both -t/--test and --gtest_filter with the current version
of
> > the
> > > script. I think what you've meant was
> > > "you can't specify both --gtest_filter and --test %s:%s" % (test,
> > gtest_filter)
> > > ?
> > 
> > You meant:
> > "You can't specify both --gtest_filter %s and --test %s" %
> > (options.gtest_filter, test)

[M-A Ruel, 2010-05-05 00:42:40]

On 2010/05/05 00:18:22, shess wrote:
> I'm not qualified to review Python code, so don't rely on my for a LGTM on
this
> one.

Me neither. :)

> That said, I'm kind of surprised that the response to my concern about
> chrome_tests.sh using a different syntax from the new try syntax was to change
> chrome_tests.sh.  I'd have expected the reverse (make the new guy match
existing
> usage).  I can see how specifying multiple --test and --gtest_filter would be
> hard in that case - but probably no harder than allowing multiple --test over
> here.
> 
> -scott
> 
> 
> On 2010/05/04 13:58:54, Timur Iskhodzhanov wrote:
> > No :-)
> > 
> > Maybe I don't fully understand the CL yet.
> > Currently, you can run
> > a) tools/valgrind/chrome_tests.sh -t base
> > b) tools/valgrind/chrome_tests.sh -t base --gtest_filter="SomeTest.*"
> > 
> > AFAIU, you suggest yet another syntax:
> > c) tools/valgrind/chrome_tests.sh -t "base:SomeTest.*"
> > 
> > The error message I'm discussing is confusing given the b) usage

All boils down to:

Do you want to be able to run multiple tests in a single script execution? It
not, keep b), it yes, keep c). IMO.

I don't use this script so I don't really care.

M-A

[vandebo (ex-Chrome), 2010-05-05 00:56:39]

> That said, I'm kind of surprised that the response to my concern about
> chrome_tests.sh using a different syntax from the new try syntax was to change

This is why I added you as a reviewer - to ensure this change addresses your
concern.

> chrome_tests.sh.  I'd have expected the reverse (make the new guy match
existing

The old format doesn't nicely lend itself to multiple tests.  You could specify
a --gtest_fitler after each --test argument, but that is much more verbose, as
well as harder to deal with.

> usage).  I can see how specifying multiple --test and --gtest_filter would be
> hard in that case - but probably no harder than allowing multiple --test over
> here.

This test will already handle multiple --test arguments, but it applies the same
--gtest_filter to all of them, and it doesn't handle the case of multiple
--gtest_filter arguments between --test arguments.  However, handling the new
format of -t test:filter is just a matter of parsing the argument further.

http://codereview.chromium.org/1899001/diff/1/2
File tools/valgrind/chrome_tests.py (right):

http://codereview.chromium.org/1899001/diff/1/2#newcode65
tools/valgrind/chrome_tests.py:65: self._test_list = {
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> Can you move that at class level? It doesn't need to be a object member.

Done.

http://codereview.chromium.org/1899001/diff/1/2#newcode82
tools/valgrind/chrome_tests.py:82: if test.find(':') > 0:
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> if ':' in test:

Done.

http://codereview.chromium.org/1899001/diff/1/2#newcode83
tools/valgrind/chrome_tests.py:83: (self._test, self._gtest_filter) =
test.split(':',1)
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> ... test.split(':', 1)

Done.

http://codereview.chromium.org/1899001/diff/1/2#newcode93
tools/valgrind/chrome_tests.py:93: "and --test %s" % test)
On 2010/05/04 05:24:12, Timur Iskhodzhanov wrote:
> I think the exception string is confusing.
> We CAN specify both -t/--test and --gtest_filter with the current version of
the
> script. I think what you've meant was
> "you can't specify both --gtest_filter and --test %s:%s" % (test,
gtest_filter)
> ?
>
> Maybe I don't fully understand the CL yet.
> Currently, you can run
> a) tools/valgrind/chrome_tests.sh -t base
> b) tools/valgrind/chrome_tests.sh -t base --gtest_filter="SomeTest.*"
> 
> AFAIU, you suggest yet another syntax:
> c) tools/valgrind/chrome_tests.sh -t "base:SomeTest.*"
> 
> The error message I'm discussing is confusing given the b) > usage

You are correct that I am adding the c case.  However, this error will only be
reported in the --gtest_filter=SomeTest.* -t base:SomeOtherTest.* case, not in
the b case.  Also, as a side effect of how I did this test, doing both -t and
--gtest_filter with the same filter will work without complaint.

http://codereview.chromium.org/1899001/diff/1/2#newcode394
tools/valgrind/chrome_tests.py:394: parser.add_option("-t", "--test",
action="append",
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> default=[],
> 
> otherwise it's messy.

Done.

http://codereview.chromium.org/1899001/diff/1/2#newcode400
tools/valgrind/chrome_tests.py:400: help="additional arguments to
--gtest_filter")
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> Shouldn't it be marked as deprecated?

The intent of this change is not to necessarily change anyone's workflow, but to
accept consistent syntax on this tool and the recent try job change.

I can deprecate this flag if you think that's the right thing to do.

http://codereview.chromium.org/1899001/diff/1/2#newcode423
tools/valgrind/chrome_tests.py:423: if not options.test or not
len(options.test):
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> if not options.test:
> 
> python is that cool.

Done.

http://codereview.chromium.org/1899001/diff/1/2#newcode425
tools/valgrind/chrome_tests.py:425: 
On 2010/05/04 00:32:56, Marc-Antoine Ruel wrote:
> If len(options.test) != 1 and options.gtest_filter:
>   parser.error("You FAIL")
> 

Done.

[Scott Hess - ex-Googler, 2010-05-05 01:07:22]

On 2010/05/05 00:56:39, vandebo wrote:
> > That said, I'm kind of surprised that the response to my concern about
> > chrome_tests.sh using a different syntax from the new try syntax was to
change
> 
> This is why I added you as a reviewer - to ensure this change addresses your
> concern.

Well, I thought the chrome_tests.sh syntax was gnarly, but didn't want to open
that can.

> > chrome_tests.sh.  I'd have expected the reverse (make the new guy match
> existing
> 
> The old format doesn't nicely lend itself to multiple tests.  You could
specify
> a --gtest_fitler after each --test argument, but that is much more verbose, as
> well as harder to deal with.

A single --gtest_filter applied to all tests should work just fine.  If anyone
is writing ui_tests and unit_tests with the same names, they should probably be
shot anyhow.

BTW, I'm just being pissy.  If the gate-keepers for chrome_tests.sh are happy
with the change, I'm not going to complain!

[Timur Iskhodzhanov, 2010-05-05 08:37:08]

On 2010/05/05 00:56:39, vandebo wrote:
> > Maybe I don't fully understand the CL yet.
> > Currently, you can run
> > a) tools/valgrind/chrome_tests.sh -t base
> > b) tools/valgrind/chrome_tests.sh -t base --gtest_filter="SomeTest.*"
> > 
> > AFAIU, you suggest yet another syntax:
> > c) tools/valgrind/chrome_tests.sh -t "base:SomeTest.*"
> > 
> > The error message I'm discussing is confusing given the b) > usage
> 
> You are correct that I am adding the c case.  However, this error will only be
> reported in the --gtest_filter=SomeTest.* -t base:SomeOtherTest.* case, not in
> the b case.  Also, as a side effect of how I did this test, doing both -t and
> --gtest_filter with the same filter will work without complaint.
Ah now I see what was confusing me!
...("Can not specify both --gtest_filter and --test %s" % test)
--> test is "base:SomeOtherTest.*" in this case.

LGTM, provided that
1. linux_valgrind, linux_tsan, mac_valgrind trybots don't complain
2. The trybots actually support the multiple target format
3. b) still works

[M-A Ruel, 2010-05-06 17:11:59]

lgtm