pkg/vm_service_client: add getSourceReport to VMServiceReference

test/source_report_test.dart

Index: test/source_report_test.dart
diff --git a/test/source_report_test.dart b/test/source_report_test.dart
new file mode 100644
index 0000000000000000000000000000000000000000..b87520f79621442c73bf2db3bbd1f82bdcd832a0
--- /dev/null
+++ b/test/source_report_test.dart
@@ -0,0 +1,162 @@
+// Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file

[nweiz, 2016/04/29 00:52:45]

2016

[kevmoo, 2016/04/29 23:36:55]

Done.

+// 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:test/test.dart';
+import 'package:vm_service_client/vm_service_client.dart';
+
+import 'utils.dart';
+
+VMServiceClient client;
+VMIsolateRef isolate;

[nweiz, 2016/04/29 00:52:44]

There's no reason for this to be a top-level variable, as opposed to a local
variable in the group whose setUp actually initializes it.

[kevmoo, 2016/04/29 23:36:55]

Done.

+
+void main() {
+  tearDown(() {
+    if (client != null) {
+      client.close();
+      client = null;

[nweiz, 2016/04/29 00:52:45]

Don't bother nulling out the client. None of the other tests do it.

[kevmoo, 2016/04/29 23:36:55]

Done.

+    }
+    isolate = null;
+  });
+
+  group('simple script', () {

[nweiz, 2016/04/29 00:52:44]

The group name plus the test name should form a sentence. See other test files
for examples.

[kevmoo, 2016/04/29 23:36:55]

Done.

+    setUp(() async {
+      client = await runAndConnect(main: r"""
+      print("one");
+      print("two");
+    """);

[nweiz, 2016/04/29 00:52:44]

Indent this correctly.

[kevmoo, 2016/04/29 23:36:55]

this is copied from other tests that already exist – how should it be indented?

[nweiz, 2016/05/04 23:05:51]

The same way it was indented in the tests you copied it from. The quotes act
like brackets, so the prints should be indented two spaces in from the base
indentation, and the close-quotes should be on the base indentation.

[kevmoo, 2016/05/05 20:46:26]

Please look at the latest code. This is changed quite a bit.

+
+      isolate = (await client.getVM()).isolates.single;
+
+      await isolate.pause();

[nweiz, 2016/04/29 00:52:45]

I'm not sure what you're going for here, but this is going to be a race
condition regardless. There's no guarantee that the isolate will pause at any
particular point relative to the prints.

If you want to wait until the isolate pauses before exiting, use
waitUntilPaused().

[kevmoo, 2016/04/29 23:36:55]

I tried that and it never gets hit – I think because of the `new ReceivePort();`
in `runAndConnect`

Thoughts?

[nweiz, 2016/05/04 23:05:51]

The simplest solution would be to just add Isolate.current.kill() after the
prints.

[kevmoo, 2016/05/05 20:46:26]

Done.

+    });
+
+    test("no options", () async {
+      var report = await isolate.getSourceReport();
+
+      expect(report.scripts, hasLength(greaterThan(1)));
+      expect(report.ranges, hasLength(greaterThan(1)));
+
+      var customScript = report.scripts.last;
+
+      expect(customScript.uri.toString(), startsWith('data:application/dart'));
+
+      var range = report.ranges.singleWhere(
+          (range) => range.scriptIndex == report.scripts.length - 1);

[nweiz, 2016/04/29 00:52:45]

There shouldn't actually be multiple ranges in the runAndConnect script, right?

[kevmoo, 2016/04/29 23:36:55]

Acknowledged.

+
+      expect(range.compiled, isTrue);
+      expect(range.startPos, lessThan(range.endPos));

[nweiz, 2016/04/29 00:52:45]

Assert that the source location is equal to the source location of the top-level
function defined in the library.

[kevmoo, 2016/04/29 23:36:55]

Done.

+      expect(range.coverage, isNull);
+      expect(range.possibleBreakpoints, isNull);
+    });
+
+    test("coverage", () async {
+      var report = await isolate.getSourceReport(includeCoverageReport: true);
+

[nweiz, 2016/04/29 00:52:44]

Nit: Having a newline between every statement isn't much more readable than
having no newlines at all.

[kevmoo, 2016/04/29 23:36:55]

Acknowledged.

[nweiz, 2016/05/04 23:05:51]

You deleted one newline :p.

[kevmoo, 2016/05/05 20:46:25]

Acknowledged.

+      var customScript = report.scripts.last;
+
+      expect(customScript.uri.toString(), startsWith('data:application/dart'));
+
+      var range = report.ranges.singleWhere(
+          (range) => range.scriptIndex == report.scripts.length - 1);
+
+      expect(range.possibleBreakpoints, isNull);
+
+      var coverage = range.coverage;
+
+      expect(coverage.hits, isNotEmpty);
+      expect(coverage.misses, isEmpty);

[nweiz, 2016/04/29 00:52:44]

These assertions are pretty loose—they don't assert anything about where the
hits are, or whether running code affects its coverage state. It would be better
to have some code that's intentionally not run and some that is, and assert that
the hit tokens aren't in the not-run section and that the miss tokens are.

[kevmoo, 2016/04/29 23:36:55]

Done.

+    });
+
+    test("possible break points", () async {
+      var report =
+          await isolate.getSourceReport(includePossibleBreakpoints: true);
+
+      var customScript = report.scripts.last;
+
+      expect(customScript.uri.toString(), startsWith('data:application/dart'));
+
+      var range = report.ranges.singleWhere(
+          (range) => range.scriptIndex == report.scripts.length - 1);
+
+      expect(range.coverage, isNull);
+
+      expect(range.possibleBreakpoints, isNotEmpty);

[nweiz, 2016/04/29 00:52:45]

Actually assert what the breakpoints are. You can use VMScript.sourceLocation to
get easily-verifiable representations in terms of the actual code.

[kevmoo, 2016/04/29 23:36:55]

Done.

+    });
+
+    test("both options", () async {

[nweiz, 2016/04/29 00:52:45]

I don't think this is necessary.

[kevmoo, 2016/04/29 23:36:55]

Eh, I like to make sure someone in the future doesn't mess this up – I could
imagine someone doing an if/else and not realizing they are breaking something

[nweiz, 2016/05/04 23:05:51]

If you're going to test the cross-product of all options, make sure to test
coverage && breakpoints && forceCompile, coverage && !breakpoints &&
forceCompile, and so on ;).

We have to draw a line on what gets tested somewhere, and it's helpful to have a
consistent notion of where that is. Generally it's good to draw that line
*before* testing combinations of orthogonal flags.

[kevmoo, 2016/05/05 20:46:25]

Acknowledged.

+      var report = await isolate.getSourceReport(
+          includeCoverageReport: true, includePossibleBreakpoints: true);
+
+      var customScript = report.scripts.last;
+
+      expect(customScript.uri.toString(), startsWith('data:application/dart'));
+
+      var range = report.ranges.singleWhere(
+          (range) => range.scriptIndex == report.scripts.length - 1);
+
+      expect(range.coverage, isNotNull);
+      expect(range.possibleBreakpoints, isNotNull);
+    });
+  });

