Add debugging support for the JSON reporter.

[nex3]

Closes #459

[nex3]

@alexander-doroshko @jwren @devoncarew Before I officially get this reviewed, can you play around with it enough to be confident that it'll fit IntelliJ's needs?

[alexander-doroshko]

I looked through the docs (json_reporter.md) and it lgtm, thanks a lot!

How do you think restartCurrent() could be effectively used? I mean UI, standard use cases. May be you can point me to a tool (or screenshots) that supports something similar in other testing framework. Sorry for being dumb here.

[nex3]

restartCurrent() addresses a user feature request, #335. It allows the browser context to be preserved across multiple runs of a test case, so that stuff like breakpoints and watched variables don't have to be reset each time. It's up to you whether you want to expose that through the IDE, but I wanted to make it possible to ensure the IDE has parity with the command line API.

[kevmoo]

Any further feedback on this @alexander-doroshko @jwren @devoncarew ??

@nex3 would like to know 😄

[alexander-doroshko]

@kevmoo @nex3
Just noticed that this PR is not landed. I hope it's not me who you are waiting for. As I said above protocol changes (json_reporter.md) look good to me. But I'm afraid I can't review the implementation code as I'm far from understanding package:test internals.
I tried to play with it by using package:test from sources with this PR applied. But I failed to proceed because of:

Warning: Debugging is currently unsupported on the Dart VM.

[nex3]

Rad, I'll go ahead and land it.

I tried to play with it by using package:test from sources with this PR applied. But I failed to proceed because of:

Warning: Debugging is currently unsupported on the Dart VM.

Yeah, you'll need to test it with browser tests for now.

[devoncarew]

Warning: Debugging is currently unsupported on the Dart VM.

Yeah, you'll need to test it with browser tests for now.

But the VM has the necessary support for this now, right? With the dart:developer Service class https://api.dartlang.org/dev/1.21.0-dev.6.0/dart-developer/Service-class.html

[nex3]

Yes! That landed very recently, but I'm certainly planning on adding support for it.

[alexander-doroshko]

Thanks for landing it!

you'll need to test it with browser tests for now

I'm afraid it is much more difficult for me to support browser debugging at IDE side, I'd like to start with VM tests debugging.

[nex3]

I'm a little confused—the API added by this pull request is clearly heavily based around interacting with the test runner via JavaScript APIs in the browser's remote debugging interface. Those changes were all I was planning on targeting for this quarter, and you said that they looked good. If you weren't planning on starting with support for them, what were you planning?

I do plan to add support for VM debugging as soon as I have time, but that time wasn't budgeted for this quarter so it remains to be seen if I'll actually be able to fit it in. Once I do start working on it, it'll take a solid chunk of time to get it stable and add a channel for an IDE to communicate with it. If we want to get IDE debugging support in this quarter, I think work needs to start happening on the IDE side before VM debugging support lands in test.

[alexander-doroshko]

I'm very sorry for the miscommunication. Let me describe my point from scratch.

I'm the author of the Dart VM Apps debugger in IntelliJ IDEA. I know VM Service protocol very well and wrote its full integration in the IDE. I was hoping to get VM tests debugging almost for free because everything is already done at IDE side. The only thing I need is to connect to the VM Service, i.e. to Observatory. I was thinking about different ways to connect to the test runner VM:

