Hide console log messages for imported WPT tests

Hide console log messages for imported WPT tests

Move console output messages for WPT tests from the test output file into stderr so they no longer affect whether the test passes or fails but can still be used when debugging a test. Remove all CONSOLE lines from external/wpt/**/*-expected.txt.

Update tests in following directories to add a console.warn statement to prevent the presubmit from trying to remove the -expected.txt file since these tests rely on console messages to determine whether they pass/fail:

third_party/WebKit/LayoutTests/csspaint (logs are from worklets which have no other way to communicate the test status)

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=679742
Review-Url: https://codereview.chromium.org/2678043002
Cr-Commit-Position: refs/heads/master@{#456362}
Committed: https://chromium.googlesource.com/chromium/src/+/f8d3488e588458268f165f6e21e2e86bff0b9b6d

[Dan Elphick, 2017-02-06 15:50:13]

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

[commit-bot: I haz the power, 2017-02-06 15:50:24]

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, 2017-02-06 17:08:23]

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

[commit-bot: I haz the power, 2017-02-06 17:08:24]

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

[Dan Elphick, 2017-02-07 13:42:45]

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

[commit-bot: I haz the power, 2017-02-07 13:43:01]

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

[Dan Elphick, 2017-02-07 14:10:20]

Description was changed from

==========
Disable console log messages for imported WPT tests

BUG=679742
==========

to

==========
Move all test console output messages from the test output file into stderr so
they no longer affect whether the test passes or fails but can still be used
when debugging a test.

BUG=679742
==========

[Dan Elphick, 2017-02-07 15:18:12]

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

[commit-bot: I haz the power, 2017-02-07 15:18:21]

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

[Dan Elphick, 2017-02-07 15:19:10]

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

[commit-bot: I haz the power, 2017-02-07 15:19:23]

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, 2017-02-07 17:35:23]

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

[commit-bot: I haz the power, 2017-02-07 17:35:23]

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

[qyearsley, 2017-02-16 18:54:43]

qyearsley@chromium.org changed reviewers:
+ qyearsley@chromium.org

[qyearsley, 2017-02-16 18:54:43]

Not sure if you wanted review yet. Main question I have about this is: after
this change, what content would go into the -expected.txt files, and when/how
would people see the stderr when running the tests? Is stderr printed by default
when running tests with run-webkit-tests, or might it be hidden by default?

https://codereview.chromium.org/2678043002/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py
(right):

https://codereview.chromium.org/2678043002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:236:
new_out += line
Console messages can have newlines in them -- so for example, if out is:

"""PASS: first test
CONSOLE WARNING: Line 1
Line 2
PASS: second test
"""

Then new_out would be:

"""PASS: first test
Line 2
PASS: second test
"""

But we don't want it to include "line 2".

Could you add a unit test to check this behavior?

[Dan Elphick, 2017-02-17 15:04:40]

https://codereview.chromium.org/2678043002/diff/60001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py
(right):

https://codereview.chromium.org/2678043002/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:236:
new_out += line
On 2017/02/16 18:54:43, qyearsley wrote:
> Console messages can have newlines in them -- so for example, if out is:
> 
> """PASS: first test
> CONSOLE WARNING: Line 1
> Line 2
> PASS: second test
> """
> 
> Then new_out would be:
> 
> """PASS: first test
> Line 2
> PASS: second test
> """
> 
> But we don't want it to include "line 2".
> 
> Could you add a unit test to check this behavior?

Just checked and this changes fails for new lines. To fix this, I changed the
logger to repeat the CONSOLE prefix for each line of a message.

[Dan Elphick, 2017-02-17 15:53:19]

On 2017/02/16 18:54:43, qyearsley wrote:
> Not sure if you wanted review yet. Main question I have about this is: after
> this change, what content would go into the -expected.txt files, and when/how
> would people see the stderr when running the tests? Is stderr printed by
default
> when running tests with run-webkit-tests, or might it be hidden by default?

Thanks for the review - all comments are welcome.

With this change console messages for WPT go into stderr, which is already
available in the test output although it's hidden by default.

For instance this is what you can see right now without my change:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

(just check stderr in the top right).

Clearly there's already a lot in stderr that's probably ignored nearly all of
the time, so adding console output to this view will likely make it invisible to
anyone who's not looking for it. I guess there could be an argument that console
output is actually some new third category that should be elevated above stderr
but still left out of the pass/fail criteria. My feeling is that building that
would be wasted effort and it would be ignored just as readily as stderr since
so many tests would output console messages.

[Dan Elphick, 2017-02-17 17:06:04]

On 2017/02/17 15:04:40, Dan Elphick wrote:
>
https://codereview.chromium.org/2678043002/diff/60001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py
> (right):
> 
>
https://codereview.chromium.org/2678043002/diff/60001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:236:
> new_out += line
> On 2017/02/16 18:54:43, qyearsley wrote:
> > Console messages can have newlines in them -- so for example, if out is:
> > 
> > """PASS: first test
> > CONSOLE WARNING: Line 1
> > Line 2
> > PASS: second test
> > """
> > 
> > Then new_out would be:
> > 
> > """PASS: first test
> > Line 2
> > PASS: second test
> > """
> > 
> > But we don't want it to include "line 2".
> > 
> > Could you add a unit test to check this behavior?
> 
> Just checked and this changes fails for new lines. To fix this, I changed the
> logger to repeat the CONSOLE prefix for each line of a message.

