base/test/launcher: don't trim leading whitespace from test output

base/test/launcher: Don't trim leading whitespace from test output
base/strings: Remove an unnecessary UTF8 check from one of the trim functions

gtest's message often use leading whitespace in a way that's visually meaningful. Some examples:

======== EXPECT_EQ(this, this+1);
my_test.cc(2048): error: Value of: this+1
  Actual: 24F8B8D4
Expected: this
Which is: 24F8B8A0
========
======== EXPECT_EQ("AA\nBB\nDD", std::string("AA\nXX\nDD"));
my_test.cc(2049): error: Value of: std::string("AA\nXX\nDD")
  Actual: "AA\nXX\nDD"
Expected: "AA\nBB\nDD"
With diff:
@@ -1,3 +1,3 @@
 AA
-BB
+XX
 DD
========

Today leading whitespaces are stripped out by the test launcher, leading to less readable test stdout on the bots (and headaches, especially, if the 'AA' value in the example above had leading whitespace itself -- which is how I noticed this).

The current whitespace trimming seems to be an accidental effect of [https://codereview.chromium.org/324893004], so it should be safe to change back.

BUG=475265
Committed: https://crrev.com/0fbbee21cbea17a544c30b6d5e769f9e4ba8ad44
Cr-Commit-Position: refs/heads/master@{#325512}

[ncarter (slow), 2015-04-10 22:32:01]

nick@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[ncarter (slow), 2015-04-10 22:32:01]

Pawel, please review! Thanks.

[Paweł Hajdan Jr., 2015-04-13 15:56:35]

LGTM, thanks!

[ncarter (slow), 2015-04-13 17:22:36]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-04-13 17:22:54]

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

[commit-bot: I haz the power, 2015-04-14 00:15:38]

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

[commit-bot: I haz the power, 2015-04-14 00:15:39]

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

[ncarter (slow), 2015-04-14 17:42:11]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-04-14 17:43:13]

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

[commit-bot: I haz the power, 2015-04-15 00:00:11]

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

[commit-bot: I haz the power, 2015-04-15 00:00:12]

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

[its_nick_at_chromium_org, 2015-04-15 03:06:26]

ncarter@google.com changed reviewers:
+ ncarter@google.com

[its_nick_at_chromium_org, 2015-04-15 03:06:26]

Pawel, any idea what's going on with the cq failure on this patch? I think it
might be a real failure (the formatting change broke some parse of the output
results later on) but I haven't connected the dots.

[ncarter (slow), 2015-04-15 16:47:23]

Nevermind -- I think I see what's happening now; we're hitting a dcheck in
SplitStringDontTrim, and I'll figure it out.

[ncarter (slow), 2015-04-15 17:28:22]

thakis: please take a look

I've removed the IsUTF8 check here after determining it to be unnecessary.
Whether it's extended ascii or UTF-8, we can strip whitespace and split along
newlines all the same. SplitString and SplitStringDontTrim share an
implementation, so what's valid input for the former should be valid for the
latter.

At least test on the linux asan bot generates stdio which is not valid UTF-8,
though I don't know why this is.

Archaeological research shows this DCHECK to have been introduced during a
file-move of this function, as part of https://codereview.chromium.org/3366011 .

[ncarter (slow), 2015-04-15 17:30:29]

nick@chromium.org changed reviewers:
+ thakis@chromium.org

[ncarter (slow), 2015-04-16 16:57:33]

nico: ping

[Nico, 2015-04-16 17:05:24]

This was the first time this showed up in my mailbox. Maybe you didn't add me to
reviewers until the ping?

The test launcher change looks fine, the split change doesn't.

https://codereview.chromium.org/1081493002/diff/20001/base/strings/string_spl...
File base/strings/string_split.cc (left):

