block-forest / dart-coveralls

Calculate coverage of your dart scripts, format it to LCOV and send it to coveralls
https://pub.dartlang.org/packages/dart_coveralls
MIT License
27 stars 17 forks source link

`dart bin/dart-coveralls.dart test/test-all.dart` gives incorrect data #61

Closed eseidelGoogle closed 7 years ago

eseidelGoogle commented 7 years ago

I'm just attempting to sanity check dart-coveralls against itself.

I'm using Dart VM version: 1.23.0-dev.0.0 (Tue Feb 14 14:19:29 2017) on "macos_x64"

dart bin/dart_coveralls.dart calc test/test_all.dart >| coverage/lcov.info

The very first record is wrong:

SF:/src/dart-coveralls/test/test_all.dart
DA:8,1
DA:9,0
DA:10,0
end_of_record

Note is says only the first line (8) is covered. But if I hack dart_coveralls.dart to route stdout from the subprocess to stderr, I see that's not true:

diff --git a/lib/src/collect_lcov.dart b/lib/src/collect_lcov.dart
index 272512e..f2b1b4e 100644
--- a/lib/src/collect_lcov.dart
+++ b/lib/src/collect_lcov.dart
@@ -131,6 +131,7 @@ class LcovCollector {
       if (uri != null) {
         hostCompleter.complete(uri);
       }
+      stderr.write(data);
     });
     Uri host = await hostCompleter.future;

Because I see it printing lines from areas under lines 9 and 10, which are claimed to "not be covered".

If I check manually in observatory it's clearly covered: dart --pause-isolates-on-exit --enable-vm-service test/test_all.dart

screen shot 2017-02-19 at 11 32 49 am

So something isn't quite right.

eseidelGoogle commented 7 years ago

The plot thickens. The json we're getting back from observatory appears wrong:

  {
    "source": "file:///src/dart-coveralls/test/test_all.dart",
    "script": {
      "type": "@Script",
      "fixedId": true,
      "id": "libraries/1/scripts/file%3A%2F%2F%2Fsrc%2Fdart-coveralls%2Ftest%2Ftest_all.dart",
      "uri": "file:///src/dart-coveralls/test/test_all.dart",
      "_kind": "library"
    },
    "hits": [
      8,
      1,
      9,
      0,
      10,
      0
    ]
  },

Using this patch:

diff --git a/lib/src/collect_lcov.dart b/lib/src/collect_lcov.dart
index 272512e..4231e16 100644
--- a/lib/src/collect_lcov.dart
+++ b/lib/src/collect_lcov.dart
@@ -2,7 +2,7 @@ library dart_coveralls.lcov;

 import "dart:async" show Completer, Future;
 import "dart:io";
-import "dart:convert" show UTF8;
+import "dart:convert" show UTF8, JsonEncoder;

 import "package:coverage/coverage.dart";
 import "package:coverage/src/util.dart" as util;
@@ -87,6 +87,9 @@ class LcovCollector {
       return null;
     }

+    JsonEncoder encoder = new JsonEncoder.withIndent('  ');
+    new File('/tmp/tmp.json').writeAsStringSync(encoder.convert(reportFile));
+
     Map<String, dynamic> hitmap = {};
     mergeHitmaps(createHitmap(reportFile), hitmap);
     return await _formatCoverageJson(hitmap);
@@ -131,6 +134,7 @@ class LcovCollector {
       if (uri != null) {
         hostCompleter.complete(uri);
       }
+      stderr.write(data);
     });
     Uri host = await hostCompleter.future;
eseidelGoogle commented 7 years ago

I think the bug is that we're not waiting until the tests are done to collect coverage. :)

eseidelGoogle commented 7 years ago

I suspect we'll have to connect to the process and sign up for the PauseExit event to know when to collect.

https://github.com/dart-lang/sdk/blob/master/runtime/vm/service/service.md#events

flutter_tools has a bunch of code for watching the test subprocess, some of which might be applicable here: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/test/flutter_platform.dart#L229

eseidel commented 7 years ago

So I think the bug is that

          await collect(host, true, false, timeout: new Duration(seconds: 60));

should have wait_paused: true, not false?

nilsdoehring commented 7 years ago

Great catch! I'll see if this change has effects on error handling and will commit a fix, tonight. Also, it's about time to publish v0.5.0 to pub.