Added two tests that verify both enabled/disabled console filtering.

[qyearsley, 2017-02-17 18:53:38]

I think this approach sounds good and this CL LGTM :-)

CL description nit: usually the first line of the description is the same as the
CL title

https://codereview.chromium.org/2678043002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py
(right):

https://codereview.chromium.org/2678043002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:210: #
filter console output for WPT tests into stderr so that tests not
Nit: capitalize comment

https://codereview.chromium.org/2678043002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:232: line
+= '\n'
Note that if there's no newline at the end of the last line in the input, this
will add one.

  _filter_console_output_into_stderr(self, "out", "")
  => "out\n", ""

Is this OK?
If we don't want to do this, you could replace the above two lines with just:

  for line in out.splitlines(True):

[Dan Elphick, 2017-02-20 10:48:55]

Description was changed from

==========
Move all test console output messages from the test output file into stderr so
they no longer affect whether the test passes or fails but can still be used
when debugging a test.

BUG=679742
==========

to

==========
Disable console log messages for imported WPT tests

Move all test console output messages from the test output file into stderr so
they no longer affect whether the test passes or fails but can still be used
when debugging a test.

BUG=679742
==========

[Dan Elphick, 2017-02-20 16:26:22]

https://codereview.chromium.org/2678043002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py
(right):

https://codereview.chromium.org/2678043002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:210: #
filter console output for WPT tests into stderr so that tests not
On 2017/02/17 18:53:38, qyearsley wrote:
> Nit: capitalize comment

Done.

https://codereview.chromium.org/2678043002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py:232: line
+= '\n'
On 2017/02/17 18:53:38, qyearsley wrote:
> Note that if there's no newline at the end of the last line in the input, this
> will add one.
> 
>   _filter_console_output_into_stderr(self, "out", "")
>   => "out\n", ""
> 
> Is this OK?
> If we don't want to do this, you could replace the above two lines with just:
> 
>   for line in out.splitlines(True):

I hadn't considered that, but it's more readable to terminate files with a new
line than not to, so I'll leave it as it is.

[Dan Elphick, 2017-02-20 17:00:52]

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

[commit-bot: I haz the power, 2017-02-20 17:01:05]

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, 2017-02-20 17:07:11]

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

[commit-bot: I haz the power, 2017-02-20 17:07:13]

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...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Dan Elphick, 2017-02-21 16:55:01]

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

[commit-bot: I haz the power, 2017-02-21 16:55:45]

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, 2017-02-21 17:04:00]

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

[commit-bot: I haz the power, 2017-02-21 17:04:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dan Elphick, 2017-02-21 17:12:33]

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

[commit-bot: I haz the power, 2017-02-21 17:13:21]

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, 2017-02-21 18:22:17]

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

[commit-bot: I haz the power, 2017-02-21 18:22:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Elphick, 2017-02-22 12:27:28]

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

[commit-bot: I haz the power, 2017-02-22 12:27:55]

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

[Dan Elphick, 2017-02-22 14:32:39]

Description was changed from

==========
Disable console log messages for imported WPT tests

Move all test console output messages from the test output file into stderr so
they no longer affect whether the test passes or fails but can still be used
when debugging a test.

BUG=679742
==========

to

==========
Hide console log messages for imported WPT tests

Move console output messages for WPT tests from the test output file into stderr
so they no longer affect whether the test passes or fails but can still be used
when debugging a test. Remove all CONSOLE lines from
external/wpt/**/*-expected.txt.

Change content_shell to now start every line of a multi-line log statement with
the "CONSOLE (MESSAGE|WARNING|ERROR):" prefix so that the above filter doesn't
leaving dangling console messages in the output.

Update tests in following directories to add a console.warn statement to prevent
the presubmit from trying to remove the -expected.txt file since these tests
rely on console messages to determine whether they pass/fail:

third_party/WebKit/LayoutTests/csspaint (logs are from worklets which have no
other way to communicate the test status)
third_party/WebKit/LayoutTests/http/tests/origin_trials/webexposed (chasej@ is
updating the tests so this is just a stopgap)

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=679742
==========

[Dan Elphick, 2017-02-22 14:32:39]

delphick@chromium.org changed reviewers:
+ chasej@chromium.org, rbyers@chromium.org

[chasej, 2017-02-23 17:33:20]

lgtm

[Rick Byers, 2017-02-24 22:02:27]

Sorry for the delay.  I've looked this over and it seems to me like there's a
much simpler solution (which will involve a lot less -expected.txt changes
also).  But perhaps I'm missing something.  What do you think?

https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:633: if
(!test_runner()->ShouldDumpConsoleMessages())
Wouldn't it be a lot simpler to just change the implementation of this function
so that it defaults to 'false' when test_runner()->is_web_platform_tests_mode_
is true?

If you did that, you'd avoid generating the console output in the first place
and so wouldn't need to filter it out later from python.  That would avoid
duplicating both the logic for what paths constitute a wpt directory, and what
prefixes are used for console output.  Also then there'd be no reason to make
this change for multi-line output (though maybe it's still worth doing for other
reasons - dunno).

https://codereview.chromium.org/2678043002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/harness-tests/wpt/console_logging.html
(right):

https://codereview.chromium.org/2678043002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/harness-tests/wpt/console_logging.html:2:
<title>Console messages with new lines should have a console prefix applied to
each line.</title>
Nit: update title to indicate that for this test in 'wpt' we don't expect to see
the console log messages at all.

