Report failed expect/assert to test launcher summary output.

Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=

Committed: https://crrev.com/961577e159e5cd9a8af365d322213cf86124914d
Cr-Commit-Position: refs/heads/master@{#441112}

[alex-ac, 2016-12-30 14:37:01]

Description was changed from

==========
Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=
==========

to

==========
Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=
==========

[alex-ac, 2016-12-30 14:42:05]

alex-ac@yandex-team.ru changed reviewers:
+ phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-12-30 19:02:40]

Thanks for the patch! Just minor comments.

https://codereview.chromium.org/2606153003/diff/20001/base/test/gtest_xml_uni...
File base/test/gtest_xml_unittest_result_printer.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/gtest_xml_uni...
base/test/gtest_xml_unittest_result_printer.cc:107: if (!NeedsEscaping(summary))
{
Shouldn't the condition be reversed?

By the way, it may be weird for clients to look for summary or summary-encoded.
Why not do proper XML escaping?

Actually, since this is internal, why not always base64-encode for simplicity?

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_result.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_result.cc:46: return "";
Should this return "unknown"?

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_result.h (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_result.h:27: TestResultPart(const TestResultPart&
other);
Why do we need these ctors/operators?

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_result.h:63: TestResult(const TestResult& other);
Why do we need these ctors/operators?

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_results_tracker.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_results_tracker.cc:389:
result_part_value->SetString("summary_base64", encoded_summary);
Could you follow pattern for "output_snippet"? Make the non-encoded key always
set, and make it point to encoded one when needed. Same below for message.

[alex-ac, 2016-12-30 22:10:20]

https://codereview.chromium.org/2606153003/diff/20001/base/test/gtest_xml_uni...
File base/test/gtest_xml_unittest_result_printer.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/gtest_xml_uni...
base/test/gtest_xml_unittest_result_printer.cc:107: if (!NeedsEscaping(summary))
{
On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> Shouldn't the condition be reversed?
> 
> By the way, it may be weird for clients to look for summary or
summary-encoded.
> Why not do proper XML escaping?
> 
> Actually, since this is internal, why not always base64-encode for simplicity?

I've deleted condition and now text is encoded in base64 always.

There are too hard to make proper XML escaping here, because there is no ready
to use escaping code either in base or in libxml. There is xml writer in the
libxml chromium utilities, but it can't be used here, because it uses buffering
and file will be empty if one of tests will crash.

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_result.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_result.cc:46: return "";
On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> Should this return "unknown"?

This is NOTREACHED() case, and i assume will always work in tests.
I've changed it to "unknown".

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_result.h (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_result.h:27: TestResultPart(const TestResultPart&
other);
On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> Why do we need these ctors/operators?

chromium-style clang plugin makes a warning about complex structure without
constructor/destructor. It also makes warning about copying of complex structure
without explicitly defined copy constructor.

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_result.h:63: TestResult(const TestResult& other);
On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> Why do we need these ctors/operators?

This is the same case (it seems that field of type vector triggers this
warning).

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_results_tracker.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_results_tracker.cc:389:
result_part_value->SetString("summary_base64", encoded_summary);
On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> Could you follow pattern for "output_snippet"? Make the non-encoded key always
> set, and make it point to encoded one when needed. Same below for message.

Done.

[Paweł Hajdan Jr., 2017-01-02 09:21:37]

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_results_tracker.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_results_tracker.cc:389:
result_part_value->SetString("summary_base64", encoded_summary);
On 2016/12/30 22:10:20, alex-ac wrote:
> On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> > Could you follow pattern for "output_snippet"? Make the non-encoded key
always
> > set, and make it point to encoded one when needed. Same below for message.
> 
> Done.

Please address  the "make the non-encoded key always set" part of my comment. It
seems better for "summary" to always be present, similar to code above which I
pointed to:

<non-UTF-8 snippet, see output_snippet_base64> (change text accordingly)

https://codereview.chromium.org/2606153003/diff/40001/base/test/launcher/test...
File base/test/launcher/test_results_tracker.cc (right):

https://codereview.chromium.org/2606153003/diff/40001/base/test/launcher/test...
base/test/launcher/test_results_tracker.cc:393:
result_part_value->SetString("message", result_part.message);
Same here.

[alex-ac, 2017-01-03 08:28:59]

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
File base/test/launcher/test_results_tracker.cc (right):

https://codereview.chromium.org/2606153003/diff/20001/base/test/launcher/test...
base/test/launcher/test_results_tracker.cc:389:
result_part_value->SetString("summary_base64", encoded_summary);
On 2017/01/02 09:21:37, Paweł Hajdan Jr. wrote:
> On 2016/12/30 22:10:20, alex-ac wrote:
> > On 2016/12/30 19:02:39, Paweł Hajdan Jr. wrote:
> > > Could you follow pattern for "output_snippet"? Make the non-encoded key
> always
> > > set, and make it point to encoded one when needed. Same below for message.
> > 
> > Done.
> 
> Please address  the "make the non-encoded key always set" part of my comment.
It
> seems better for "summary" to always be present, similar to code above which I
> pointed to:
> 
> <non-UTF-8 snippet, see output_snippet_base64> (change text accordingly)

Done.

https://codereview.chromium.org/2606153003/diff/40001/base/test/launcher/test...
File base/test/launcher/test_results_tracker.cc (right):

https://codereview.chromium.org/2606153003/diff/40001/base/test/launcher/test...
base/test/launcher/test_results_tracker.cc:393:
result_part_value->SetString("message", result_part.message);
On 2017/01/02 09:21:37, Paweł Hajdan Jr. wrote:
> Same here.

Done.

[Paweł Hajdan Jr., 2017-01-03 09:03:03]

LGTM

[alex-ac, 2017-01-03 12:54:27]

The CQ bit was checked by alex-ac@yandex-team.ru

[commit-bot: I haz the power, 2017-01-03 12:54:39]

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-01-03 13:47:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483448067967660,
"parent_rev": "6dfa6dd3c168ab325e77d88e8e5c348937e08308", "commit_rev":
"f9fc6b7cbdda374e2640910d2815fead6603cf6e"}

[commit-bot: I haz the power, 2017-01-03 13:47:43]

Description was changed from

==========
Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=
==========

to

==========
Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=

Review-Url: https://codereview.chromium.org/2606153003
==========

[commit-bot: I haz the power, 2017-01-03 13:47:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-03 13:49:48]

Description was changed from

==========
Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=

Review-Url: https://codereview.chromium.org/2606153003
==========

to

==========
Report failed expect/assert to test launcher summary output.

Knowledge of concrete reason of test failure makes posible to properly
group broken/flacky tests to create issues. This CL gathers failed gtest
EXPECT_/ASSERT_* statements, and reports them to test launcher summary
output json.

This CL is continuation of previous one:
https://codereview.chromium.org/2502953002/

I created a temporary test with single line EXPECT_TRUE(false); and there
are some relevant lines from the summary: http://pastebin.com/vFMuADEX

BUG=

Committed: https://crrev.com/961577e159e5cd9a8af365d322213cf86124914d
Cr-Commit-Position: refs/heads/master@{#441112}
==========

[commit-bot: I haz the power, 2017-01-03 13:49:49]

Patchset 4 (id:??) landed as
https://crrev.com/961577e159e5cd9a8af365d322213cf86124914d
Cr-Commit-Position: refs/heads/master@{#441112}