Add stacktrace_test

Add stacktrace_test

- also add helper program for working with sourcemaps.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/d153808ca647c0c4e9484b903848851b4d314b50

[Johnni Winther, 2016-10-20 13:49:45]

Description was changed from

==========
Add stack_trace_test

- also add helper program for working with sourcemaps.
==========

to

==========
Add stacktrace_test

- also add helper program for working with sourcemaps.
==========

[Johnni Winther, 2016-10-20 13:49:54]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Johnni Winther, 2016-10-20 13:49:55]

Please test stacktrace_test this for a spin on Linux/MacOS

[Johnni Winther, 2016-10-24 08:22:06]

ping

[Siggi Cherem (dart-lang), 2016-10-27 13:25:19]

sorry - I thought I had sent these comments earlier, seems I forgot to publish.

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/lazon.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/lazon.dart:11: library lazon;
funny name, but it might help to rename this library (and file) to some
descriptive name like "json_with_comments"or "commented_json"or "lazy_json".

I might prefer the former names, since lazy makes me think that the decoder
below is lazily loading code little by little, while in reality it's pretty
eager.

A crazy alternative: would it be possible to remove the comments with some
regexp and then use the standard json decoder?

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:85: while (index <
code.length) {
the code here to compute start-lines seems very similar to the logic we use in
`SourceFile` from package:source_span, maybe you can directly use SourceFile
here instead?

(https://github.com/dart-lang/source_span/blob/master/lib/src/file.dart#L22)

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:88: case 0x0A:
can you define a couple const variables to give these a name?

const _LF = 0x0A;
const _CR = 0x0D; ?

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:161:
Process.runSync(d8executable, [output]);
when running test.py, are we guaranteed that the current working directory is
the sdk-root? (so that the relative path from d8executable exists)?

What do you think about detecting the sdk-root, so that you can easily run this
test with test.py, or directly in dart from either the sdk-root or the folder
where it is at?

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:227: factory
StackTraceLine.fromText(String text) {
it might help to add a dartdoc with an example stack trace line to document what
kind of line you are parsing below.

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:240: int lastColon =
text.lastIndexOf(':');
idea: consider splitting text by ':' first, then the case analysis below might
get collapsed/simplified?

var segments = text.split(':');
var fileName = segments[0];
var lineNo = (segments.length > 1) ? int.parse(segments[1], onError: (_) =>
null) : null;
var columnNo = (segments.length > 2) ? int.parse(segments[2], onError: (_) =>
null): null;

[Johnni Winther, 2016-11-02 14:34:31]

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/lazon.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/lazon.dart:11: library lazon;
On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> funny name, but it might help to rename this library (and file) to some
> descriptive name like "json_with_comments"or "commented_json"or "lazy_json".
> 
> I might prefer the former names, since lazy makes me think that the decoder
> below is lazily loading code little by little, while in reality it's pretty
> eager.
> 
> A crazy alternative: would it be possible to remove the comments with some
> regexp and then use the standard json decoder?

How about lax_json (or laxon), it also supports trailing commas?

Regexp? Maybe, but in a later CL, maybe if we have to fix this implementation
for some reason.

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:85: while (index <
code.length) {
On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> the code here to compute start-lines seems very similar to the logic we use in
> `SourceFile` from package:source_span, maybe you can directly use SourceFile
> here instead?
> 
> (https://github.com/dart-lang/source_span/blob/master/lib/src/file.dart#L22)

This is not just for extracting the values but to decode the @{x:y} markers and
compute the resulting offsets.

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:88: case 0x0A:
On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> can you define a couple const variables to give these a name?
> 
> const _LF = 0x0A;
> const _CR = 0x0D; ?

Done.

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:161:
Process.runSync(d8executable, [output]);
On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> when running test.py, are we guaranteed that the current working directory is
> the sdk-root? (so that the relative path from d8executable exists)?
> 
> What do you think about detecting the sdk-root, so that you can easily run
this
> test with test.py, or directly in dart from either the sdk-root or the folder
> where it is at?

We are always running from the root (other unittests rely upon that).

Leaving `run from anywhere` for a later CL (where you can pass any entrypoint to
main to get an interpreted stack trace).

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:227: factory
StackTraceLine.fromText(String text) {
On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> it might help to add a dartdoc with an example stack trace line to document
what
> kind of line you are parsing below.

Done.

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:240: int lastColon =
text.lastIndexOf(':');
On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> idea: consider splitting text by ':' first, then the case analysis below might
> get collapsed/simplified?
> 
> var segments = text.split(':');
> var fileName = segments[0];
> var lineNo = (segments.length > 1) ? int.parse(segments[1], onError: (_) =>
> null) : null;
> var columnNo = (segments.length > 2) ? int.parse(segments[2], onError: (_) =>
> null): null;

Doesn't work on Windows - file names may contain :

[Siggi Cherem (dart-lang), 2016-11-02 15:25:24]

lgtm

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:240: int lastColon =
text.lastIndexOf(':');
On 2016/11/02 14:34:30, Johnni Winther wrote:
> On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> > idea: consider splitting text by ':' first, then the case analysis below
might
> > get collapsed/simplified?
> > 
> > var segments = text.split(':');
> > var fileName = segments[0];
> > var lineNo = (segments.length > 1) ? int.parse(segments[1], onError: (_) =>
> > null) : null;
> > var columnNo = (segments.length > 2) ? int.parse(segments[2], onError: (_)
=>
> > null): null;
> 
> Doesn't work on Windows - file names may contain :

I see, for this specific case, is it because the drive name will show up in the
stack trace?

It might still be worth to keep the "split" logic separate from how you convert
those values to line/file/column numbers. For example, with some ad-hoc split
function:

_split(t) {
  var i1 = t.lastIndexOf(':');
  var i2 = t.lastIndexOf(':', i1 - 1);
  var result = [];
  int start = 0;
  if (i2 > 0) {
    result.add(t.substring(start, i2));
    start = i2 + 1;
  }
  if (i1 > 0) {
    result.add(t.substring(start, i1));
    start = i1 + 1;
  }
  result.add(t.substring(start));
  return result;
}

[Johnni Winther, 2016-11-03 09:20:18]

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:240: int lastColon =
text.lastIndexOf(':');
On 2016/11/02 15:25:24, Siggi Cherem (dart-lang) wrote:
> On 2016/11/02 14:34:30, Johnni Winther wrote:
> > On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> > > idea: consider splitting text by ':' first, then the case analysis below
> might
> > > get collapsed/simplified?
> > > 
> > > var segments = text.split(':');
> > > var fileName = segments[0];
> > > var lineNo = (segments.length > 1) ? int.parse(segments[1], onError: (_)
=>
> > > null) : null;
> > > var columnNo = (segments.length > 2) ? int.parse(segments[2], onError: (_)
> =>
> > > null): null;
> > 
> > Doesn't work on Windows - file names may contain :
> 
> I see, for this specific case, is it because the drive name will show up in
the
> stack trace?
> 
> It might still be worth to keep the "split" logic separate from how you
convert
> those values to line/file/column numbers. For example, with some ad-hoc split
> function:
> 
> _split(t) {
>   var i1 = t.lastIndexOf(':');
>   var i2 = t.lastIndexOf(':', i1 - 1);
>   var result = [];
>   int start = 0;
>   if (i2 > 0) {
>     result.add(t.substring(start, i2));
>     start = i2 + 1;
>   }
>   if (i1 > 0) {
>     result.add(t.substring(start, i1));
>     start = i1 + 1;
>   }
>   result.add(t.substring(start));
>   return result;
> }
> 

Alas `t.lastIndexOf(':', i1 - 1);` throws if `i1 - 1` is negative. That's way
need to got backwards.

[Johnni Winther, 2016-11-03 10:00:56]

Description was changed from

==========
Add stacktrace_test

- also add helper program for working with sourcemaps.
==========

to

==========
Add stacktrace_test

- also add helper program for working with sourcemaps.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d153808ca647c0c4e9484b903848851b4d314b50
==========

[Johnni Winther, 2016-11-03 10:00:57]

Committed patchset #4 (id:60001) manually as
d153808ca647c0c4e9484b903848851b4d314b50 (presubmit successful).

[Johnni Winther, 2016-11-03 11:21:10]

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2431223006/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:240: int lastColon =
text.lastIndexOf(':');
On 2016/11/03 09:20:18, Johnni Winther wrote:
> On 2016/11/02 15:25:24, Siggi Cherem (dart-lang) wrote:
> > On 2016/11/02 14:34:30, Johnni Winther wrote:
> > > On 2016/10/27 13:25:19, Siggi Cherem (dart-lang) wrote:
> > > > idea: consider splitting text by ':' first, then the case analysis below
> > might
> > > > get collapsed/simplified?
> > > > 
> > > > var segments = text.split(':');
> > > > var fileName = segments[0];
> > > > var lineNo = (segments.length > 1) ? int.parse(segments[1], onError: (_)
> =>
> > > > null) : null;
> > > > var columnNo = (segments.length > 2) ? int.parse(segments[2], onError:
(_)
> > =>
> > > > null): null;
> > > 
> > > Doesn't work on Windows - file names may contain :
> > 
> > I see, for this specific case, is it because the drive name will show up in
> the
> > stack trace?
> > 
> > It might still be worth to keep the "split" logic separate from how you
> convert
> > those values to line/file/column numbers. For example, with some ad-hoc
split
> > function:
> > 
> > _split(t) {
> >   var i1 = t.lastIndexOf(':');
> >   var i2 = t.lastIndexOf(':', i1 - 1);
> >   var result = [];
> >   int start = 0;
> >   if (i2 > 0) {
> >     result.add(t.substring(start, i2));
> >     start = i2 + 1;
> >   }
> >   if (i1 > 0) {
> >     result.add(t.substring(start, i1));
> >     start = i1 + 1;
> >   }
> >   result.add(t.substring(start));
> >   return result;
> > }
> > 
> 
> Alas `t.lastIndexOf(':', i1 - 1);` throws if `i1 - 1` is negative. That's way
> need to got backwards.

Should have been: That's why we need to go backwards.