[Dan Elphick, 2017-02-27 11:03:06]

https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:633: if
(!test_runner()->ShouldDumpConsoleMessages())
On 2017/02/24 22:02:27, Rick Byers wrote:
> Wouldn't it be a lot simpler to just change the implementation of this
function
> so that it defaults to 'false' when test_runner()->is_web_platform_tests_mode_
> is true?
> 
> If you did that, you'd avoid generating the console output in the first place
> and so wouldn't need to filter it out later from python.  That would avoid
> duplicating both the logic for what paths constitute a wpt directory, and what
> prefixes are used for console output.  Also then there'd be no reason to make
> this change for multi-line output (though maybe it's still worth doing for
other
> reasons - dunno).

Isn't this functionally equivalent to just calling setDumpConsoleMessages(false)
during the test for instance in testharnessreport.js when the test matches a wpt
directory? I suggested that in
https://bugs.chromium.org/p/chromium/issues/detail?id=679742#c3 and Quintin even
tested it, but there was concern that this would completely remove the console
messages so it was harder to debug a failing test.

If our aim was to avoid duplicating the wpt detection code, we could perhaps
change this method to instead of calling PrintMessage, just output via
fprintf(stderr, ...). That would eliminate the python change at least.

https://codereview.chromium.org/2678043002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/harness-tests/wpt/console_logging.html
(right):

https://codereview.chromium.org/2678043002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/harness-tests/wpt/console_logging.html:2:
<title>Console messages with new lines should have a console prefix applied to
each line.</title>
On 2017/02/24 22:02:27, Rick Byers wrote:
> Nit: update title to indicate that for this test in 'wpt' we don't expect to
see
> the console log messages at all.

Done.

[Rick Byers, 2017-02-28 16:29:32]

https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:633: if
(!test_runner()->ShouldDumpConsoleMessages())
On 2017/02/27 11:03:06, Dan Elphick wrote:
> On 2017/02/24 22:02:27, Rick Byers wrote:
> > Wouldn't it be a lot simpler to just change the implementation of this
> function
> > so that it defaults to 'false' when
test_runner()->is_web_platform_tests_mode_
> > is true?
> > 
> > If you did that, you'd avoid generating the console output in the first
place
> > and so wouldn't need to filter it out later from python.  That would avoid
> > duplicating both the logic for what paths constitute a wpt directory, and
what
> > prefixes are used for console output.  Also then there'd be no reason to
make
> > this change for multi-line output (though maybe it's still worth doing for
> other
> > reasons - dunno).
> 
> Isn't this functionally equivalent to just calling
setDumpConsoleMessages(false)
> during the test for instance in testharnessreport.js when the test matches a
wpt
> directory? I suggested that in
> https://bugs.chromium.org/p/chromium/issues/detail?id=679742#c3 and Quintin
even
> tested it, but there was concern that this would completely remove the console
> messages so it was harder to debug a failing test.
> 
> If our aim was to avoid duplicating the wpt detection code, we could perhaps
> change this method to instead of calling PrintMessage, just output via
> fprintf(stderr, ...). That would eliminate the python change at least.

Ah, I'm sorry - I was lacking the context on the discussion here.  I just read
over the whole bug history.

One option that doesn't seem like it was discussed is to add a new method to
BlinkTestResultPrinter like PrintMessage but for debugging output which
shouldn't be included in the comparison (or maybe an option to the existing
method / IPC).  It looks like it would be pretty easy to handle this new path in
BlinkTestResultPrinter (similar to BlinkTestResultPrinter::AddErrorMessage) to
output only to stderr.

Would that have the same effect as what you're achieving here without the need /
bittleness of outputting then stripping the output?

I _think_ that could be nearly as simple as what you're doing here, but perhaps
I'm missing something.  If it is really more complex for some reason then I'd
personally be ok with the output-then-filter approach being done here.  But
since it's potentially brittle I'd want a second opinion from someone with more
experience - perhaps add dpranke@?

[Dan Elphick, 2017-02-28 17:05:57]

On 2017/02/28 16:29:32, Rick Byers wrote:
>
https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
> File content/shell/test_runner/web_frame_test_client.cc (right):
> 
>
https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
> content/shell/test_runner/web_frame_test_client.cc:633: if
> (!test_runner()->ShouldDumpConsoleMessages())
> On 2017/02/27 11:03:06, Dan Elphick wrote:
> > On 2017/02/24 22:02:27, Rick Byers wrote:
> > > Wouldn't it be a lot simpler to just change the implementation of this
> > function
> > > so that it defaults to 'false' when
> test_runner()->is_web_platform_tests_mode_
> > > is true?
> > > 
> > > If you did that, you'd avoid generating the console output in the first
> place
> > > and so wouldn't need to filter it out later from python.  That would avoid
> > > duplicating both the logic for what paths constitute a wpt directory, and
> what
> > > prefixes are used for console output.  Also then there'd be no reason to
> make
> > > this change for multi-line output (though maybe it's still worth doing for
> > other
> > > reasons - dunno).
> > 
> > Isn't this functionally equivalent to just calling
> setDumpConsoleMessages(false)
> > during the test for instance in testharnessreport.js when the test matches a
> wpt
> > directory? I suggested that in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=679742#c3 and Quintin
> even
> > tested it, but there was concern that this would completely remove the
console
> > messages so it was harder to debug a failing test.
> > 
> > If our aim was to avoid duplicating the wpt detection code, we could perhaps
> > change this method to instead of calling PrintMessage, just output via
> > fprintf(stderr, ...). That would eliminate the python change at least.
> 
> Ah, I'm sorry - I was lacking the context on the discussion here.  I just read
> over the whole bug history.
> 
> One option that doesn't seem like it was discussed is to add a new method to
> BlinkTestResultPrinter like PrintMessage but for debugging output which
> shouldn't be included in the comparison (or maybe an option to the existing
> method / IPC).  It looks like it would be pretty easy to handle this new path
in
> BlinkTestResultPrinter (similar to BlinkTestResultPrinter::AddErrorMessage) to
> output only to stderr.
> 
> Would that have the same effect as what you're achieving here without the need
/
> bittleness of outputting then stripping the output?

