Fix behavior causing frames lacking a source map to always be omitted. Add `includeUnmappedFrames` …

test/source_map_stack_trace_test.dart

Index: test/source_map_stack_trace_test.dart
diff --git a/test/source_map_stack_trace_test.dart b/test/source_map_stack_trace_test.dart
index 64145a63de1b5dc310b60a294fc9b88f4a7e7866..651ca3523771d8cfa0e2d16fbcc27af247e5fba2 100644
--- a/test/source_map_stack_trace_test.dart
+++ b/test/source_map_stack_trace_test.dart
@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:package_resolver/package_resolver.dart';
+import 'package:path/path.dart' as p;
 import 'package:source_maps/source_maps.dart';
 import 'package:source_span/source_span.dart';
 import 'package:stack_trace/stack_trace.dart';
@@ -10,18 +11,14 @@ import 'package:source_map_stack_trace/source_map_stack_trace.dart';
 import 'package:test/test.dart';
 
 /// A simple [Mapping] for tests that don't need anything special.
-final _simpleMapping = parseJson(
-    (new SourceMapBuilder()
-        ..addSpan(
-            new SourceMapSpan.identifier(
-                new SourceLocation(1,
-                    line: 1, column: 3, sourceUrl: "foo.dart"),
-                "qux"),
-            new SourceSpan(
-                new SourceLocation(8, line: 5, column: 0),
-                new SourceLocation(18, line: 15, column: 0),
-                "\n" * 10)))
-        .build("foo.dart.js.map"));

[nweiz, 2016/12/12 22:04:56]

Please remove unrelated formatting changes.

[Jacob, 2016/12/13 18:01:54]

Done.

+final _simpleMapping = parseJson((new SourceMapBuilder()
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1, line: 1, column: 3, sourceUrl: "foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(18, line: 15, column: 0), "\n" * 10)))
+    .build("foo.dart.js.map"));
 
 void main() {
   test("maps a JS line and column to a Dart line and span", () {
@@ -62,18 +59,49 @@ foo.dart.js 10:11  baz
     expect(frames.last.member, equals("baz"));
   });
 
+  test("include JS frames from external libraries with source map bundle", () {

[nweiz, 2016/12/12 22:04:56]

"includes"

I'm not sure what "external" means here. What makes a library external vs
internal?

[Jacob, 2016/12/13 18:01:54]

jquery.js is external library
foo.dart.js is internal.

[nweiz, 2016/12/13 22:05:46]

Maybe "JS files not covered by the source map bundle"

[Jacob, 2016/12/13 23:13:05]

Done.

+    var trace = new Trace.parse("""
+foo.dart.js 10:11  foo
+jquery.js 10:1  foo
+""");
+    var builder = new SourceMapBuilder()
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1,
+                  line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(12, line: 9, column: 1), "\n" * 4));
+    var sourceMapJson = builder.build("foo.dart.js.map");
+    sourceMapJson['file'] = "foo.dart.js";
+
+    var bundle = [sourceMapJson];
+    var mapping = parseJsonExtended(bundle);
+    var frames = _mapTrace(mapping, trace,
+            packageResolver: new SyncPackageResolver.root("packages/"))
+        .frames;
+
+    expect(frames.length, equals(2));
+
+    var frame = frames.first;
+    expect(frame.uri, equals(Uri.parse("package:foo/foo.dart")));
+    expect(frame.line, equals(2));
+    expect(frame.column, equals(4));
+
+    frame = frames.last;
+    expect(p.basename(frame.uri.toString()), equals("jquery.js"));
+    expect(frame.line, equals(10));
+  });
+

[nweiz, 2016/12/12 22:04:56]

Also test a source map bundle contains multiple source maps for multiple
different target files.

[Jacob, 2016/12/13 18:01:54]

that is the sort of test that belongs in the source maps package. tests in this
package just need to make sure we are actually passing in the uris so that the
source map package can do its thing.