https://codereview.chromium.org/1081493002/diff/20001/base/strings/string_spl...
base/strings/string_split.cc:196: DCHECK(IsStringUTF8(str));
This change looks both unrelated and incorrect. (`c` is a single byte, so
this'll only work for utf-8)

[ncarter (slow), 2015-04-16 18:08:01]

https://codereview.chromium.org/1081493002/diff/20001/base/strings/string_spl...
File base/strings/string_split.cc (left):

https://codereview.chromium.org/1081493002/diff/20001/base/strings/string_spl...
base/strings/string_split.cc:196: DCHECK(IsStringUTF8(str));
On 2015/04/16 17:05:24, Nico wrote:
> This change looks both unrelated and incorrect. (`c` is a single byte, so
> this'll only work for utf-8)

"only work for UTF-8" as opposed to what?

Did you see my message where I explained that it's both related and correct?
Here's what I wrote:

===

I've removed the IsUTF8 check here after determining it to be unnecessary.
Whether it's extended ascii or UTF-8, we can strip whitespace and split along
newlines all the same. SplitString and SplitStringDontTrim share an
implementation, so what's valid input for the former should be valid for the
latter.

At least test on the linux asan bot generates stdio which is not valid UTF-8,
though I don't know why this is.

Archaeological research shows this DCHECK to have been introduced during a
file-move of this function, as part of https://codereview.chromium.org/3366011 .

===

[ncarter (slow), 2015-04-16 18:08:50]

https://codereview.chromium.org/1081493002/diff/20001/base/strings/string_spl...
File base/strings/string_split.cc (left):

https://codereview.chromium.org/1081493002/diff/20001/base/strings/string_spl...
base/strings/string_split.cc:196: DCHECK(IsStringUTF8(str));
On 2015/04/16 18:08:00, ncarter wrote:
> On 2015/04/16 17:05:24, Nico wrote:
> > This change looks both unrelated and incorrect. (`c` is a single byte, so
> > this'll only work for utf-8)
> 
> "only work for UTF-8" as opposed to what?
> 
> Did you see my message where I explained that it's both related and correct?
> Here's what I wrote:
> 
> ===
> 
> I've removed the IsUTF8 check here after determining it to be unnecessary.
> Whether it's extended ascii or UTF-8, we can strip whitespace and split along
> newlines all the same. SplitString and SplitStringDontTrim share an
> implementation, so what's valid input for the former should be valid for the
> latter.
> 
> At least test on the linux asan bot generates stdio which is not valid UTF-8,
> though I don't know why this is.
> 
> Archaeological research shows this DCHECK to have been introduced during a
> file-move of this function, as part of https://codereview.chromium.org/3366011
.
> 
> ===

And oh, to understand the relationship: see the failed tryjob on patchset 1.

[Nico, 2015-04-16 18:22:04]

On Thu, Apr 16, 2015 at 11:08 AM, <nick@chromium.org> wrote:

>
> https://codereview.chromium.org/1081493002/diff/20001/
> base/strings/string_split.cc
> File base/strings/string_split.cc (left):
>
> https://codereview.chromium.org/1081493002/diff/20001/
> base/strings/string_split.cc#oldcode196
> base/strings/string_split.cc:196: DCHECK(IsStringUTF8(str));
> On 2015/04/16 17:05:24, Nico wrote:
>
>> This change looks both unrelated and incorrect. (`c` is a single byte,
>>
> so
>
>> this'll only work for utf-8)
>>
>
> "only work for UTF-8" as opposed to what?
>

utf-16…oh, this takes a string and isn't a template function. Nevermind
then, lgtm.


>
> Did you see my message where I explained that it's both related and
> correct?


No. As I said, all I got for this review was "nico: ping".


> Here's what I wrote:
>
> ===
>
> I've removed the IsUTF8 check here after determining it to be
> unnecessary.
> Whether it's extended ascii or UTF-8, we can strip whitespace and split
> along
> newlines all the same. SplitString and SplitStringDontTrim share an
> implementation, so what's valid input for the former should be valid for
> the
> latter.
>
> At least test on the linux asan bot generates stdio which is not valid
> UTF-8,
> though I don't know why this is.
>
> Archaeological research shows this DCHECK to have been introduced during
> a
> file-move of this function, as part of
> https://codereview.chromium.org/3366011 .
>

:-(


>
> ===
>
> https://codereview.chromium.org/1081493002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-04-16 18:22:20]

lgtm from rietveld too

[ncarter (slow), 2015-04-16 18:26:08]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-04-16 18:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1081493002/#ps20001
(title: "Remove UTF8 DCHECK from SplitStringDontTrim")

[commit-bot: I haz the power, 2015-04-16 18:26:42]

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

[commit-bot: I haz the power, 2015-04-16 20:58:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-16 20:59:32]

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