Not sure I understand what this new method BlinkTestResultPrinter method would
do
or who would call it (this code base is fairly new to me). Would
WebFrameTestClient
call this method or the other way round? I'm not clear that this would block
a CONSOLE ERROR due to an unhandled promise rejection unless it's something
WebFrameTestClient would call since that's currently where the code goes.

> I _think_ that could be nearly as simple as what you're doing here, but
perhaps
> I'm missing something.  If it is really more complex for some reason then I'd
> personally be ok with the output-then-filter approach being done here.  But
> since it's potentially brittle I'd want a second opinion from someone with
more
> experience - perhaps add dpranke@?

Something like the following can eliminate the python filtering, although it
needs
the test to signal that it's doesn't want to dump console messages.

for (auto piece : base::SplitStringPieceUsingSubstr(
         new_message, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL)) {
  if (dump_to_stderr) {
    fprintf(stderr, "%s%s\n", prefix.c_str(), piece.as_string().c_str());
  } else {
    delegate_->PrintMessage(prefix);
    delegate_->PrintMessage(piece.as_string());
    delegate_->PrintMessage(std::string("\n"));
  }
}

The other side of this is that testharnessreport.js already disables console
messages
towards the end of the test, and with a change like this the message it's trying
to
suppress would go in stderr, possibly inflating a lot of logs. Perhaps a ternary
flag
or another test_runner method is what's needed.

[Dan Elphick, 2017-02-28 17:07:38]

delphick@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dan Elphick, 2017-02-28 17:09:16]

Dirk, do you have any views on the console log message filtering approaches
discussed here?

[Rick Byers, 2017-02-28 20:12:16]

On 2017/02/28 17:05:57, Dan Elphick wrote:
> On 2017/02/28 16:29:32, Rick Byers wrote:
> >
>
https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
> > File content/shell/test_runner/web_frame_test_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2678043002/diff/200001/content/shell/test_run...
> > content/shell/test_runner/web_frame_test_client.cc:633: if
> > (!test_runner()->ShouldDumpConsoleMessages())
> > On 2017/02/27 11:03:06, Dan Elphick wrote:
> > > On 2017/02/24 22:02:27, Rick Byers wrote:
> > > > Wouldn't it be a lot simpler to just change the implementation of this
> > > function
> > > > so that it defaults to 'false' when
> > test_runner()->is_web_platform_tests_mode_
> > > > is true?
> > > > 
> > > > If you did that, you'd avoid generating the console output in the first
> > place
> > > > and so wouldn't need to filter it out later from python.  That would
avoid
> > > > duplicating both the logic for what paths constitute a wpt directory,
and
> > what
> > > > prefixes are used for console output.  Also then there'd be no reason to
> > make
> > > > this change for multi-line output (though maybe it's still worth doing
for
> > > other
> > > > reasons - dunno).
> > > 
> > > Isn't this functionally equivalent to just calling
> > setDumpConsoleMessages(false)
> > > during the test for instance in testharnessreport.js when the test matches
a
> > wpt
> > > directory? I suggested that in
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=679742#c3 and
Quintin
> > even
> > > tested it, but there was concern that this would completely remove the
> console
> > > messages so it was harder to debug a failing test.
> > > 
> > > If our aim was to avoid duplicating the wpt detection code, we could
perhaps
> > > change this method to instead of calling PrintMessage, just output via
> > > fprintf(stderr, ...). That would eliminate the python change at least.
> > 
> > Ah, I'm sorry - I was lacking the context on the discussion here.  I just
read
> > over the whole bug history.
> > 
> > One option that doesn't seem like it was discussed is to add a new method to
> > BlinkTestResultPrinter like PrintMessage but for debugging output which
> > shouldn't be included in the comparison (or maybe an option to the existing
> > method / IPC).  It looks like it would be pretty easy to handle this new
path
> in
> > BlinkTestResultPrinter (similar to BlinkTestResultPrinter::AddErrorMessage)
to
> > output only to stderr.
> > 
> > Would that have the same effect as what you're achieving here without the
need
> /
> > bittleness of outputting then stripping the output?
> 
> Not sure I understand what this new method BlinkTestResultPrinter method would
> do
> or who would call it (this code base is fairly new to me). Would
> WebFrameTestClient
> call this method or the other way round? I'm not clear that this would block
> a CONSOLE ERROR due to an unhandled promise rejection unless it's something
> WebFrameTestClient would call since that's currently where the code goes.

I don't have much experience with this particular code either, so I may be
mis-understanding.  But it looks to me like BlinkTestResultPrinter is what's
ultimately responsible for generating the output. So if we want a different sort
of output that goes to stderr, it should probably come from here.  I was just
following the existing code path for console messages:

