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

test/source_map_stack_trace_test.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // 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';
 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", () {
     var trace = new Trace.parse("foo.dart.js 10:11  foo");
     var frame = _mapTrace(_simpleMapping, trace).frames.first;
     expect(frame.uri, equals(Uri.parse("foo.dart")));
 
     // These are +1 because stack_trace is 1-based whereas source_span is
     // 0-basd.
     expect(frame.line, equals(2));
@@ skipping 20 matching lines @@
 foo.dart.js 10:11  baz
 """);
 
     var frames = _mapTrace(_simpleMapping, trace).frames;
 
     expect(frames.length, equals(2));
     expect(frames.first.member, equals("foo"));
     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;
     expect(frame.uri, equals(Uri.parse("foo.dart")));
     expect(frame.line, equals(2));
     expect(frame.column, equals(4));
   });
 
   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));
   });
 
   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,
         packageResolver: new SyncPackageResolver.root("packages/"));
     var frame = mappedTrace.frames.first;
     expect(frame.uri, equals(Uri.parse("package:foo/foo.dart")));
     expect(frame.line, equals(2));
     expect(frame.column, equals(4));
   });
 
   test("uses package: URIs for frames within a packageResolver.packageMap URL",
       () {
     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));
     expect(frame.column, equals(4));
   });
 
   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;
     expect(frame.uri, equals(Uri.parse("dart:async/foo.dart")));
     expect(frame.line, equals(2));
     expect(frame.column, equals(4));
   });
 
   test("converts a stack chain", () {
     var trace = new Chain([
@@ skipping 30 matching lines @@
       expect(_prettify(r"foo$1$bar"), equals("foo"));
       expect(_prettify(r"foo$123$bar$bang$qux"), equals("foo"));
     });
 
     test("closures", () {
       expect(_prettify("foo_closure.call"), equals("foo.<fn>"));
     });
 
     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", () {
       expect(_prettify("foo.call"), equals("foo"));
     });
 
     test("top-level functions", () {
       expect(_prettify("dart.foo"), equals("foo"));
     });
 
@@ skipping 12 matching lines @@
     test("lots of stuff", () {
       expect(_prettify(r"lib$Foo.static.lib$Foo_closure.call$0/<"),
           equals("Foo.<fn>"));
     });
   });
 }
 
 /// 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));
 }
 
 /// Runs the mapper's prettification logic on [member] and returns the result.
 String _prettify(String member) {
   var trace = new Trace([new Frame(Uri.parse("foo.dart.js"), 10, 11, member)]);
   return _mapTrace(_simpleMapping, trace).frames.first.member;
 }