dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
498 stars 214 forks source link

Coverage not collected when test spawns isolate #1108

Open simolus3 opened 5 years ago

simolus3 commented 5 years ago

When a test spawns a new isolate, code run on that isolate won't be included in the coverage output:

void main() {
  test('background isolate test') async {
    final receive = ReceivePort();
    await Isolate.spawn(runInBackground, receive.sendPort);
    await receive.first;
  }
}

void runInBackground(SendPort notifyComplete) {
  // ...
  notifyComplete.send(null);
}

When running that test via pub run test --coverage=dir, it won't report on runInBackground.

I see that gatherCoverage obtains the target isolate id from the suite environment, so that only includes the isolate running the tests.

grouma commented 5 years ago

I don't know if we have a way to track which isolates are spawned by the test. We get the isolateId from the observatoryURl here: https://github.com/dart-lang/test/blob/457abf08997a0903f86f19b0574d164574527735/pkgs/test_core/lib/src/runner/coverage.dart#L21

@cbracken do you have thoughts on how we can tackle this?

simolus3 commented 5 years ago

The test suite already uses the vm_service package to listen for the exit of the main isolate, so maybe it could also listen for the start of new isolates. But I couldn't find a way to filter for isolates spawned by a specific isolate, which would be necessary for tests.

If this turns out to be impossible, maybe the test api could provide a trackIsolate(Isolate) method that I'd have to call manually?

grouma commented 5 years ago

Tracking spawned isolates is a good idea. I looked through the VM Service Protocol: https://github.com/dart-lang/sdk/blob/master/runtime/vm/service/service.md

As you suspected, we don't have the information to filter isolates spawned by a specific isolate. The alternative of exposing a top level trackIsolate function seems reasonable.

simolus3 commented 5 years ago

I can submit a PR for this, but I'd appreciate quick feedback on two issues to verify I'm on the right track. I would go with the trackIsolate route as I couldn't find any other workaround that would listen for isolates spawned in a specific test suite. Is it ok to just add that function to test_api/test_api.dart? I'm not sure if there are any drawbacks when dart:isolate is imported with ddc/dart2js. Does it make sense to introduce additional libraries instead, say test(ε|_core|_api)/track_isolate.dart?

I'm also not entirely sure how this should be implemented without leaking implementation details into the invoker. After a quick look, the VMEnvironment seems like a suitable place to collect additional isolate refs. But how does one get there from the invoker? Is there some platform interface in test_api that's implemented in test_core? Should I add one?

grouma commented 5 years ago

Is it ok to just add that function to test_api/test_api.dart?

Yeah it's probably fine to make a top level function there that is well documented.

I'm not sure if there are any drawbacks when dart:isolate is imported with ddc/dart2js.

Unless it's hidden behind a conditional import it will be problematic for platform specific compilers. Since we only really care about the isolate ID let's just change the signature of the function to trackIsolate(String isolateId).

Does it make sense to introduce additional libraries instead, say test(ε|_core|_api)/track_isolate.dart?

It's probably best to just keep it in test_api/test_api.dart as there a couple other helper methods there.

I'm also not entirely sure how this should be implemented without leaking implementation details into the invoker. After a quick look, the VMEnvironment seems like a suitable place to collect additional isolate refs. But how does one get there from the invoker? Is there some platform interface in test_api that's implemented in test_core? Should I add one?

I haven't looked into it but you likely can just add some extra details onto the current zone and pull them out during the gatherCoverage call.

Coronon commented 3 years ago

What are the plans for this? I could really need this feature.

simolus3 commented 3 years ago

I didn't explore this further. In my case I was able to collect coverage by running some tests in a mode where I use background isolates (to make sure it works) and in a mode where I communicate through SendPorts, but only on the same isolate (to collect coverage). So far, this worked well for me.

IMO the best way forwards is to add this information to the VM service, but that will require a separate issue on the SDK to track.

Coronon commented 3 years ago

You seem to be quite familiar with the depths of the Dart SDK. Would you mind creating that issue? I think that with the language growing this will become a much more frequent problem people stumble across.