WebFrameTestClient::didAddMessageToConsole ->
WebTestDelegate::PrintMessage ->
IPC ShellViewHostMsg_PrintMessage ->
BlinkTestController::OnPrintMessage ->
BlinkTestResultPrinter::AddMessageRaw


> > I _think_ that could be nearly as simple as what you're doing here, but
> perhaps
> > I'm missing something.  If it is really more complex for some reason then
I'd
> > personally be ok with the output-then-filter approach being done here.  But
> > since it's potentially brittle I'd want a second opinion from someone with
> more
> > experience - perhaps add dpranke@?
> 
> Something like the following can eliminate the python filtering, although it
> needs
> the test to signal that it's doesn't want to dump console messages.
> 
> for (auto piece : base::SplitStringPieceUsingSubstr(
>          new_message, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL)) {
>   if (dump_to_stderr) {
>     fprintf(stderr, "%s%s\n", prefix.c_str(), piece.as_string().c_str());
>   } else {
>     delegate_->PrintMessage(prefix);
>     delegate_->PrintMessage(piece.as_string());
>     delegate_->PrintMessage(std::string("\n"));
>   }
> }

That's essentially what I'm suggesting except that I assumed we'd want to route
all output via the existing abstraction layers rather than fprintf directly from
the WebFrameTestClient code.

In terms of having the test signal it doesn't want console messages on stdout,
this is what I was saying TestRunner::is_web_platform_tests_mode would be good
for.  WebFrameTestClient can easily query that to change it's behavior (as we do
in some other cases).  This seems preferable to me to having independent logic
in the python code for identifying paths which want a different behavior for
handling output (confine all the logic for "how WPT tests are different" to a
single mode switch in the test runner code).

> The other side of this is that testharnessreport.js already disables console
> messages
> towards the end of the test, and with a change like this the message it's
trying
> to
> suppress would go in stderr, possibly inflating a lot of logs. Perhaps a
ternary
> flag
> or another test_runner method is what's needed.

So stderr is captured in the test run logs instead of in the -expected files
today?  Your python-based change already re-sends all console messages to stderr
when filtering them out of the output, right?  How is the alternative we're
talking about here any different in that regard?

[Dirk Pranke, 2017-03-01 01:01:28]

On 2017/02/28 17:09:16, Dan Elphick wrote:
> Dirk, do you have any views on the console log message filtering approaches
> discussed here?

To the extent that I've followed this thread and the related email thread (which
is to say, not closely), 
I don't have any strong leanings here.

If you're really stuck I could think about this for a while and probably form an
opinion, but given
that I'm not really working on Blink these days it's probably better for you to
decide what you 
want yourselves.

[Dan Elphick, 2017-03-06 13:59:21]

On 2017/02/28 20:12:16, Rick Byers wrote:
> On 2017/02/28 17:05:57, Dan Elphick wrote:
> > Not sure I understand what this new method BlinkTestResultPrinter method
would
> > do
> > or who would call it (this code base is fairly new to me). Would
> > WebFrameTestClient
> > call this method or the other way round? I'm not clear that this would block
> > a CONSOLE ERROR due to an unhandled promise rejection unless it's something
> > WebFrameTestClient would call since that's currently where the code goes.
> 
> I don't have much experience with this particular code either, so I may be
> mis-understanding.  But it looks to me like BlinkTestResultPrinter is what's
> ultimately responsible for generating the output. So if we want a different
sort
> of output that goes to stderr, it should probably come from here.  I was just
> following the existing code path for console messages:
> 
> WebFrameTestClient::didAddMessageToConsole ->
> WebTestDelegate::PrintMessage ->
> IPC ShellViewHostMsg_PrintMessage ->
> BlinkTestController::OnPrintMessage ->
> BlinkTestResultPrinter::AddMessageRaw

I'll take a look at it.

> > The other side of this is that testharnessreport.js already disables console
> > messages
> > towards the end of the test, and with a change like this the message it's
> trying
> > to
> > suppress would go in stderr, possibly inflating a lot of logs. Perhaps a
> ternary
> > flag
> > or another test_runner method is what's needed.
> 
> So stderr is captured in the test run logs instead of in the -expected files
> today?  Your python-based change already re-sends all console messages to
stderr
> when filtering them out of the output, right?  How is the alternative we're
> talking about here any different in that regard?

I was referring to
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...
That suppresses console messages (completely) and so my python filtering never
sees
the messages to redirect them and so they are thrown away as is intended.

If we can use is_web_platform_tests_mode() to determine whether to dump to
output
or stderr and then continue to use ShouldDumpConsoleMessages() to determine
whether to log at all, I think we can maintain all the advantages without having
to put
additional WPT detection code inside the python.

[Dan Elphick, 2017-03-06 17:14:46]

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

[commit-bot: I haz the power, 2017-03-06 17:15:10]

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

[Dan Elphick, 2017-03-06 17:23:25]