• allow to pass VM options to pub run, so that I could say -observe (Pub run should support passing in flags to vm pub#1018)
• change pub run to spawn an isolate rather than starting a subprocess ("pub run" should spawn an isolate rather than starting a subprocess pub#1204), this way I could start pub as dart -observe pub.dart.snapshot

You've chosen 3rd approach: test runner itself reports Observatory URL in its DebugEvent. Looks like a viable solution, so I wrote that I like this test reporter protocol change.

May be we mean different things by 'tests debugging'? I mean connect to the VM Service and using its API to set breakpoints, show stack frames and variables when stopped at a breakpoint, stepping, etc. All I need to debug VM tests is Observatory URL. Nothing more. I find myself not understanding the 2nd part of your previous comment.

As for debugging tests in browser - unfortunately I know much less about it. We have JavaScript debugger in IntelliJ IDEA that is able to debug web apps in Chrome, but I'm not the author and do not know all the nuances. I thought it would be the same: IDE connects to Chrome and uses Chrome Debug Protocol to set breakpoints, explore frames and vars, stepping, etc. I do not know if any JavaScript API at package:test side is required. Sorry for being dumb here.

So actually the only thing I reviewed in this pull request is that test reporter protocol is going to report Observatory URL for VM tests debugging.

I took another look and have one comment. I overlooked that Observatory URL is reported not once per test run, but once per test suite. Does it make sense? I thought a single test run can't combine VM tests with any other in one shot. From IDE point of view I'd like to get Observatory URL once and as early as possible.

Thanks, and sorry again for miscommunication.

[alexander-doroshko]

Observatory URL is reported not once per test run, but once per test suite. Does it make sense? I thought a single test run can't combine VM tests with any other in one shot.

Sorry, I checked that it is possible to pass test runner parameters like -p vm,chrome and tests will run both in Chrome and VM in one test run.
Will it be a correct assumption that for all suites that are executed on the same platform during one test run observatory URL (and remoteDebugger URL correspondingly) is the same?

[nex3]

May be we mean different things by 'tests debugging'? I mean connect to the VM Service and using its API to set breakpoints, show stack frames and variables when stopped at a breakpoint, stepping, etc. All I need to debug VM tests is Observatory URL. Nothing more. I find myself not understanding the 2nd part of your previous comment.

That's fine for debugging an executable directly, but it's not that simple when you're talking about a test runner. If we just emitted the Observatory URL and did nothing else different, you'd have a race condition where you'd be trying to interact with the VM service protocol at the same time as the test runner is trying to run tests. So at the very least, you need the test runner to pause after the iframe/isolate containing the test has been loaded and before any tests have begun running. And once it's paused in this way, you also need some means of telling it "I'm done setting breakpoints, start running again".

This flow is necessary for both the VM and for browsers. The only difference is exactly how the IDE talks to the test runner—at least to tell the test runner "start running again", and possibly also to invoke more advanced APIs like restartCurrent(). For browsers, this is done by talking to JS via the Chrome remote debugging protocol; for the VM, there's nothing in place yet.

I think it's pretty clear from our user base that browser debugging is more important than VM debugging, and I suspect that most of the work will carry over between the two. I'd strongly encourage you to start implementing browser debugging now and only add VM debugging later on once test supports it.

I took another look and have one comment. I overlooked that Observatory URL is reported not once per test run, but once per test suite. Does it make sense? I thought a single test run can't combine VM tests with any other in one shot. From IDE point of view I'd like to get Observatory URL once and as early as possible.

Sorry, I checked that it is possible to pass test runner parameters like -p vm,chrome and tests will run both in Chrome and VM in one test run.
Will it be a correct assumption that for all suites that are executed on the same platform during one test run observatory URL (and remoteDebugger URL correspondingly) is the same?

I'm cautious about offering strong guarantees here. Currently we do only spawn a single browser for each platform in a given run, but I can imagine circumstances that would cause that to change. The safest thing to do would be to check URL equality to determine whether or not you need to connect to a new Observatory instance.

One aspect of the URLs that's likely to differ between suites in the near future is the anchor, which we'll use to route users to the proper isolate for their test suite. Make sure your URL equality checks factor that out.

[devoncarew]

I think it's pretty clear from our user base that browser debugging is more important than VM debugging

I would like to be able to debug VM tests from IntelliJ; I don't think there's any great technical hurdle here.

you'd have a race condition where you'd be trying to interact with the VM service protocol at the same time as the test runner is trying to run tests. So at the very least, you need the test runner to pause after the iframe/isolate containing the test has been loaded and before any tests have begun running. And once it's paused in this way, you also need some means of telling it "I'm done setting breakpoints, start running again".

That all makes sense. @nex3, @alexander-doroshko, I may follow up by email just to clarify what the technical details are here.

[nex3]

I would like to be able to debug VM tests from IntelliJ; I don't think there's any great technical hurdle here.

Don't get me wrong, I totally agree that VM debugging is important—and it's something we should definitely aim to have in place in the next few months. I'm just talking about relative priorities here. We have many more web users than we have pure VM users, so it doesn't make sense to block support for browser debugging while you wait for VM debugging to be available.

[devoncarew]

Cool, understood! And I do agree that browser test debugging is important as well :)

Just to give you my perspective on the importance of vm tests, I have talked to internal customers that have written many of their tests as platform independent tests - not using dart:io or dart:html - and would have a slightly better workflow running these tests in the vm. And, flutter tests are much closer to vm tests than browser ones, and we'd like to get debuggability of those tests as well.

[nex3]

Flutter debuggability could be supported today entirely within the Flutter plugin; the plugin API they use is flexible enough that they can just plug in an Observatory URI and everything else would fall into place.