To verify that, having jquery.js and foo.dart.js is sufficient.
The tests in the source_maps package are the right place for tests that verify
in detail that edge cases resolving source map bundles work.

[nweiz, 2016/12/13 22:05:46]

This CL concretely changes the behavior with respect to bundles, because it's
now passing in the URL from the frame. That's the most important user-facing
change here, and we should test it.

[Jacob, 2016/12/13 23:13:05]

Integration level testing of that behavior exists in this file. notice that
behavior for the line in foo.dart and and jquery.js are different only because
the file names are passed in. If we had a bug and omitted the uri then behavior
for the jquery.js line would change.
That is the appropriate level of granularity to test in this package.
In source_maps you should and I do have tests that verify edge cases within the
source_maps package.

[nweiz, 2016/12/13 23:43:48]

That's not clear from a black-box perspective. There's a big semantic difference
between "different files that appear in the source map" and "a file that doesn't
appear in the source map at all", and it's reasonable to test both cases.

   test("falls back to column 0 for unlisted column", () {
     var trace = new Trace.parse("foo.dart.js 10  foo");
     var builder = new SourceMapBuilder()
-        ..addSpan(
-            new SourceMapSpan.identifier(
-                new SourceLocation(1,
-                    line: 1, column: 3, sourceUrl: "foo.dart"),
-                "qux"),
-            new SourceSpan(
-                new SourceLocation(8, line: 5, column: 0),
-                new SourceLocation(12, line: 9, column: 1),
-                "\n" * 4));
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1, line: 1, column: 3, sourceUrl: "foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(12, line: 9, column: 1), "\n" * 4));
 
     var mapping = parseJson(builder.build("foo.dart.js.map"));
     var frame = _mapTrace(mapping, trace).frames.first;
@@ -85,19 +113,17 @@ foo.dart.js 10:11  baz
   test("uses package: URIs for frames within packageRoot", () {
     var trace = new Trace.parse("foo.dart.js 10  foo");
     var builder = new SourceMapBuilder()
-        ..addSpan(
-            new SourceMapSpan.identifier(
-                new SourceLocation(1,
-                    line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
-                "qux"),
-            new SourceSpan(
-                new SourceLocation(8, line: 5, column: 0),
-                new SourceLocation(12, line: 9, column: 1),
-                "\n" * 4));
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1,
+                  line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(12, line: 9, column: 1), "\n" * 4));
 
     var mapping = parseJson(builder.build("foo.dart.js.map"));
-    var frame = _mapTrace(mapping, trace, packageRoot: "packages/")
-        .frames.first;
+    var frame =
+        _mapTrace(mapping, trace, packageRoot: "packages/").frames.first;
     expect(frame.uri, equals(Uri.parse("package:foo/foo.dart")));
     expect(frame.line, equals(2));
     expect(frame.column, equals(4));
@@ -106,15 +132,13 @@ foo.dart.js 10:11  baz
   test("uses package: URIs for frames within packageResolver.packageRoot", () {
     var trace = new Trace.parse("foo.dart.js 10  foo");
     var builder = new SourceMapBuilder()
-        ..addSpan(
-            new SourceMapSpan.identifier(
-                new SourceLocation(1,
-                    line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
-                "qux"),
-            new SourceSpan(
-                new SourceLocation(8, line: 5, column: 0),
-                new SourceLocation(12, line: 9, column: 1),
-                "\n" * 4));
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1,
+                  line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(12, line: 9, column: 1), "\n" * 4));
 
     var mapping = parseJson(builder.build("foo.dart.js.map"));
     var mappedTrace = _mapTrace(mapping, trace,
@@ -129,21 +153,18 @@ foo.dart.js 10:11  baz
       () {
     var trace = new Trace.parse("foo.dart.js 10  foo");
     var builder = new SourceMapBuilder()
-        ..addSpan(
-            new SourceMapSpan.identifier(
-                new SourceLocation(1,
-                    line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
-                "qux"),
-            new SourceSpan(
-                new SourceLocation(8, line: 5, column: 0),
-                new SourceLocation(12, line: 9, column: 1),
-                "\n" * 4));
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1,
+                  line: 1, column: 3, sourceUrl: "packages/foo/foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(12, line: 9, column: 1), "\n" * 4));
 
     var mapping = parseJson(builder.build("foo.dart.js.map"));
     var mappedTrace = _mapTrace(mapping, trace,
-        packageResolver: new SyncPackageResolver.config({
-          "foo": Uri.parse("packages/foo")
-        }));
+        packageResolver:
+            new SyncPackageResolver.config({"foo": Uri.parse("packages/foo")}));
     var frame = mappedTrace.frames.first;
     expect(frame.uri, equals(Uri.parse("package:foo/foo.dart")));
     expect(frame.line, equals(2));
@@ -153,15 +174,13 @@ foo.dart.js 10:11  baz
   test("uses dart: URIs for frames within sdkRoot", () {
     var trace = new Trace.parse("foo.dart.js 10  foo");
     var builder = new SourceMapBuilder()
-        ..addSpan(
-            new SourceMapSpan.identifier(
-                new SourceLocation(1,
-                    line: 1, column: 3, sourceUrl: "sdk/lib/async/foo.dart"),
-                "qux"),
-            new SourceSpan(
-                new SourceLocation(8, line: 5, column: 0),
-                new SourceLocation(12, line: 9, column: 1),
-                "\n" * 4));
+      ..addSpan(
+          new SourceMapSpan.identifier(
+              new SourceLocation(1,
+                  line: 1, column: 3, sourceUrl: "sdk/lib/async/foo.dart"),
+              "qux"),
+          new SourceSpan(new SourceLocation(8, line: 5, column: 0),
+              new SourceLocation(12, line: 9, column: 1), "\n" * 4));
 
     var mapping = parseJson(builder.build("foo.dart.js.map"));
     var frame = _mapTrace(mapping, trace, sdkRoot: "sdk/").frames.first;
@@ -212,8 +231,8 @@ foo.dart.js 10:11  baz
 
     test("nested closures", () {
       expect(_prettify("foo__closure.call"), equals("foo.<fn>.<fn>"));
-      expect(_prettify("foo____closure.call"),
-          equals("foo.<fn>.<fn>.<fn>.<fn>"));
+      expect(
+          _prettify("foo____closure.call"), equals("foo.<fn>.<fn>.<fn>.<fn>"));
     });
 
     test(".call", () {
@@ -246,20 +265,28 @@ foo.dart.js 10:11  baz
 /// Like [mapStackTrace], but is guaranteed to return a [Trace] so it can be
 /// inspected.
 Trace _mapTrace(Mapping sourceMap, StackTrace stackTrace,
-    {bool minified: false, SyncPackageResolver packageResolver, sdkRoot,
+    {bool minified: false,
+    SyncPackageResolver packageResolver,
+    sdkRoot,
     packageRoot}) {
   return new Trace.from(mapStackTrace(sourceMap, stackTrace,
-      minified: minified, packageResolver: packageResolver, sdkRoot: sdkRoot,
+      minified: minified,
+      packageResolver: packageResolver,
+      sdkRoot: sdkRoot,
       packageRoot: packageRoot));
 }
 
 /// Like [mapStackTrace], but is guaranteed to return a [Chain] so it can be
 /// inspected.
 Chain _mapChain(Mapping sourceMap, StackTrace stackTrace,
-    {bool minified: false, SyncPackageResolver packageResolver, sdkRoot,
+    {bool minified: false,
+    SyncPackageResolver packageResolver,
+    sdkRoot,
     packageRoot}) {
   return new Chain.forTrace(mapStackTrace(sourceMap, stackTrace,
-      minified: minified, packageResolver: packageResolver, sdkRoot: sdkRoot,
+      minified: minified,
+      packageResolver: packageResolver,
+      sdkRoot: sdkRoot,
       packageRoot: packageRoot));
 }