On 2017/03/06 13:59:21, Dan Elphick wrote:
> On 2017/02/28 20:12:16, Rick Byers wrote:
> > On 2017/02/28 17:05:57, Dan Elphick wrote:
> > > Not sure I understand what this new method BlinkTestResultPrinter method
> would
> > > do
> > > or who would call it (this code base is fairly new to me). Would
> > > WebFrameTestClient
> > > call this method or the other way round? I'm not clear that this would
block
> > > a CONSOLE ERROR due to an unhandled promise rejection unless it's
something
> > > WebFrameTestClient would call since that's currently where the code goes.
> > 
> > I don't have much experience with this particular code either, so I may be
> > mis-understanding.  But it looks to me like BlinkTestResultPrinter is what's
> > ultimately responsible for generating the output. So if we want a different
> sort
> > of output that goes to stderr, it should probably come from here.  I was
just
> > following the existing code path for console messages:
> > 
> > WebFrameTestClient::didAddMessageToConsole ->
> > WebTestDelegate::PrintMessage ->
> > IPC ShellViewHostMsg_PrintMessage ->
> > BlinkTestController::OnPrintMessage ->
> > BlinkTestResultPrinter::AddMessageRaw
> 
> I'll take a look at it.
> 
> > > The other side of this is that testharnessreport.js already disables
console
> > > messages
> > > towards the end of the test, and with a change like this the message it's
> > trying
> > > to
> > > suppress would go in stderr, possibly inflating a lot of logs. Perhaps a
> > ternary
> > > flag
> > > or another test_runner method is what's needed.
> > 
> > So stderr is captured in the test run logs instead of in the -expected files
> > today?  Your python-based change already re-sends all console messages to
> stderr
> > when filtering them out of the output, right?  How is the alternative we're
> > talking about here any different in that regard?
> 
> I was referring to
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...
> That suppresses console messages (completely) and so my python filtering never
> sees
> the messages to redirect them and so they are thrown away as is intended.
> 
> If we can use is_web_platform_tests_mode() to determine whether to dump to
> output
> or stderr and then continue to use ShouldDumpConsoleMessages() to determine
> whether to log at all, I think we can maintain all the advantages without
having
> to put
> additional WPT detection code inside the python.

Rick, I think I've implemented what you asked for.

I will also remove the console prefix repetition code on line breaks since it's
causing huge amounts of churn in the log files. Do you want me to reintroduce it
in a separate change or shall I drop it completely?

[chasej, 2017-03-06 18:08:48]

not LGTM, more detailed comment below.

https://codereview.chromium.org/2678043002/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/webexposed/imagecapture-origin-trial-interfaces.html
(right):

https://codereview.chromium.org/2678043002/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/origin_trials/webexposed/imagecapture-origin-trial-interfaces.html:19:
console.warn('This test relies on console message comparison.');
This warn should not be needed anymore, based my earlier commit to rework the
tests.

I haven't reviewed the recent changes to the rest of the CL. But, my assumption
is that
no changes should be required to any of the origin trials layout tests.

[commit-bot: I haz the power, 2017-03-06 20:45:47]

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

[commit-bot: I haz the power, 2017-03-06 20:45:53]

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

[Dan Elphick, 2017-03-07 12:25:06]

On 2017/03/06 18:08:48, chasej wrote:
> not LGTM, more detailed comment below.
> 
>
https://codereview.chromium.org/2678043002/diff/230001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/http/tests/origin_trials/webexposed/imagecapture-origin-trial-interfaces.html
> (right):
> 
>
https://codereview.chromium.org/2678043002/diff/230001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/origin_trials/webexposed/imagecapture-origin-trial-interfaces.html:19:
> console.warn('This test relies on console message comparison.');
> This warn should not be needed anymore, based my earlier commit to rework the
> tests.
> 
> I haven't reviewed the recent changes to the rest of the CL. But, my
assumption
> is that
> no changes should be required to any of the origin trials layout tests.

Sorry that was an oversight. I've since removed all changes to files outside
external/wpt since my change no longer adds a prefix to every line of a
multi-line console message.

PTAL in case you think I missed anyt

[chasej, 2017-03-07 22:33:29]

lgtm

[qyearsley, 2017-03-08 22:51:26]

On 2017/03/07 at 22:33:29, chasej wrote:
> lgtm

Is this CL ready to commit?

By the way, I'm excited about this change, since I think it will decrease flaky
failures of web-platform-tests on our infrastructure :-)

[Dan Elphick, 2017-03-09 10:52:06]

On 2017/03/08 22:51:26, qyearsley wrote:
> On 2017/03/07 at 22:33:29, chasej wrote:
> > lgtm
> 
> Is this CL ready to commit?
> 
> By the way, I'm excited about this change, since I think it will decrease
flaky
> failures of web-platform-tests on our infrastructure :-)

Yes, just in need of an LGTM for the new way of logging.

[Rick Byers, 2017-03-09 13:13:57]

