Silence testserver logs when tests are run in non-verbose mode

Silence testserver logs when tests are run in non-verbose mode

The console output from testserver.py, chromiumsync.py and xmppserver.py during sync integration test runs is pretty chatty, and could do with being disabled by default. In addition, testserver.py spits out logs for non-sync tests even when the tests are run in non-verbose mode.

This patch adds a switch to testserver.py called "log-to-console", and ties it up to the logging level returned by logging::GetMinLogLevel(). This way, testserver logs are printed on the console (in addition to being written to testserver.log) when tests are run in verbose mode. However, when tests are run in non-verbose mode, testserver logs are only written to the log file, but not to the console.

BUG=71241
TEST=sync_integration_tests

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

[Raghu Simha, 2011-01-29 00:29:58]

Fred, I've taken a first stab at making the integration test logss less chatty.
Let me know what you think of this approach. Thanks.

[Paweł Hajdan Jr., 2011-01-29 10:26:24]

http://codereview.chromium.org/6404003/diff/1/net/test/test_server.h
File net/test/test_server.h (right):

http://codereview.chromium.org/6404003/diff/1/net/test/test_server.h#newcode119
net/test/test_server.h:119: void SetConsoleLogging(bool log_to_console) {
Please don't add new setters to this class. Can the logging setting be
determined at construction time?

[Raghu Simha, 2011-01-29 10:50:38]

http://codereview.chromium.org/6404003/diff/1/net/test/test_server.h
File net/test/test_server.h (right):

http://codereview.chromium.org/6404003/diff/1/net/test/test_server.h#newcode119
net/test/test_server.h:119: void SetConsoleLogging(bool log_to_console) {
On 2011/01/29 10:26:24, Paweł Hajdan Jr. wrote:
> Please don't add new setters to this class. Can the logging setting be
> determined at construction time?

It certainly can. The reason I added this setter was to avoid having to change
every instance where a TestServer is constructed. If you're fine with that, I'll
be happy to make the change and get rid of this setter.

Also, assuming we go that way, what value of log_to_console should consumers of
TestServer use? (The current behavior is equivalent to a value of true, i.e.,
logs are printed on the console)

[Paweł Hajdan Jr., 2011-01-29 11:27:03]

The best option seems to use the TYPE_SYNC ctor argument, and append your
command-line flag in TestServer::AddCommandLineArguments. How does that
sound to you?

On Sat, Jan 29, 2011 at 11:50, <rsimha@chromium.org> wrote:

>
> http://codereview.chromium.org/6404003/diff/1/net/test/test_server.h
> File net/test/test_server.h (right):
>
>
>
http://codereview.chromium.org/6404003/diff/1/net/test/test_server.h#newcode119
> net/test/test_server.h:119: void SetConsoleLogging(bool log_to_console)
> {
> On 2011/01/29 10:26:24, Paweł Hajdan Jr. wrote:
>
>> Please don't add new setters to this class. Can the logging setting be
>> determined at construction time?
>>
>
> It certainly can. The reason I added this setter was to avoid having to
> change every instance where a TestServer is constructed. If you're fine
> with that, I'll be happy to make the change and get rid of this setter.
>
> Also, assuming we go that way, what value of log_to_console should
> consumers of TestServer use? (The current behavior is equivalent to a
> value of true, i.e., logs are printed on the console)
>
>
> http://codereview.chromium.org/6404003/
>

[Raghu Simha, 2011-01-31 19:30:04]

On 2011/01/29 11:27:03, Paweł Hajdan Jr. wrote:
> The best option seems to use the TYPE_SYNC ctor argument, and append your
> command-line flag in TestServer::AddCommandLineArguments. How does that
> sound to you?

I just realised that we don't need to maintain a separate flag or add a setter
to class TestServer. Tying up the logging level of the testserver to
logging::GetMinLogLevel() gives us the behavior we want for free. And what's
better, it doesn't have to be sync specific anymore.

I've uploaded a new patch. Let me know what you think.

[Paweł Hajdan Jr., 2011-01-31 20:43:54]

Will that turn off logs for all tests?

If non-sync tests still print the logs to stdout, LGTM. Otherwise, please apply
the logic only for TYPE_SYNC and wait for another review.

[Raghu Simha, 2011-01-31 21:21:47]

On 2011/01/31 20:43:54, Paweł Hajdan Jr. wrote:
> Will that turn off logs for all tests?
> 
> If non-sync tests still print the logs to stdout, LGTM. Otherwise, please
apply
> the logic only for TYPE_SYNC and wait for another review.

Is there a reason you want to special-case TYPE_SYNC here? Isn't it a good thing
for testserver.py's logs to appear on the console when chromium logs are
enabled, and to be silenced when chromium logs are disabled?

Here's how this patch works:
If any test that uses the test server were to run in verbose mode, the test
server logs will be printed to the console. However, if the test is run in
non-verbose mode, testserver logs will be written to testserver.log, but not
printed to the console. See
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/10518/st....

In the past:
If a test that uses the test server is run in non-verbose mode, then the only
logs that are seen on the console are the ones from testserver.py. See
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/10536/st....

[Paweł Hajdan Jr., 2011-02-01 07:49:59]

On 2011/01/31 21:21:47, rsimha wrote:
> Is there a reason you want to special-case TYPE_SYNC here? Isn't it a good
thing
> for testserver.py's logs to appear on the console when chromium logs are
> enabled, and to be silenced when chromium logs are disabled?

I remember various issues and inconsistencies with chromium logging (e.g. on
Windows levels below ERROR used to be only printed to file and not console,
etc). The testserver logs may be useful to eradicate test server flakiness, and
I want to be sure they are available when they are needed.

If you want to have a different behavior for sync tests, I'm fine with that
(they make a lot more requests so that's indeed noisy), hence the special case.

> Here's how this patch works:

Looking at those links it seems both print testserver output, which is good, but
it might change in the future.

[akalin, 2011-02-01 21:31:38]

LGTM

On 2011/02/01 07:49:59, Paweł Hajdan Jr. wrote:
> On 2011/01/31 21:21:47, rsimha wrote:
> > Is there a reason you want to special-case TYPE_SYNC here? Isn't it a good
> thing
> > for testserver.py's logs to appear on the console when chromium logs are
> > enabled, and to be silenced when chromium logs are disabled?
> 
> I remember various issues and inconsistencies with chromium logging (e.g. on
> Windows levels below ERROR used to be only printed to file and not console,
> etc). The testserver logs may be useful to eradicate test server flakiness,
and
> I want to be sure they are available when they are needed.
> 
> If you want to have a different behavior for sync tests, I'm fine with that
> (they make a lot more requests so that's indeed noisy), hence the special
case.
> 
> > Here's how this patch works:
> 
> Looking at those links it seems both print testserver output, which is good,
but
> it might change in the future.