Fixed utf8 encoding/decoding issues in the testing scripts

Fixed utf8 encoding/decoding issues in the testing scripts

Committed: https://code.google.com/p/dart/source/detail?r=17057

[kustermann, 2013-01-15 10:20:41]

On 2013/01/15 10:15:48, kustermann wrote:

There are a few places left, where we do "somestring".charCodes. But the strings
in these places are always ascii, so it doesn't make a difference there (and
charCodes is probably a bit faster than encodeUtf8).

[ricow1, 2013-01-15 10:25:37]

LGTM, as discussed offline we should get some input from Mads if this is ever an
issue on windows (adding him as reviewer)

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:71: bytes = null;
why do you do this?

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1315: bytes = null;
again why do you set this to null?

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/utils.dart
File tools/testing/dart/utils.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/utils.d...
tools/testing/dart/utils.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
2013

[kustermann, 2013-01-15 10:31:01]

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:71: bytes = null;
On 2013/01/15 10:25:37, ricow1 wrote:
> why do you do this?

It was there before ('chars = null') so I left it. I think peter put this in
because it allows the GC to collect the data even if the function hasn't ended.
On the other hand, the VM should be smart enough to figure out that the variable
is no longer referenced and not include it in the root-set?!

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1315: bytes = null;
On 2013/01/15 10:25:37, ricow1 wrote:
> again why do you set this to null?
See my other comment.

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/utils.dart
File tools/testing/dart/utils.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/utils.d...
tools/testing/dart/utils.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2013/01/15 10:25:37, ricow1 wrote:
> 2013

Done.

[ricow1, 2013-01-15 10:33:00]

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:71: bytes = null;
On 2013/01/15 10:31:01, kustermann wrote:
> On 2013/01/15 10:25:37, ricow1 wrote:
> > why do you do this?
> 
> It was there before ('chars = null') so I left it. I think peter put this in
> because it allows the GC to collect the data even if the function hasn't
ended.
> On the other hand, the VM should be smart enough to figure out that the
variable
> is no longer referenced and not include it in the root-set?!
I don't expect it to be that clever actually, it will still be alive here. Why
did you move it down here?

[ahe, 2013-01-15 10:36:18]

DBC

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:63: String contents = decodeUtf8(bytes);
I would write this as:

decodeUtf8(new File.fromPath(filePath).readAsBytesSync());

[Mads Ager (google), 2013-01-15 10:36:53]

There are a number of commands from which we can get output here. I don't think
we are guaranteed that they are all utf-8. In particular, we can write raw bytes
to the output using stdout.writeList in dart:io. Attempting to utf-8 decode that
will sometimes fail. Maybe what we really need is a little safe utf-8 decoding
library?

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/test_pr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/test_pr...
tools/testing/dart/test_progress.dart:192:
output.add(decodeUtf8(test.lastCommandOutput.stdout));
Which commands end up here? We know that the output generated with 'print' from
a dart script is utf8. However, if these commands can be other things (or dart
scripts that print raw bytes) we have no idea whether or not this is utf8
encoded.

Maybe we should be even more defensive here and provide out own output decoding
library as part of test.dart which will deal with decoding failures (by putting
in markers in the output?).

[Mads Ager (google), 2013-01-15 10:52:59]

LGTM

Ignore my previous comments. As you showed me offline you are using the utf
library which does put in markers for invalid utf8 sequences. :)

[kustermann, 2013-01-15 12:22:10]

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:63: String contents = decodeUtf8(bytes);
On 2013/01/15 10:36:19, ahe wrote:
> I would write this as:
> 
> decodeUtf8(new File.fromPath(filePath).readAsBytesSync());
I saw your comment too late, sorry.

https://codereview.chromium.org/11883033/diff/2001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:71: bytes = null;
On 2013/01/15 10:33:00, ricow1 wrote:
> On 2013/01/15 10:31:01, kustermann wrote:
> > On 2013/01/15 10:25:37, ricow1 wrote:
> > > why do you do this?
> > 
> > It was there before ('chars = null') so I left it. I think peter put this in
> > because it allows the GC to collect the data even if the function hasn't
> ended.
> > On the other hand, the VM should be smart enough to figure out that the
> variable
> > is no longer referenced and not include it in the root-set?!
> I don't expect it to be that clever actually, it will still be alive here. Why
> did you move it down here?
> 
Just by accident, but this way 'bytes = null' and 'contents = null' are in one
place. It really shouldn't matter anyway, so I'll leave it.