On 2017/03/06 17:23:25, Dan Elphick wrote:
> On 2017/03/06 13:59:21, Dan Elphick wrote:
> > On 2017/02/28 20:12:16, Rick Byers wrote:
> > > On 2017/02/28 17:05:57, Dan Elphick wrote:
> > > > Not sure I understand what this new method BlinkTestResultPrinter method
> > would
> > > > do
> > > > or who would call it (this code base is fairly new to me). Would
> > > > WebFrameTestClient
> > > > call this method or the other way round? I'm not clear that this would
> block
> > > > a CONSOLE ERROR due to an unhandled promise rejection unless it's
> something
> > > > WebFrameTestClient would call since that's currently where the code
goes.
> > > 
> > > I don't have much experience with this particular code either, so I may be
> > > mis-understanding.  But it looks to me like BlinkTestResultPrinter is
what's
> > > ultimately responsible for generating the output. So if we want a
different
> > sort
> > > of output that goes to stderr, it should probably come from here.  I was
> just
> > > following the existing code path for console messages:
> > > 
> > > WebFrameTestClient::didAddMessageToConsole ->
> > > WebTestDelegate::PrintMessage ->
> > > IPC ShellViewHostMsg_PrintMessage ->
> > > BlinkTestController::OnPrintMessage ->
> > > BlinkTestResultPrinter::AddMessageRaw
> > 
> > I'll take a look at it.
> > 
> > > > The other side of this is that testharnessreport.js already disables
> console
> > > > messages
> > > > towards the end of the test, and with a change like this the message
it's
> > > trying
> > > > to
> > > > suppress would go in stderr, possibly inflating a lot of logs. Perhaps a
> > > ternary
> > > > flag
> > > > or another test_runner method is what's needed.
> > > 
> > > So stderr is captured in the test run logs instead of in the -expected
files
> > > today?  Your python-based change already re-sends all console messages to
> > stderr
> > > when filtering them out of the output, right?  How is the alternative
we're
> > > talking about here any different in that regard?
> > 
> > I was referring to
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...
> > That suppresses console messages (completely) and so my python filtering
never
> > sees
> > the messages to redirect them and so they are thrown away as is intended.
> > 
> > If we can use is_web_platform_tests_mode() to determine whether to dump to
> > output
> > or stderr and then continue to use ShouldDumpConsoleMessages() to determine
> > whether to log at all, I think we can maintain all the advantages without
> having
> > to put
> > additional WPT detection code inside the python.
> 
> Rick, I think I've implemented what you asked for.

Great, looking now.  Sorry for the delay (perf!)

> I will also remove the console prefix repetition code on line breaks since
it's
> causing huge amounts of churn in the log files. Do you want me to reintroduce
it
> in a separate change or shall I drop it completely?

Nah if it's not necessary I wouldn't bother - doesn't seem worth the churn to
me.

[Rick Byers, 2017-03-09 13:22:24]

LGTM with nits.  Thank you very much for putting the extra time into this, this
approach seems much more maintainable to me!

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:655: bool dump_to_stderr =
test_runner()->is_web_platform_tests_mode();
nit: add a comment saying why, eg. "Console messages shouldn't be included in
the expected output for web-platform-tests because they may create
non-determinism not intended by the test author.  But still generate the output
on stderr for debugging purposes".

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
File content/shell/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
content/shell/test_runner/web_test_delegate.h:73: // Add a message to stderr.
nit: explicitly add "(not saved to expected output files, for debugging only)"
to make the purpose clear.

https://codereview.chromium.org/2678043002/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/harness-tests/console_logging.html (right):

https://codereview.chromium.org/2678043002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/harness-tests/console_logging.html:2:
<title>Console messages with new lines should have a console prefix applied to
each line.</title>
nit: update test description now that this behavior is no longer true

[Dan Elphick, 2017-03-09 13:30:44]

delphick@chromium.org changed reviewers:
+ peter@chromium.org

[Dan Elphick, 2017-03-09 13:41:40]

Thanks for the comments! This approach is much better than using python for the
filtering.

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:655: bool dump_to_stderr =
test_runner()->is_web_platform_tests_mode();
On 2017/03/09 13:22:24, Rick Byers (slow) wrote:
> nit: add a comment saying why, eg. "Console messages shouldn't be included in
> the expected output for web-platform-tests because they may create
> non-determinism not intended by the test author.  But still generate the
output
> on stderr for debugging purposes".

Done.

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
File content/shell/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/2678043002/diff/260001/content/shell/test_run...
content/shell/test_runner/web_test_delegate.h:73: // Add a message to stderr.
On 2017/03/09 13:22:24, Rick Byers (slow) wrote:
> nit: explicitly add "(not saved to expected output files, for debugging only)"
> to make the purpose clear.

Done.

https://codereview.chromium.org/2678043002/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/harness-tests/console_logging.html (right):

https://codereview.chromium.org/2678043002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/harness-tests/console_logging.html:2:
<title>Console messages with new lines should have a console prefix applied to
each line.</title>
On 2017/03/09 13:22:24, Rick Byers (slow) wrote:
> nit: update test description now that this behavior is no longer true

Done.

[Rick Byers, 2017-03-09 14:21:00]

Still LGTM, thanks!

[Peter Beverloo, 2017-03-09 15:07:49]

//content/shell lgtm w/ nits