[nweiz, 2016/04/29 00:52:45]

Test forceCompile as well.

[kevmoo, 2016/04/29 23:36:56]

Done.

+
+  test("script and ranges", () async {
+    client = await runAndConnect(
+        topLevel: r'''final foo = 5;
+        int add(a, b) => a + b;
+        ''',
+        main: r"""
+      print("one");
+      print("two");
+    """);
+
+    isolate = (await client.getVM()).isolates.single;
+
+    await isolate.pause();
+
+    var report = await isolate.getSourceReport(includeCoverageReport: true);
+
+    var customScript = report.scripts.last;
+
+    expect(customScript.uri.toString(), startsWith('data:application/dart'));
+
+    report = await customScript.getSourceReport(
+        includeCoverageReport: true, includePossibleBreakpoints: true);

[nweiz, 2016/04/29 00:52:45]

Rather than grabbing source locations from a previous report, grab them from the
library's members.

[kevmoo, 2016/04/29 23:36:55]

Done.

+
+    expect(report.ranges, hasLength(2));
+
+    var firstRange = report.ranges.first;
+    var secondRange = report.ranges.last;
+
+    expect(firstRange.endPos, lessThan(secondRange.startPos),
+        reason: 'The first range should come before the second range.');
+
+    // validate the start param
+
+    report = await customScript.getSourceReport(
+        includeCoverageReport: true,
+        includePossibleBreakpoints: true,
+        tokenPos: firstRange.endPos + 1);
+
+    expect(report.ranges.single.startPos, secondRange.startPos);
+
+    // validate the end param
+
+    report = await customScript.getSourceReport(
+        includeCoverageReport: true,
+        includePossibleBreakpoints: true,
+        endTokenPos: secondRange.startPos - 1);
+
+    expect(report.ranges.single.endPos, firstRange.endPos);
+
+    // validate both params
+
+    report = await customScript.getSourceReport(
+        includeCoverageReport: true,
+        includePossibleBreakpoints: true,
+        tokenPos: firstRange.endPos + 1,
+        endTokenPos: secondRange.startPos - 1);
+
+    expect(report.ranges, isEmpty);
+  });

[nweiz, 2016/04/29 00:52:44]

Split this up into multiple tests with a shared setUp.

[kevmoo, 2016/04/29 23:36:55]

Done.

+}