[test.dart] Complain if there is non-utf8 formatted data in test output

[test.dart] Complain if there is non-utf8 formatted data in test output

related #28872

[zra, 2017-02-23 18:34:54]

zra@google.com changed reviewers:
+ fschneider@google.com, kustermann@google.com

[ahe, 2017-02-23 18:46:20]

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

[ahe, 2017-02-23 18:46:21]

lgtm

https://codereview.chromium.org/2710003005/diff/1/tools/testing/dart/test_run...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2710003005/diff/1/tools/testing/dart/test_run...
tools/testing/dart/test_runner.dart:1803: hasNonUtf8 = true;
How about:

String malformed = UTF8.decode(data, allowMalformed: true);
data
    ..clear()
    ..addAll(malformed.codeUnits)
    ..addAll("""
...

[zra, 2017-02-23 21:24:50]

https://codereview.chromium.org/2710003005/diff/1/tools/testing/dart/test_run...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2710003005/diff/1/tools/testing/dart/test_run...
tools/testing/dart/test_runner.dart:1803: hasNonUtf8 = true;
On 2017/02/23 18:46:20, ahe wrote:
> How about:
> 
> String malformed = UTF8.decode(data, allowMalformed: true);
> data
>     ..clear()
>     ..addAll(malformed.codeUnits)
>     ..addAll("""
> ...
> 

Done.

[kustermann, 2017-02-24 16:24:17]

Would you mind explaining what this is for? If we have tests which output
non-utf8 data we should fix the tests (since we control them).

Tests which wan to exactly test non-utf8 output can do so via a subprocess.

This adds really a lot of code and I don't understand why we should have it.

https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:1806: ..addAll(malformed.codeUnits)
The input 'List<int> data' here is a list of bytes (i.e. the entries are 0 <=
value <= 255).

Here you add codeUnits, which are not bytes but rather UTF16 code points (as far
as I know).

The places where this list is used use UTF8.decode(data) which assumes [data]
contains only bytes / 8-bit values.


(Maybe this should have been written using a BytesBuilder and Utf8List instead
to make this more clear and avoid having these growable Lists with Smis in
them.)

[zra, 2017-02-24 17:50:03]

On 2017/02/24 16:24:17, kustermann wrote:
> Would you mind explaining what this is for? If we have tests which output
> non-utf8 data we should fix the tests (since we control them).

Prior to this CL, tests emitting non-utf8 formatted data would succeed when they
should have failed. Or rather, they would only fail on Android, when they should
have failed everywhere.

> Tests which wan to exactly test non-utf8 output can do so via a subprocess.

Yes, they still can...

> This adds really a lot of code and I don't understand why we should have it.

Really? I find this change to be rather unobtrusive. It's less than 100 lines...

>
https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
> tools/testing/dart/test_runner.dart:1806: ..addAll(malformed.codeUnits)
> The input 'List<int> data' here is a list of bytes (i.e. the entries are 0 <=
> value <= 255).
> 
> Here you add codeUnits, which are not bytes but rather UTF16 code points (as
far
> as I know).
> 
> The places where this list is used use UTF8.decode(data) which assumes [data]
> contains only bytes / 8-bit values.
> 
> 
> (Maybe this should have been written using a BytesBuilder and Utf8List instead
> to make this more clear and avoid having these growable Lists with Smis in
> them.)

Just took Peter's suggestion for this. The goal is to replace non-utf8 data with
<?>(0xfffd) that we get from UTF8.decode. Any thoughts about how to retain that?

[ahe, 2017-02-25 10:42:07]

On 2017/02/24 17:50:03, zra wrote:
> On 2017/02/24 16:24:17, kustermann wrote:
> > Would you mind explaining what this is for? If we have tests which output
> > non-utf8 data we should fix the tests (since we control them).
> 
> Prior to this CL, tests emitting non-utf8 formatted data would succeed when
they
> should have failed. Or rather, they would only fail on Android, when they
should
> have failed everywhere.
> 
> > Tests which wan to exactly test non-utf8 output can do so via a subprocess.
> 
> Yes, they still can...
> 
> > This adds really a lot of code and I don't understand why we should have it.
> 
> Really? I find this change to be rather unobtrusive. It's less than 100
lines...
> 
> >
>
https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
> > File tools/testing/dart/test_runner.dart (right):
> > 
> >
>
https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
> > tools/testing/dart/test_runner.dart:1806: ..addAll(malformed.codeUnits)
> > The input 'List<int> data' here is a list of bytes (i.e. the entries are 0
<=
> > value <= 255).
> > 
> > Here you add codeUnits, which are not bytes but rather UTF16 code points (as
> far
> > as I know).
> > 
> > The places where this list is used use UTF8.decode(data) which assumes
[data]
> > contains only bytes / 8-bit values.
> > 
> > 
> > (Maybe this should have been written using a BytesBuilder and Utf8List
instead
> > to make this more clear and avoid having these growable Lists with Smis in
> > them.)
> 
> Just took Peter's suggestion for this. The goal is to replace non-utf8 data
with
> <?>(0xfffd) that we get from UTF8.decode. Any thoughts about how to retain
that?

I got confused about codePoints. They work on the string literal you used
because US-ASCII is the same in UTF-8 and UTF-16. It needs to be
UTF8.encode(malformed), I think.

[ahe, 2017-02-25 10:44:57]

On 2017/02/24 17:50:03, zra wrote:
> On 2017/02/24 16:24:17, kustermann wrote:
> > Would you mind explaining what this is for? If we have tests which output
> > non-utf8 data we should fix the tests (since we control them).
> 
> Prior to this CL, tests emitting non-utf8 formatted data would succeed when
they
> should have failed. Or rather, they would only fail on Android, when they
should
> have failed everywhere.
> 
> > Tests which wan to exactly test non-utf8 output can do so via a subprocess.
> 
> Yes, they still can...
> 
> > This adds really a lot of code and I don't understand why we should have it.
> 
> Really? I find this change to be rather unobtrusive. It's less than 100
lines...

Given how confusing it is to debug non-UTF8 output from tests, I think Zach's
changes are paying for themselves.

Without these changes, the test runner would simply crash, and I only guessed
which test it was due to the blame list on the build bot.

[kustermann, 2017-02-25 11:37:48]

Since you two agree, I suggest you just go ahead and land it.


> > This adds really a lot of code and I don't understand why we should have it.
> Really? I find this change to be rather unobtrusive. It's less than 100
lines...

I understand that it looks simple, but personally I'm very cautious about adding
any more code to the testing scripts, because there is already so much
complexity/convoluted code in it. Sometimes we just need to change test.dart,
but this CL seemed to me not solving a real problem. But maybe I'm just wrong.

Would a normal negative test which outputs non-utf8 now fail with thew new
marker or would it pass? What about a multitest negative test? I can only answer
these questions by reading through multitest.dart, test_suite.dart and finally
the if/else cascade in test_runner.dart.

Also some of the lines you added we normally cannot hit (e.g. the browser sends
only string data back, so we can never have invalid utf8 in the outputs of these
tests). The analyzer we run normally via a string-based batch mode, which also
means we cannot have utf8 output in test stdout (test.dart might still crash
with a utf8 decoding error, but at an earlier place instead).

https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:1806: ..addAll(malformed.codeUnits)
On 2017/02/24 16:24:17, kustermann wrote:
> The input 'List<int> data' here is a list of bytes (i.e. the entries are 0 <=
> value <= 255).
> 
> Here you add codeUnits, which are not bytes but rather UTF16 code points (as
far
> as I know).
> 
> The places where this list is used use UTF8.decode(data) which assumes [data]
> contains only bytes / 8-bit values.
> 
> 
> (Maybe this should have been written using a BytesBuilder and Utf8List instead
> to make this more clear and avoid having these growable Lists with Smis in
> them.)

To clarify, what I mean is changing:

  data..clear()..addAll(malformed.codeUnits)

to 

  data..clear()..addAll(UTF8.encode(malformed))


Because later on code will do

UTF8.decode(testCase.lastCommandOutput.stdout /* = data */ )

and UTF8.decode() takes only bytes, not UTF16 code points.

[zra, 2017-02-27 19:05:58]

This change landed, but the codereview site didn't catch it because of the
authentication problems last week.

https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:1806: ..addAll(malformed.codeUnits)
On 2017/02/25 11:37:48, kustermann wrote:
> On 2017/02/24 16:24:17, kustermann wrote:
> > The input 'List<int> data' here is a list of bytes (i.e. the entries are 0
<=
> > value <= 255).
> > 
> > Here you add codeUnits, which are not bytes but rather UTF16 code points (as
> far
> > as I know).
> > 
> > The places where this list is used use UTF8.decode(data) which assumes
[data]
> > contains only bytes / 8-bit values.
> > 
> > 
> > (Maybe this should have been written using a BytesBuilder and Utf8List
instead
> > to make this more clear and avoid having these growable Lists with Smis in
> > them.)
> 
> To clarify, what I mean is changing:
> 
>   data..clear()..addAll(malformed.codeUnits)
> 
> to 
> 
>   data..clear()..addAll(UTF8.encode(malformed))
> 
> 
> Because later on code will do
> 
> UTF8.decode(testCase.lastCommandOutput.stdout /* = data */ )
> 
> and UTF8.decode() takes only bytes, not UTF16 code points.

Making this fix here: https://codereview.chromium.org/2721683002/

[Florian Schneider, 2017-03-02 22:57:46]

On 2017/02/27 19:05:58, zra wrote:
> This change landed, but the codereview site didn't catch it because of the
> authentication problems last week.
> 

dbc: You can manually close the rietveld issue in "Edit issue".

>
https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://codereview.chromium.org/2710003005/diff/40001/tools/testing/dart/test...
> tools/testing/dart/test_runner.dart:1806: ..addAll(malformed.codeUnits)
> On 2017/02/25 11:37:48, kustermann wrote:
> > On 2017/02/24 16:24:17, kustermann wrote:
> > > The input 'List<int> data' here is a list of bytes (i.e. the entries are 0
> <=
> > > value <= 255).
> > > 
> > > Here you add codeUnits, which are not bytes but rather UTF16 code points
(as
> > far
> > > as I know).
> > > 
> > > The places where this list is used use UTF8.decode(data) which assumes
> [data]
> > > contains only bytes / 8-bit values.
> > > 
> > > 
> > > (Maybe this should have been written using a BytesBuilder and Utf8List
> instead
> > > to make this more clear and avoid having these growable Lists with Smis in
> > > them.)
> > 
> > To clarify, what I mean is changing:
> > 
> >   data..clear()..addAll(malformed.codeUnits)
> > 
> > to 
> > 
> >   data..clear()..addAll(UTF8.encode(malformed))
> > 
> > 
> > Because later on code will do
> > 
> > UTF8.decode(testCase.lastCommandOutput.stdout /* = data */ )
> > 
> > and UTF8.decode() takes only bytes, not UTF16 code points.
> 
> Making this fix here: https://codereview.chromium.org/2721683002/

[zra, 2017-03-03 18:01:30]

Description was changed from

==========
[test.dart] Complain if there is non-utf8 formatted data in test output

related #28872
==========

to

==========
[test.dart] Complain if there is non-utf8 formatted data in test output

related #28872
==========