This change modifies testharnessreport

testharnessreport: Print test results even if a test didn't complete.

BUG=618444
Committed: https://crrev.com/efc66f4fe0e3cbc13539db5b2bd47f042d618605
Cr-Commit-Position: refs/heads/master@{#399245}

[atotic1, 2016-06-08 21:06:17]

atotic@google.com changed reviewers:
+ tkent@chromium.org

[atotic1, 2016-06-08 21:06:17]

Hi Kent,
found your name in git blame fro testharnessreport.js. If you are not the right
guy to review this, do you know someone who is?

This is a very small change, it just removes an else guard. 
All it does is show test results instead of suppressing them when test fails.

I've discovered this bug when developing a test suite. One of my async
tests was timing out, and not seeing which one of the tests 
was failing was slowing me down.

With this fix, I was able to see what tests were failing right away.

[jsbell, 2016-06-08 21:12:19]

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

[jsbell, 2016-06-08 21:12:19]

While chromium (blink) has its own copy of testharnessreport.js to get the
output we need for layout tests. The imported one shouldn't be used when running
layout tests. The imported file should not be modified.

We could modify the local one with this change, but I'm not sure we want to
diverge here. (But I don't feel strongly.) If it's a general improvement, I'd
try proposing the change upstream (i.e. https://github.com/w3c/testharness.js)

https://codereview.chromium.org/2052643002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js
(left):

https://codereview.chromium.org/2052643002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js:94: }
else {
This file is imported, and should not be changed directly in the chromium
repository.

If a change is necessary, it needs to be made at:

https://github.com/w3c/testharness.js/

[jsbell, 2016-06-08 21:12:22]

While chromium (blink) has its own copy of testharnessreport.js to get the
output we need for layout tests. The imported one shouldn't be used when running
layout tests. The imported file should not be modified.

We could modify the local one with this change, but I'm not sure we want to
diverge here. (But I don't feel strongly.) If it's a general improvement, I'd
try proposing the change upstream (i.e. https://github.com/w3c/testharness.js)

[atotic1, 2016-06-08 21:55:51]

On 2016/06/08 21:12:22, jsbell wrote:
> While chromium (blink) has its own copy of testharnessreport.js to get the
> output we need for layout tests. The imported one shouldn't be used when
running
> layout tests. The imported file should not be modified.
> 
> We could modify the local one with this change, but I'm not sure we want to
> diverge here. (But I don't feel strongly.) If it's a general improvement, I'd
> try proposing the change upstream (i.e. https://github.com/w3c/testharness.js)

I've already looked at
https://github.com/w3c/testharness.js/blob/master/testharnessreport.js
Our version differs a lot, can barely tell it is the same file.
The upstream version does not contain this code, so I can't merge there.

[jsbell, 2016-06-08 22:00:28]

On 2016/06/08 21:55:51, atotic1 wrote:
> On 2016/06/08 21:12:22, jsbell wrote:
> > While chromium (blink) has its own copy of testharnessreport.js to get the
> > output we need for layout tests. The imported one shouldn't be used when
> running
> > layout tests. The imported file should not be modified.
> > 
> > We could modify the local one with this change, but I'm not sure we want to
> > diverge here. (But I don't feel strongly.) If it's a general improvement,
I'd
> > try proposing the change upstream (i.e.
https://github.com/w3c/testharness.js)
> 
> I've already looked at
> https://github.com/w3c/testharness.js/blob/master/testharnessreport.js
> Our version differs a lot, can barely tell it is the same file.
> The upstream version does not contain this code, so I can't merge there.

Oh, that's right - this is one of the cases where we *overwrite* the imported
file. Very sorry about that - ignore my previous comments. :(

...

Can you give an example of the output before/after your change? I assume it's
just a set of PASS/FAIL lines after the generic "Harness Error.
harness_status.status = ..." header?

[atotic1, 2016-06-08 22:16:53]

On 2016/06/08 22:00:28, jsbell wrote:
> Can you give an example of the output before/after your change? I assume it's
> just a set of PASS/FAIL lines after the generic "Harness Error.
> harness_status.status = ..." header?

I've added screenshots to the bug #618444

[jsbell, 2016-06-08 22:48:23]

Thanks for your patience. I agree this will make debugging failures easier! I
double-checked the testharness_results.py code to ensure this would still count
as failure.

lgtm

[tkent, 2016-06-08 23:05:18]

Description was changed from

==========
This change modifies testharnessreport
so that test results are printed on failure.

BUG=618444
==========

to

==========
testharnessreport: Print test results even if a test didn't complete.

BUG=618444
==========

[tkent, 2016-06-08 23:05:40]

lgtm.

I updated the CL description.

[atotic1, 2016-06-10 00:33:04]

The CQ bit was checked by atotic@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-10 00:34:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052643002/20001

[commit-bot: I haz the power, 2016-06-10 02:35:23]

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

[commit-bot: I haz the power, 2016-06-10 02:35:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[atotic1, 2016-06-10 17:29:22]

The trybots failed because 4 failing test expectations did not match up.

I've patched the failing tests and added them to the new patch. Love seeing
those useful error messages.

Are you guys ok with committing the new patch?

[jsbell, 2016-06-10 18:54:31]

yep, still lgtm!

[atotic1, 2016-06-10 18:59:16]

The CQ bit was checked by atotic@google.com

[atotic1, 2016-06-10 18:59:17]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2052643002/#ps20001
(title: "Fix failing tests")

[commit-bot: I haz the power, 2016-06-10 18:59:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052643002/20001

[commit-bot: I haz the power, 2016-06-10 19:04:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-10 19:04:16]

CQ bit was unchecked

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

Description was changed from

==========
testharnessreport: Print test results even if a test didn't complete.

BUG=618444
==========

to

==========
testharnessreport: Print test results even if a test didn't complete.

BUG=618444

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

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

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