GTTF: Add test launcher developer mode for local debugging.

GTTF: Add test launcher developer mode for local debugging.

BUG=236893, 312984
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233667

[Nico, 2013-11-06 00:47:00]

I don't think this addresses "The default behaviour for tests should be to not
suppress stdout/stderr outputs. The buildbots/trybots can use some flag to
suppress the logs if desired (although ideally the tests would be fixed to not
spew as much log)." from the duped log.

stdout shouldn't be suppressed by default. That'll lead to people adding even
more logging (chrome already logs too much, which is a cost users pay, in binary
size, and log output). When we replaced ninja with make, lots of warnings got
fixed since ninja produced so much less output that the warnings got actively
annoying. Since the new test launcher also produces less output, I'd expect that
log output from tests goes down over time as well.

[Paweł Hajdan Jr., 2013-11-06 00:56:47]

On 2013/11/06 00:47:00, Nico wrote:
> I don't think this addresses "The default behaviour for tests should be to not
> suppress stdout/stderr outputs. The buildbots/trybots can use some flag to
> suppress the logs if desired (although ideally the tests would be fixed to not
> spew as much log)." from the duped log.

If you have more questions or comments I'd rather move this to chromium-dev.
IMHO the default behavior should be to run the whole test suite as fast and
stable as possible. This means parallelism (which means redirecting stdio) and
retries. When debugging a specific subset of tests the developer is already
passing --gtest_filter, so it shouldn't hurt (and I'm developer myself and I'm
working regularly with these tests) to also add --test-launcher-developer-mode .

> stdout shouldn't be suppressed by default. That'll lead to people adding even
> more logging (chrome already logs too much, which is a cost users pay, in
binary
> size, and log output). When we replaced ninja with make, lots of warnings got
> fixed since ninja produced so much less output that the warnings got actively
> annoying. Since the new test launcher also produces less output, I'd expect
that
> log output from tests goes down over time as well.

I don't remember any comments about this when run_test_cases.py was introduced
on the bots. While a successful compiler invocation doesn't output anything, a
successful test run outputs at least 2 lines for a unit test, and ~10+ lines for
a browser tests. This is how gtest works - specific ideas how to address it are
welcome, I will listen. Please move that discussion to chromium-dev though.
Also, remember to be specific and constructive - I pointed out some differences
which don't make it as easy as it might seem.

[Nico, 2013-11-06 01:02:24]

On Tue, Nov 5, 2013 at 4:56 PM, <phajdan.jr@chromium.org> wrote:

> On 2013/11/06 00:47:00, Nico wrote:
>
>> I don't think this addresses "The default behaviour for tests should be
>> to not
>> suppress stdout/stderr outputs. The buildbots/trybots can use some flag to
>> suppress the logs if desired (although ideally the tests would be fixed
>> to not
>> spew as much log)." from the duped log.
>>
>
> If you have more questions or comments I'd rather move this to
> chromium-dev.
> IMHO the default behavior should be to run the whole test suite as fast and
> stable as possible. This means parallelism (which means redirecting stdio)


Sure, buffering makes sense. It doesn't have to be omitted though.


> and
> retries. When debugging a specific subset of tests the developer is already
> passing --gtest_filter, so it shouldn't hurt (and I'm developer myself and
> I'm
> working regularly with these tests) to also add
> --test-launcher-developer-mode .
>
>
>  stdout shouldn't be suppressed by default. That'll lead to people adding
>> even
>> more logging (chrome already logs too much, which is a cost users pay, in
>>
> binary
>
>> size, and log output). When we replaced ninja with make, lots of warnings
>> got
>> fixed since ninja produced so much less output that the warnings got
>> actively
>> annoying. Since the new test launcher also produces less output, I'd
>> expect
>>
> that
>
>> log output from tests goes down over time as well.
>>
>
> I don't remember any comments about this when run_test_cases.py was
> introduced
> on the bots. While a successful compiler invocation doesn't output
> anything, a
> successful test run outputs at least 2 lines for a unit test, and ~10+
> lines for
> a browser tests. This is how gtest works - specific ideas how to address
> it are
> welcome, I will listen.


You can remove the 2 lines for a passing test by giving gtest a
specialized testing::EmptyTestEventListener. ninja's unit tests output just
a single line of text, with overprinting (unless a test fails, in which
case the failing output is printed). You can look at
https://github.com/nico/ninja/commit/94f999b681ea4ced1cc27b29e0db77d72554ecf9for
the basic idea (and
https://github.com/nico/ninja/blob/master/src/ninja_test.cc for how that
code looks now).


> Please move that discussion to chromium-dev though.
> Also, remember to be specific and constructive - I pointed out some
> differences
> which don't make it as easy as it might seem.
>
> https://chromiumcodereview.appspot.com/61363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-11-06 14:17:17]