(You'll still need an IPC reviewer for shell_messages.h.)

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:651: std::string
consoleMessage(std::string("CONSOLE ") + level + ": ");
console_message

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
File content/shell/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
content/shell/test_runner/web_test_delegate.h:75: virtual void
PrintLogMessage(const std::string& message) = 0;
Here and elsewhere: The difference between "PrintMessage" and "PrintLogMessage"
is not clear from the method naming itself. Since the latter is specifically
about printing to stderr, what about "PrintStderrMessage", or even
"PrintErrorMessage"?

[Dan Elphick, 2017-03-09 21:18:28]

delphick@chromium.org changed reviewers:
+ dcheng@chromium.org

[Dan Elphick, 2017-03-09 21:18:31]

Adding Daniel for the IPC message change.

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
File content/shell/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
content/shell/test_runner/web_frame_test_client.cc:651: std::string
consoleMessage(std::string("CONSOLE ") + level + ": ");
On 2017/03/09 15:07:49, Peter Beverloo wrote:
> console_message

Done.

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
File content/shell/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/2678043002/diff/280001/content/shell/test_run...
content/shell/test_runner/web_test_delegate.h:75: virtual void
PrintLogMessage(const std::string& message) = 0;
On 2017/03/09 15:07:49, Peter Beverloo wrote:
> Here and elsewhere: The difference between "PrintMessage" and
"PrintLogMessage"
> is not clear from the method naming itself. Since the latter is specifically
> about printing to stderr, what about "PrintStderrMessage", or even
> "PrintErrorMessage"?

I wanted to avoid PrintErrorMessage because it sounds like an error where in
many cases these messages are informational. PrintStderrMessage is probably the
best we can do as it certainly avoids confusion.

[dcheng, 2017-03-09 21:38:36]

LGTM with a naming nit

https://codereview.chromium.org/2678043002/diff/320001/content/shell/test_run...
File content/shell/test_runner/web_test_delegate.h (right):

https://codereview.chromium.org/2678043002/diff/320001/content/shell/test_run...
content/shell/test_runner/web_test_delegate.h:75: virtual void
PrintStderrMessage(const std::string& message) = 0;
FWIW, I'd call this PrintMessageToStderr (along with all the related support
stuff)

[Dan Elphick, 2017-03-13 10:17:00]

The CQ bit was checked by delphick@chromium.org

[Dan Elphick, 2017-03-13 10:17:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org, chasej@chromium.org, peter@chromium.org,
rbyers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2678043002/#ps340001
(title: "Change method names to *MessageToStderr")

[commit-bot: I haz the power, 2017-03-13 10:17:17]

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

[Dan Elphick, 2017-03-13 11:46:18]

Description was changed from

==========
Hide console log messages for imported WPT tests

Move console output messages for WPT tests from the test output file into stderr
so they no longer affect whether the test passes or fails but can still be used
when debugging a test. Remove all CONSOLE lines from
external/wpt/**/*-expected.txt.

Change content_shell to now start every line of a multi-line log statement with
the "CONSOLE (MESSAGE|WARNING|ERROR):" prefix so that the above filter doesn't
leaving dangling console messages in the output.

Update tests in following directories to add a console.warn statement to prevent
the presubmit from trying to remove the -expected.txt file since these tests
rely on console messages to determine whether they pass/fail:

third_party/WebKit/LayoutTests/csspaint (logs are from worklets which have no
other way to communicate the test status)
third_party/WebKit/LayoutTests/http/tests/origin_trials/webexposed (chasej@ is
updating the tests so this is just a stopgap)

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=679742
==========

to

==========
Hide console log messages for imported WPT tests

Move console output messages for WPT tests from the test output file into stderr
so they no longer affect whether the test passes or fails but can still be used
when debugging a test. Remove all CONSOLE lines from
external/wpt/**/*-expected.txt.

Update tests in following directories to add a console.warn statement to prevent
the presubmit from trying to remove the -expected.txt file since these tests
rely on console messages to determine whether they pass/fail:

third_party/WebKit/LayoutTests/csspaint (logs are from worklets which have no
other way to communicate the test status)

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=679742
==========

[Dan Elphick, 2017-03-13 11:53:55]

The CQ bit was checked by delphick@chromium.org

[Dan Elphick, 2017-03-13 11:53:56]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
rbyers@chromium.org, chasej@chromium.org, dcheng@chromium.org,
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2678043002/#ps360001
(title: "update platform specific expected files missed last time around")

[commit-bot: I haz the power, 2017-03-13 11:54:11]

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, 2017-03-13 13:29:38]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1489406035251950,
"parent_rev": "2ac5618843f8e7b3044b9931c571d45aaf2915ca", "commit_rev":
"f8d3488e588458268f165f6e21e2e86bff0b9b6d"}

[commit-bot: I haz the power, 2017-03-13 13:30:23]

Description was changed from

==========
Hide console log messages for imported WPT tests

Move console output messages for WPT tests from the test output file into stderr
so they no longer affect whether the test passes or fails but can still be used
when debugging a test. Remove all CONSOLE lines from
external/wpt/**/*-expected.txt.

Update tests in following directories to add a console.warn statement to prevent
the presubmit from trying to remove the -expected.txt file since these tests
rely on console messages to determine whether they pass/fail:

third_party/WebKit/LayoutTests/csspaint (logs are from worklets which have no
other way to communicate the test status)

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=679742
==========

to

==========
Hide console log messages for imported WPT tests

Move console output messages for WPT tests from the test output file into stderr
so they no longer affect whether the test passes or fails but can still be used
when debugging a test. Remove all CONSOLE lines from
external/wpt/**/*-expected.txt.

Update tests in following directories to add a console.warn statement to prevent
the presubmit from trying to remove the -expected.txt file since these tests
rely on console messages to determine whether they pass/fail:

third_party/WebKit/LayoutTests/csspaint (logs are from worklets which have no
other way to communicate the test status)

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=679742

Review-Url: https://codereview.chromium.org/2678043002
Cr-Commit-Position: refs/heads/master@{#456362}
Committed:
https://chromium.googlesource.com/chromium/src/+/f8d3488e588458268f165f6e21e2...
==========

[commit-bot: I haz the power, 2017-03-13 13:30:25]

Committed patchset #23 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/f8d3488e588458268f165f6e21e2...