Mojo: Suppress validation error logging in some tests

Mojo: Suppress validation error logging in some tests

These errors are useful for developers when developers make mistakes,
but they make a mess of test logs. Disable them in tests which are
particularly noisy.

BUG=659195
Committed: https://crrev.com/dafb77a4efa4ea0e6d2080ea239f401ff1c4a691
Cr-Commit-Position: refs/heads/master@{#429393}

[Ken Rockot(use gerrit already), 2016-10-25 17:21:49]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2016-10-25 17:21:55]

rockot@chromium.org changed reviewers:
+ jam@chromium.org

[commit-bot: I haz the power, 2016-10-25 17:22:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 17:26:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 17:26:39]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2016-10-25 17:28:22]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 17:29:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-10-25 17:34:12]

I don't have a strong opinion but just want to let you know that it did
help me before: when my CL introduced validation bugs, unittests spit out
validation error log that helped me to immediately realize what I have done
wrong. (But maybe that means I didn't write enough validation test cases,
rather than we should log during testing.)

On Wed, Oct 26, 2016 at 1:21 AM, <rockot@chromium.org> wrote:

> Reviewers: jam
> CL: https://codereview.chromium.org/2448203002/
>
> Description:
> Mojo: Suppress validation error logging in tests
>
> These errors are useful for developers when developers make mistakes,
> but they make a mess of test logs. Disable them in tests.
>
> BUG=659195
>
> Affected files (+16, -3 lines):
> M mojo/edk/test/run_all_unittests.cc
> M mojo/public/cpp/bindings/lib/validation_errors.h
> M mojo/public/cpp/bindings/lib/validation_errors.cc
>
>
> Index: mojo/edk/test/run_all_unittests.cc
> diff --git a/mojo/edk/test/run_all_unittests.cc b/mojo/edk/test/run_all_
> unittests.cc
> index 05aed913a4868f2fa2ae443e1c9098a44ab23ade..
> e5802ecb125d8e3f013dad8700324c96fecc062f 100644
> --- a/mojo/edk/test/run_all_unittests.cc
> +++ b/mojo/edk/test/run_all_unittests.cc
> @@ -14,6 +14,7 @@
> #include "mojo/edk/test/multiprocess_test_helper.h"
> #include "mojo/edk/test/scoped_ipc_support.h"
> #include "mojo/edk/test/test_support_impl.h"
> +#include "mojo/public/cpp/bindings/lib/validation_errors.h"
> #include "mojo/public/tests/test_support_private.h"
> #include "testing/gtest/include/gtest/gtest.h"
>
> @@ -48,6 +49,7 @@ int main(int argc, char** argv) {
> // loop, which is destructed inside base::LaunchUnitTests.
> new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
>
> + mojo::internal::SuppressValidationErrorLoggingForTests(true);
> return base::LaunchUnitTests(
> argc, argv,
> base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
> Index: mojo/public/cpp/bindings/lib/validation_errors.cc
> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.cc
> b/mojo/public/cpp/bindings/lib/validation_errors.cc
> index 67106a0fd508dfe2e74a960564319edce92f8a5a..
> d7107714227504f2b85a791859baed10e9e20011 100644
> --- a/mojo/public/cpp/bindings/lib/validation_errors.cc
> +++ b/mojo/public/cpp/bindings/lib/validation_errors.cc
> @@ -14,6 +14,7 @@ namespace {
> ValidationErrorObserverForTesting* g_validation_error_observer = nullptr;
> SerializationWarningObserverForTesting* g_serialization_warning_observer =
> nullptr;
> +bool g_suppress_logging = false;
>
> } // namespace
>
> @@ -71,8 +72,10 @@ void ReportValidationError(ValidationContext* context,
> }
>
> if (description) {
> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error) << "
> ("
> - << description << ")";
> + if (!g_suppress_logging) {
> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
> + << " (" << description << ")";
> + }
> if (context->message()) {
> context->message()->NotifyBadMessage(
> base::StringPrintf("Validation failed for %s [%s (%s)]",
> @@ -80,7 +83,8 @@ void ReportValidationError(ValidationContext* context,
> ValidationErrorToString(error), description));
> }
> } else {
> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
> + if (!g_suppress_logging)
> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
> if (context->message()) {
> context->message()->NotifyBadMessage(
> base::StringPrintf("Validation failed for %s [%s]",
> @@ -101,6 +105,10 @@ void ReportValidationErrorForMessage(
> ReportValidationError(&validation_context, error);
> }
>
> +void SuppressValidationErrorLoggingForTests(bool suppress) {
> + g_suppress_logging = suppress;
> +}
> +
> ValidationErrorObserverForTesting::ValidationErrorObserverForTesting(
> const base::Closure& callback)
> : last_error_(VALIDATION_ERROR_NONE), callback_(callback) {
> Index: mojo/public/cpp/bindings/lib/validation_errors.h
> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h
> b/mojo/public/cpp/bindings/lib/validation_errors.h
> index 7636e391dad4414dfff7659a04e75c12e9dd1ff4..
> 496c5dd41cf983067b1d614e641944b3a66e4d90 100644
> --- a/mojo/public/cpp/bindings/lib/validation_errors.h
> +++ b/mojo/public/cpp/bindings/lib/validation_errors.h
> @@ -89,6 +89,9 @@ MOJO_CPP_BINDINGS_EXPORT void
> ReportValidationErrorForMessage(
> ValidationError error,
> const char* description = nullptr);
>
> +MOJO_CPP_BINDINGS_EXPORT void SuppressValidationErrorLoggingForTests(
> + bool suppress);
> +
> // Only used by validation tests and when there is only one thread doing
> message
> // validation.
> class MOJO_CPP_BINDINGS_EXPORT ValidationErrorObserverForTesting {
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-25 17:35:30]

Maybe it would be better if we introduced a flag for this and added it to
bot configs?

On Tue, Oct 25, 2016 at 10:33 AM, Yuzhu Shen <yzshen@chromium.org> wrote:

> I don't have a strong opinion but just want to let you know that it did
> help me before: when my CL introduced validation bugs, unittests spit out
> validation error log that helped me to immediately realize what I have done
> wrong. (But maybe that means I didn't write enough validation test cases,
> rather than we should log during testing.)
>
> On Wed, Oct 26, 2016 at 1:21 AM, <rockot@chromium.org> wrote:
>
>> Reviewers: jam
>> CL: https://codereview.chromium.org/2448203002/
>>
>> Description:
>> Mojo: Suppress validation error logging in tests
>>
>> These errors are useful for developers when developers make mistakes,
>> but they make a mess of test logs. Disable them in tests.
>>
>> BUG=659195
>>
>> Affected files (+16, -3 lines):
>> M mojo/edk/test/run_all_unittests.cc
>> M mojo/public/cpp/bindings/lib/validation_errors.h
>> M mojo/public/cpp/bindings/lib/validation_errors.cc
>>
>>
>> Index: mojo/edk/test/run_all_unittests.cc
>> diff --git a/mojo/edk/test/run_all_unittests.cc
>> b/mojo/edk/test/run_all_unittests.cc
>> index
05aed913a4868f2fa2ae443e1c9098a44ab23ade..e5802ecb125d8e3f013dad8700324c96fecc062f
>> 100644
>> --- a/mojo/edk/test/run_all_unittests.cc
>> +++ b/mojo/edk/test/run_all_unittests.cc
>> @@ -14,6 +14,7 @@
>> #include "mojo/edk/test/multiprocess_test_helper.h"
>> #include "mojo/edk/test/scoped_ipc_support.h"
>> #include "mojo/edk/test/test_support_impl.h"
>> +#include "mojo/public/cpp/bindings/lib/validation_errors.h"
>> #include "mojo/public/tests/test_support_private.h"
>> #include "testing/gtest/include/gtest/gtest.h"
>>
>> @@ -48,6 +49,7 @@ int main(int argc, char** argv) {
>> // loop, which is destructed inside base::LaunchUnitTests.
>> new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
>>
>> + mojo::internal::SuppressValidationErrorLoggingForTests(true);
>> return base::LaunchUnitTests(
>> argc, argv,
>> base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
>> Index: mojo/public/cpp/bindings/lib/validation_errors.cc
>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.cc
>> b/mojo/public/cpp/bindings/lib/validation_errors.cc
>> index
67106a0fd508dfe2e74a960564319edce92f8a5a..d7107714227504f2b85a791859baed10e9e20011
>> 100644
>> --- a/mojo/public/cpp/bindings/lib/validation_errors.cc
>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.cc
>> @@ -14,6 +14,7 @@ namespace {
>> ValidationErrorObserverForTesting* g_validation_error_observer = nullptr;
>> SerializationWarningObserverForTesting* g_serialization_warning_observer
>> =
>> nullptr;
>> +bool g_suppress_logging = false;
>>
>> } // namespace
>>
>> @@ -71,8 +72,10 @@ void ReportValidationError(ValidationContext* context,
>> }
>>
>> if (description) {
>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error) <<
>> " ("
>> - << description << ")";
>> + if (!g_suppress_logging) {
>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
>> + << " (" << description << ")";
>> + }
>> if (context->message()) {
>> context->message()->NotifyBadMessage(
>> base::StringPrintf("Validation failed for %s [%s (%s)]",
>> @@ -80,7 +83,8 @@ void ReportValidationError(ValidationContext* context,
>> ValidationErrorToString(error), description));
>> }
>> } else {
>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>> + if (!g_suppress_logging)
>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>> if (context->message()) {
>> context->message()->NotifyBadMessage(
>> base::StringPrintf("Validation failed for %s [%s]",
>> @@ -101,6 +105,10 @@ void ReportValidationErrorForMessage(
>> ReportValidationError(&validation_context, error);
>> }
>>
>> +void SuppressValidationErrorLoggingForTests(bool suppress) {
>> + g_suppress_logging = suppress;
>> +}
>> +
>> ValidationErrorObserverForTesting::ValidationErrorObserverForTesting(
>> const base::Closure& callback)
>> : last_error_(VALIDATION_ERROR_NONE), callback_(callback) {
>> Index: mojo/public/cpp/bindings/lib/validation_errors.h
>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h
>> b/mojo/public/cpp/bindings/lib/validation_errors.h
>> index
7636e391dad4414dfff7659a04e75c12e9dd1ff4..496c5dd41cf983067b1d614e641944b3a66e4d90
>> 100644
>> --- a/mojo/public/cpp/bindings/lib/validation_errors.h
>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.h
>> @@ -89,6 +89,9 @@ MOJO_CPP_BINDINGS_EXPORT void
>> ReportValidationErrorForMessage(
>> ValidationError error,
>> const char* description = nullptr);
>>
>> +MOJO_CPP_BINDINGS_EXPORT void SuppressValidationErrorLoggingForTests(
>> + bool suppress);
>> +
>> // Only used by validation tests and when there is only one thread doing
>> message
>> // validation.
>> class MOJO_CPP_BINDINGS_EXPORT ValidationErrorObserverForTesting {
>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-25 17:36:34]

In fact it would be nice if there were a log level that were suitable for
this use case: on by default for developers, not on by default for bot
configurations.

On Tue, Oct 25, 2016 at 10:35 AM, Ken Rockot <rockot@chromium.org> wrote:

> Maybe it would be better if we introduced a flag for this and added it to
> bot configs?
>
> On Tue, Oct 25, 2016 at 10:33 AM, Yuzhu Shen <yzshen@chromium.org> wrote:
>
>> I don't have a strong opinion but just want to let you know that it did
>> help me before: when my CL introduced validation bugs, unittests spit out
>> validation error log that helped me to immediately realize what I have done
>> wrong. (But maybe that means I didn't write enough validation test cases,
>> rather than we should log during testing.)
>>
>> On Wed, Oct 26, 2016 at 1:21 AM, <rockot@chromium.org> wrote:
>>
>>> Reviewers: jam
>>> CL: https://codereview.chromium.org/2448203002/
>>>
>>> Description:
>>> Mojo: Suppress validation error logging in tests
>>>
>>> These errors are useful for developers when developers make mistakes,
>>> but they make a mess of test logs. Disable them in tests.
>>>
>>> BUG=659195
>>>
>>> Affected files (+16, -3 lines):
>>> M mojo/edk/test/run_all_unittests.cc
>>> M mojo/public/cpp/bindings/lib/validation_errors.h
>>> M mojo/public/cpp/bindings/lib/validation_errors.cc
>>>
>>>
>>> Index: mojo/edk/test/run_all_unittests.cc
>>> diff --git a/mojo/edk/test/run_all_unittests.cc
>>> b/mojo/edk/test/run_all_unittests.cc
>>> index
05aed913a4868f2fa2ae443e1c9098a44ab23ade..e5802ecb125d8e3f013dad8700324c96fecc062f
>>> 100644
>>> --- a/mojo/edk/test/run_all_unittests.cc
>>> +++ b/mojo/edk/test/run_all_unittests.cc
>>> @@ -14,6 +14,7 @@
>>> #include "mojo/edk/test/multiprocess_test_helper.h"
>>> #include "mojo/edk/test/scoped_ipc_support.h"
>>> #include "mojo/edk/test/test_support_impl.h"
>>> +#include "mojo/public/cpp/bindings/lib/validation_errors.h"
>>> #include "mojo/public/tests/test_support_private.h"
>>> #include "testing/gtest/include/gtest/gtest.h"
>>>
>>> @@ -48,6 +49,7 @@ int main(int argc, char** argv) {
>>> // loop, which is destructed inside base::LaunchUnitTests.
>>> new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
>>>
>>> + mojo::internal::SuppressValidationErrorLoggingForTests(true);
>>> return base::LaunchUnitTests(
>>> argc, argv,
>>> base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
>>> Index: mojo/public/cpp/bindings/lib/validation_errors.cc
>>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> b/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> index
67106a0fd508dfe2e74a960564319edce92f8a5a..d7107714227504f2b85a791859baed10e9e20011
>>> 100644
>>> --- a/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> @@ -14,6 +14,7 @@ namespace {
>>> ValidationErrorObserverForTesting* g_validation_error_observer =
>>> nullptr;
>>> SerializationWarningObserverForTesting* g_serialization_warning_observer
>>> =
>>> nullptr;
>>> +bool g_suppress_logging = false;
>>>
>>> } // namespace
>>>
>>> @@ -71,8 +72,10 @@ void ReportValidationError(ValidationContext*
>>> context,
>>> }
>>>
>>> if (description) {
>>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error) <<
>>> " ("
>>> - << description << ")";
>>> + if (!g_suppress_logging) {
>>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
>>> + << " (" << description << ")";
>>> + }
>>> if (context->message()) {
>>> context->message()->NotifyBadMessage(
>>> base::StringPrintf("Validation failed for %s [%s (%s)]",
>>> @@ -80,7 +83,8 @@ void ReportValidationError(ValidationContext* context,
>>> ValidationErrorToString(error), description));
>>> }
>>> } else {
>>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>>> + if (!g_suppress_logging)
>>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>>> if (context->message()) {
>>> context->message()->NotifyBadMessage(
>>> base::StringPrintf("Validation failed for %s [%s]",
>>> @@ -101,6 +105,10 @@ void ReportValidationErrorForMessage(
>>> ReportValidationError(&validation_context, error);
>>> }
>>>
>>> +void SuppressValidationErrorLoggingForTests(bool suppress) {
>>> + g_suppress_logging = suppress;
>>> +}
>>> +
>>> ValidationErrorObserverForTesting::ValidationErrorObserverForTesting(
>>> const base::Closure& callback)
>>> : last_error_(VALIDATION_ERROR_NONE), callback_(callback) {
>>> Index: mojo/public/cpp/bindings/lib/validation_errors.h
>>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h
>>> b/mojo/public/cpp/bindings/lib/validation_errors.h
>>> index
7636e391dad4414dfff7659a04e75c12e9dd1ff4..496c5dd41cf983067b1d614e641944b3a66e4d90
>>> 100644
>>> --- a/mojo/public/cpp/bindings/lib/validation_errors.h
>>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.h
>>> @@ -89,6 +89,9 @@ MOJO_CPP_BINDINGS_EXPORT void
>>> ReportValidationErrorForMessage(
>>> ValidationError error,
>>> const char* description = nullptr);
>>>
>>> +MOJO_CPP_BINDINGS_EXPORT void SuppressValidationErrorLoggingForTests(
>>> + bool suppress);
>>> +
>>> // Only used by validation tests and when there is only one thread doing
>>> message
>>> // validation.
>>> class MOJO_CPP_BINDINGS_EXPORT ValidationErrorObserverForTesting {
>>>
>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2016-10-25 17:45:42]

perhaps we can just have a way for a particular test suite to have higher
limits?

[yzshen1, 2016-10-25 18:04:41]

On Wed, Oct 26, 2016 at 1:33 AM, Yuzhu Shen <yzshen@chromium.org> wrote:

> I don't have a strong opinion but just want to let you know that it did
> help me before: when my CL introduced validation bugs, unittests spit out
> validation error log that helped me to immediately realize what I have done
> wrong.
>


> (But maybe that means I didn't write enough validation test cases, rather
> than we should log during testing.)
>

Another way to put it: currently we don't have enough validation test cases
that test the "PASS" cases (and it may be tedious to add coverage); most
unittests sending mojo messages are actually used as "PASS" validation test
cases. Removing logging may make it a little harder to figure out
validation bugs during bindings development.


>
On Wed, Oct 26, 2016 at 1:21 AM, <rockot@chromium.org> wrote:
>
>> Reviewers: jam
>> CL: https://codereview.chromium.org/2448203002/
>>
>> Description:
>> Mojo: Suppress validation error logging in tests
>>
>> These errors are useful for developers when developers make mistakes,
>> but they make a mess of test logs. Disable them in tests.
>>
>> BUG=659195
>>
>> Affected files (+16, -3 lines):
>> M mojo/edk/test/run_all_unittests.cc
>> M mojo/public/cpp/bindings/lib/validation_errors.h
>> M mojo/public/cpp/bindings/lib/validation_errors.cc
>>
>>
>> Index: mojo/edk/test/run_all_unittests.cc
>> diff --git a/mojo/edk/test/run_all_unittests.cc
>> b/mojo/edk/test/run_all_unittests.cc
>> index
05aed913a4868f2fa2ae443e1c9098a44ab23ade..e5802ecb125d8e3f013dad8700324c96fecc062f
>> 100644
>> --- a/mojo/edk/test/run_all_unittests.cc
>> +++ b/mojo/edk/test/run_all_unittests.cc
>> @@ -14,6 +14,7 @@
>> #include "mojo/edk/test/multiprocess_test_helper.h"
>> #include "mojo/edk/test/scoped_ipc_support.h"
>> #include "mojo/edk/test/test_support_impl.h"
>> +#include "mojo/public/cpp/bindings/lib/validation_errors.h"
>> #include "mojo/public/tests/test_support_private.h"
>> #include "testing/gtest/include/gtest/gtest.h"
>>
>> @@ -48,6 +49,7 @@ int main(int argc, char** argv) {
>> // loop, which is destructed inside base::LaunchUnitTests.
>> new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
>>
>> + mojo::internal::SuppressValidationErrorLoggingForTests(true);
>> return base::LaunchUnitTests(
>> argc, argv,
>> base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
>> Index: mojo/public/cpp/bindings/lib/validation_errors.cc
>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.cc
>> b/mojo/public/cpp/bindings/lib/validation_errors.cc
>> index
67106a0fd508dfe2e74a960564319edce92f8a5a..d7107714227504f2b85a791859baed10e9e20011
>> 100644
>> --- a/mojo/public/cpp/bindings/lib/validation_errors.cc
>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.cc
>> @@ -14,6 +14,7 @@ namespace {
>> ValidationErrorObserverForTesting* g_validation_error_observer = nullptr;
>> SerializationWarningObserverForTesting* g_serialization_warning_observer
>> =
>> nullptr;
>> +bool g_suppress_logging = false;
>>
>> } // namespace
>>
>> @@ -71,8 +72,10 @@ void ReportValidationError(ValidationContext* context,
>> }
>>
>> if (description) {
>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error) <<
>> " ("
>> - << description << ")";
>> + if (!g_suppress_logging) {
>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
>> + << " (" << description << ")";
>> + }
>> if (context->message()) {
>> context->message()->NotifyBadMessage(
>> base::StringPrintf("Validation failed for %s [%s (%s)]",
>> @@ -80,7 +83,8 @@ void ReportValidationError(ValidationContext* context,
>> ValidationErrorToString(error), description));
>> }
>> } else {
>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>> + if (!g_suppress_logging)
>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>> if (context->message()) {
>> context->message()->NotifyBadMessage(
>> base::StringPrintf("Validation failed for %s [%s]",
>> @@ -101,6 +105,10 @@ void ReportValidationErrorForMessage(
>> ReportValidationError(&validation_context, error);
>> }
>>
>> +void SuppressValidationErrorLoggingForTests(bool suppress) {
>> + g_suppress_logging = suppress;
>> +}
>> +
>> ValidationErrorObserverForTesting::ValidationErrorObserverForTesting(
>> const base::Closure& callback)
>> : last_error_(VALIDATION_ERROR_NONE), callback_(callback) {
>> Index: mojo/public/cpp/bindings/lib/validation_errors.h
>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h
>> b/mojo/public/cpp/bindings/lib/validation_errors.h
>> index
7636e391dad4414dfff7659a04e75c12e9dd1ff4..496c5dd41cf983067b1d614e641944b3a66e4d90
>> 100644
>> --- a/mojo/public/cpp/bindings/lib/validation_errors.h
>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.h
>> @@ -89,6 +89,9 @@ MOJO_CPP_BINDINGS_EXPORT void
>> ReportValidationErrorForMessage(
>> ValidationError error,
>> const char* description = nullptr);
>>
>> +MOJO_CPP_BINDINGS_EXPORT void SuppressValidationErrorLoggingForTests(
>> + bool suppress);
>> +
>> // Only used by validation tests and when there is only one thread doing
>> message
>> // validation.
>> class MOJO_CPP_BINDINGS_EXPORT ValidationErrorObserverForTesting {
>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-25 18:12:05]

Yeah I don't think this CL is the right thing to do.

On Tue, Oct 25, 2016 at 11:04 AM, Yuzhu Shen <yzshen@chromium.org> wrote:

> On Wed, Oct 26, 2016 at 1:33 AM, Yuzhu Shen <yzshen@chromium.org> wrote:
>
>> I don't have a strong opinion but just want to let you know that it did
>> help me before: when my CL introduced validation bugs, unittests spit out
>> validation error log that helped me to immediately realize what I have done
>> wrong.
>>
>
>
>> (But maybe that means I didn't write enough validation test cases, rather
>> than we should log during testing.)
>>
>
> Another way to put it: currently we don't have enough validation test
> cases that test the "PASS" cases (and it may be tedious to add coverage);
> most unittests sending mojo messages are actually used as "PASS" validation
> test cases. Removing logging may make it a little harder to figure out
> validation bugs during bindings development.
>
>
>>
> On Wed, Oct 26, 2016 at 1:21 AM, <rockot@chromium.org> wrote:
>>
>>> Reviewers: jam
>>> CL: https://codereview.chromium.org/2448203002/
>>>
>>> Description:
>>> Mojo: Suppress validation error logging in tests
>>>
>>> These errors are useful for developers when developers make mistakes,
>>> but they make a mess of test logs. Disable them in tests.
>>>
>>> BUG=659195
>>>
>>> Affected files (+16, -3 lines):
>>> M mojo/edk/test/run_all_unittests.cc
>>> M mojo/public/cpp/bindings/lib/validation_errors.h
>>> M mojo/public/cpp/bindings/lib/validation_errors.cc
>>>
>>>
>>> Index: mojo/edk/test/run_all_unittests.cc
>>> diff --git a/mojo/edk/test/run_all_unittests.cc
>>> b/mojo/edk/test/run_all_unittests.cc
>>> index
05aed913a4868f2fa2ae443e1c9098a44ab23ade..e5802ecb125d8e3f013dad8700324c96fecc062f
>>> 100644
>>> --- a/mojo/edk/test/run_all_unittests.cc
>>> +++ b/mojo/edk/test/run_all_unittests.cc
>>> @@ -14,6 +14,7 @@
>>> #include "mojo/edk/test/multiprocess_test_helper.h"
>>> #include "mojo/edk/test/scoped_ipc_support.h"
>>> #include "mojo/edk/test/test_support_impl.h"
>>> +#include "mojo/public/cpp/bindings/lib/validation_errors.h"
>>> #include "mojo/public/tests/test_support_private.h"
>>> #include "testing/gtest/include/gtest/gtest.h"
>>>
>>> @@ -48,6 +49,7 @@ int main(int argc, char** argv) {
>>> // loop, which is destructed inside base::LaunchUnitTests.
>>> new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
>>>
>>> + mojo::internal::SuppressValidationErrorLoggingForTests(true);
>>> return base::LaunchUnitTests(
>>> argc, argv,
>>> base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
>>> Index: mojo/public/cpp/bindings/lib/validation_errors.cc
>>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> b/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> index
67106a0fd508dfe2e74a960564319edce92f8a5a..d7107714227504f2b85a791859baed10e9e20011
>>> 100644
>>> --- a/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.cc
>>> @@ -14,6 +14,7 @@ namespace {
>>> ValidationErrorObserverForTesting* g_validation_error_observer =
>>> nullptr;
>>> SerializationWarningObserverForTesting* g_serialization_warning_observer
>>> =
>>> nullptr;
>>> +bool g_suppress_logging = false;
>>>
>>> } // namespace
>>>
>>> @@ -71,8 +72,10 @@ void ReportValidationError(ValidationContext*
>>> context,
>>> }
>>>
>>> if (description) {
>>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error) <<
>>> " ("
>>> - << description << ")";
>>> + if (!g_suppress_logging) {
>>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
>>> + << " (" << description << ")";
>>> + }
>>> if (context->message()) {
>>> context->message()->NotifyBadMessage(
>>> base::StringPrintf("Validation failed for %s [%s (%s)]",
>>> @@ -80,7 +83,8 @@ void ReportValidationError(ValidationContext* context,
>>> ValidationErrorToString(error), description));
>>> }
>>> } else {
>>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>>> + if (!g_suppress_logging)
>>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>>> if (context->message()) {
>>> context->message()->NotifyBadMessage(
>>> base::StringPrintf("Validation failed for %s [%s]",
>>> @@ -101,6 +105,10 @@ void ReportValidationErrorForMessage(
>>> ReportValidationError(&validation_context, error);
>>> }
>>>
>>> +void SuppressValidationErrorLoggingForTests(bool suppress) {
>>> + g_suppress_logging = suppress;
>>> +}
>>> +
>>> ValidationErrorObserverForTesting::ValidationErrorObserverForTesting(
>>> const base::Closure& callback)
>>> : last_error_(VALIDATION_ERROR_NONE), callback_(callback) {
>>> Index: mojo/public/cpp/bindings/lib/validation_errors.h
>>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h
>>> b/mojo/public/cpp/bindings/lib/validation_errors.h
>>> index
7636e391dad4414dfff7659a04e75c12e9dd1ff4..496c5dd41cf983067b1d614e641944b3a66e4d90
>>> 100644
>>> --- a/mojo/public/cpp/bindings/lib/validation_errors.h
>>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.h
>>> @@ -89,6 +89,9 @@ MOJO_CPP_BINDINGS_EXPORT void
>>> ReportValidationErrorForMessage(
>>> ValidationError error,
>>> const char* description = nullptr);
>>>
>>> +MOJO_CPP_BINDINGS_EXPORT void SuppressValidationErrorLoggingForTests(
>>> + bool suppress);
>>> +
>>> // Only used by validation tests and when there is only one thread doing
>>> message
>>> // validation.
>>> class MOJO_CPP_BINDINGS_EXPORT ValidationErrorObserverForTesting {
>>>
>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-10-25 18:16:39]

Thanks for reply!

I don't have strong opinion whether we should have a new log level (as Ken
suggested) or finer-grain control (as John suggested).



On Wed, Oct 26, 2016 at 2:12 AM, Ken Rockot <rockot@chromium.org> wrote:

> Yeah I don't think this CL is the right thing to do.
>
> On Tue, Oct 25, 2016 at 11:04 AM, Yuzhu Shen <yzshen@chromium.org> wrote:
>
>> On Wed, Oct 26, 2016 at 1:33 AM, Yuzhu Shen <yzshen@chromium.org> wrote:
>>
>>> I don't have a strong opinion but just want to let you know that it did
>>> help me before: when my CL introduced validation bugs, unittests spit out
>>> validation error log that helped me to immediately realize what I have done
>>> wrong.
>>>
>>
>>
>>> (But maybe that means I didn't write enough validation test cases,
>>> rather than we should log during testing.)
>>>
>>
>> Another way to put it: currently we don't have enough validation test
>> cases that test the "PASS" cases (and it may be tedious to add coverage);
>> most unittests sending mojo messages are actually used as "PASS" validation
>> test cases. Removing logging may make it a little harder to figure out
>> validation bugs during bindings development.
>>
>>
>>>
>> On Wed, Oct 26, 2016 at 1:21 AM, <rockot@chromium.org> wrote:
>>>
>>>> Reviewers: jam
>>>> CL: https://codereview.chromium.org/2448203002/
>>>>
>>>> Description:
>>>> Mojo: Suppress validation error logging in tests
>>>>
>>>> These errors are useful for developers when developers make mistakes,
>>>> but they make a mess of test logs. Disable them in tests.
>>>>
>>>> BUG=659195
>>>>
>>>> Affected files (+16, -3 lines):
>>>> M mojo/edk/test/run_all_unittests.cc
>>>> M mojo/public/cpp/bindings/lib/validation_errors.h
>>>> M mojo/public/cpp/bindings/lib/validation_errors.cc
>>>>
>>>>
>>>> Index: mojo/edk/test/run_all_unittests.cc
>>>> diff --git a/mojo/edk/test/run_all_unittests.cc
>>>> b/mojo/edk/test/run_all_unittests.cc
>>>> index
05aed913a4868f2fa2ae443e1c9098a44ab23ade..e5802ecb125d8e3f013dad8700324c96fecc062f
>>>> 100644
>>>> --- a/mojo/edk/test/run_all_unittests.cc
>>>> +++ b/mojo/edk/test/run_all_unittests.cc
>>>> @@ -14,6 +14,7 @@
>>>> #include "mojo/edk/test/multiprocess_test_helper.h"
>>>> #include "mojo/edk/test/scoped_ipc_support.h"
>>>> #include "mojo/edk/test/test_support_impl.h"
>>>> +#include "mojo/public/cpp/bindings/lib/validation_errors.h"
>>>> #include "mojo/public/tests/test_support_private.h"
>>>> #include "testing/gtest/include/gtest/gtest.h"
>>>>
>>>> @@ -48,6 +49,7 @@ int main(int argc, char** argv) {
>>>> // loop, which is destructed inside base::LaunchUnitTests.
>>>> new mojo::edk::test::ScopedIPCSupport(test_io_thread.task_runner());
>>>>
>>>> + mojo::internal::SuppressValidationErrorLoggingForTests(true);
>>>> return base::LaunchUnitTests(
>>>> argc, argv,
>>>> base::Bind(&base::TestSuite::Run, base::Unretained(&test_suite)));
>>>> Index: mojo/public/cpp/bindings/lib/validation_errors.cc
>>>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.cc
>>>> b/mojo/public/cpp/bindings/lib/validation_errors.cc
>>>> index
67106a0fd508dfe2e74a960564319edce92f8a5a..d7107714227504f2b85a791859baed10e9e20011
>>>> 100644
>>>> --- a/mojo/public/cpp/bindings/lib/validation_errors.cc
>>>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.cc
>>>> @@ -14,6 +14,7 @@ namespace {
>>>> ValidationErrorObserverForTesting* g_validation_error_observer =
>>>> nullptr;
>>>> SerializationWarningObserverForTesting* g_serialization_warning_observer
>>>> =
>>>> nullptr;
>>>> +bool g_suppress_logging = false;
>>>>
>>>> } // namespace
>>>>
>>>> @@ -71,8 +72,10 @@ void ReportValidationError(ValidationContext*
>>>> context,
>>>> }
>>>>
>>>> if (description) {
>>>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
>>>> << " ("
>>>> - << description << ")";
>>>> + if (!g_suppress_logging) {
>>>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error)
>>>> + << " (" << description << ")";
>>>> + }
>>>> if (context->message()) {
>>>> context->message()->NotifyBadMessage(
>>>> base::StringPrintf("Validation failed for %s [%s (%s)]",
>>>> @@ -80,7 +83,8 @@ void ReportValidationError(ValidationContext*
>>>> context,
>>>> ValidationErrorToString(error), description));
>>>> }
>>>> } else {
>>>> - LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>>>> + if (!g_suppress_logging)
>>>> + LOG(ERROR) << "Invalid message: " << ValidationErrorToString(error);
>>>> if (context->message()) {
>>>> context->message()->NotifyBadMessage(
>>>> base::StringPrintf("Validation failed for %s [%s]",
>>>> @@ -101,6 +105,10 @@ void ReportValidationErrorForMessage(
>>>> ReportValidationError(&validation_context, error);
>>>> }
>>>>
>>>> +void SuppressValidationErrorLoggingForTests(bool suppress) {
>>>> + g_suppress_logging = suppress;
>>>> +}
>>>> +
>>>> ValidationErrorObserverForTesting::ValidationErrorObserverForTesting(
>>>> const base::Closure& callback)
>>>> : last_error_(VALIDATION_ERROR_NONE), callback_(callback) {
>>>> Index: mojo/public/cpp/bindings/lib/validation_errors.h
>>>> diff --git a/mojo/public/cpp/bindings/lib/validation_errors.h
>>>> b/mojo/public/cpp/bindings/lib/validation_errors.h
>>>> index
7636e391dad4414dfff7659a04e75c12e9dd1ff4..496c5dd41cf983067b1d614e641944b3a66e4d90
>>>> 100644
>>>> --- a/mojo/public/cpp/bindings/lib/validation_errors.h
>>>> +++ b/mojo/public/cpp/bindings/lib/validation_errors.h
>>>> @@ -89,6 +89,9 @@ MOJO_CPP_BINDINGS_EXPORT void
>>>> ReportValidationErrorForMessage(
>>>> ValidationError error,
>>>> const char* description = nullptr);
>>>>
>>>> +MOJO_CPP_BINDINGS_EXPORT void SuppressValidationErrorLoggingForTests(
>>>> + bool suppress);
>>>> +
>>>> // Only used by validation tests and when there is only one thread
>>>> doing message
>>>> // validation.
>>>> class MOJO_CPP_BINDINGS_EXPORT ValidationErrorObserverForTesting {
>>>>
>>>>
>>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-10-25 19:11:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 19:11:07]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ken Rockot(use gerrit already), 2016-11-02 15:57:00]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2016-11-02 15:57:22]

Description was changed from

==========
Mojo: Suppress validation error logging in tests

These errors are useful for developers when developers make mistakes,
but they make a mess of test logs. Disable them in tests.

BUG=659195
==========

to

==========
Mojo: Suppress validation error logging in some tests

These errors are useful for developers when developers make mistakes,
but they make a mess of test logs. Disable them in tests which are
particularly noisy.

BUG=659195
==========

[commit-bot: I haz the power, 2016-11-02 15:57:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-11-02 15:57:49]

Yuzhu, please take another look. Thanks!

[Ken Rockot(use gerrit already), 2016-11-02 16:01:29]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 16:02:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-11-02 16:44:20]

lgtm

https://codereview.chromium.org/2448203002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.cc (right):

https://codereview.chromium.org/2448203002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.cc:108: void
SuppressValidationErrorLoggingForTests(bool suppress) {
This function is not used (or declared in the .h file).

[commit-bot: I haz the power, 2016-11-02 17:18:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 17:18:43]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-11-02 17:21:10]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 17:21:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-11-02 17:23:15]

https://codereview.chromium.org/2448203002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.cc (right):

https://codereview.chromium.org/2448203002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.cc:108: void
SuppressValidationErrorLoggingForTests(bool suppress) {
On 2016/11/02 at 16:44:20, yzshen1 wrote:
> This function is not used (or declared in the .h file).

Oops, forgot to delete. Done.

[Ken Rockot(use gerrit already), 2016-11-02 17:23:19]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-11-02 17:23:21]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-11-02 17:23:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2448203002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-11-02 17:23:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 20:46:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-02 20:48:34]

Description was changed from

==========
Mojo: Suppress validation error logging in some tests

These errors are useful for developers when developers make mistakes,
but they make a mess of test logs. Disable them in tests which are
particularly noisy.

BUG=659195
==========

to

==========
Mojo: Suppress validation error logging in some tests

These errors are useful for developers when developers make mistakes,
but they make a mess of test logs. Disable them in tests which are
particularly noisy.

BUG=659195

Committed: https://crrev.com/dafb77a4efa4ea0e6d2080ea239f401ff1c4a691
Cr-Commit-Position: refs/heads/master@{#429393}
==========

[commit-bot: I haz the power, 2016-11-02 20:48:35]

Patchset 5 (id:??) landed as
https://crrev.com/dafb77a4efa4ea0e6d2080ea239f401ff1c4a691
Cr-Commit-Position: refs/heads/master@{#429393}