https://codereview.chromium.org/61363002/diff/1/base/test/test_switches.cc
File base/test/test_switches.cc (right):

https://codereview.chromium.org/61363002/diff/1/base/test/test_switches.cc#ne...
base/test/test_switches.cc:17: "test-launcher-developer-mode";
Is there a reason you don't want to make this implied by the existence of a
filter?

[Paweł Hajdan Jr., 2013-11-06 15:55:21]

https://codereview.chromium.org/61363002/diff/1/base/test/test_switches.cc
File base/test/test_switches.cc (right):

https://codereview.chromium.org/61363002/diff/1/base/test/test_switches.cc#ne...
base/test/test_switches.cc:17: "test-launcher-developer-mode";
On 2013/11/06 14:17:18, sky wrote:
> Is there a reason you don't want to make this implied by the existence of a
> filter?

Some bots use a filter, e.g. "manual" webrtc tests on chromium.webrtc waterfall.

[sadrul, 2013-11-06 17:39:55]

On 2013/11/06 00:56:47, Paweł Hajdan Jr. wrote:
> On 2013/11/06 00:47:00, Nico wrote:
> > I don't think this addresses "The default behaviour for tests should be to
not
> > suppress stdout/stderr outputs. The buildbots/trybots can use some flag to
> > suppress the logs if desired (although ideally the tests would be fixed to
not
> > spew as much log)." from the duped log.
> 
> If you have more questions or comments I'd rather move this to chromium-dev.
> IMHO the default behavior should be to run the whole test suite as fast and
> stable as possible. This means parallelism (which means redirecting stdio) and
> retries. When debugging a specific subset of tests the developer is already
> passing --gtest_filter, so it shouldn't hurt (and I'm developer myself and I'm
> working regularly with these tests) to also add --test-launcher-developer-mode
.

It would hurt the bots even less to add a custom flag to suppress the
stdout/stderr. We should not require developers to remember all the flags they
need to use. The bots can remember that with much more ease.

[Paweł Hajdan Jr., 2013-11-06 19:47:19]

On 2013/11/06 17:39:55, sadrul wrote:
> It would hurt the bots even less to add a custom flag to suppress the
> stdout/stderr. We should not require developers to remember all the flags they
> need to use. The bots can remember that with much more ease.

Sadrul, I'm happy to continue the discussion off-Rietveld. Short response for
now: arguably there are cases when running locally you want to run the tests as
fast as possible (and with retries for stability). When you are debugging,
passing one flag is reasonable IMHO. Assumption that developers are always
debugging is just not true I think, unless there is evidence (can be just many
people agreeing with that). Feel free to do a poll or something - for now I'm
just trying to add an option that does address the issue in some (maybe
imperfect) way.

[sky, 2013-11-06 21:28:06]

Can you start the discussion on chromium-dev. Both Nico and Sadrul
have expressed concern that this makes developers lives more
complicated, which we don't want.

  -Scott

On Wed, Nov 6, 2013 at 11:47 AM,  <phajdan.jr@chromium.org> wrote:
> On 2013/11/06 17:39:55, sadrul wrote:
>>
>> It would hurt the bots even less to add a custom flag to suppress the
>> stdout/stderr. We should not require developers to remember all the flags
>> they
>> need to use. The bots can remember that with much more ease.
>
>
> Sadrul, I'm happy to continue the discussion off-Rietveld. Short response
> for
> now: arguably there are cases when running locally you want to run the tests
> as
> fast as possible (and with retries for stability). When you are debugging,
> passing one flag is reasonable IMHO. Assumption that developers are always
> debugging is just not true I think, unless there is evidence (can be just
> many
> people agreeing with that). Feel free to do a poll or something - for now
> I'm
> just trying to add an option that does address the issue in some (maybe
> imperfect) way.
>
> https://codereview.chromium.org/61363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Paweł Hajdan Jr., 2013-11-07 03:00:48]

On 2013/11/06 21:28:06, sky wrote:
> Can you start the discussion on chromium-dev. Both Nico and Sadrul
> have expressed concern that this makes developers lives more
> complicated, which we don't want.

The discussion is
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Mw1hrGwnUpw/sTpXO...

Note that with this CL it's at least an option to pass the flag and get the
desired behavior. More bikeshedding on this will IMHO just delay availability of
the fix.

I'd _very_ much like to avoid having serialized unbuffered test run to be the
default. It's several times slower on developer workstations. I don't see a
decisive majority in favor of going that way. I also presented some arguments in
this review pointing out why I think an explicit --test-launcher-developer-mode
is the best solution for this (while being open to constructive alternatives
addressing the problems I pointed out).

Let's move further discussion to above chromium-dev thread.

[sky, 2013-11-07 16:55:54]

LGTM

[Paweł Hajdan Jr., 2013-11-07 19:53:41]

Committed patchset #1 manually as r233667 (presubmit successful).