Improve command line parsing for dartdev when passing flags to the VM

devoncarew commented 3 years ago

This is in support of IDEs switching to dart test when running and debugging tests. We'd like to do this in the 2.12.0 stable timeframe.

From https://github.com/flutter/flutter-intellij/issues/5043 and https://github.com/Dart-Code/Dart-Code/issues/2950, my understanding is that we're blocked on moving to dart test as not all the flags we need to pass to the VM are being interpreted by the VM (some are being passed to dartdev itself?). @DanTup has more details.

cc @bkonyi @sigurdm @jonasfj

DanTup commented 3 years ago

The VM args currently used in VS Code are enable-vm-service and pause_isolates_on_start (and then some args for package:test like -r json). In future it might be nice to also use --write-service-info though we don't today.

The following command (using dart run test:test) does work as expected:

dart --enable-vm-service=0 --pause_isolates_on_start=true run test:test test/test_test.dart

However using dart test does not:

dart --enable-vm-service=0 --pause_isolates_on_start=true test test/test_test.dart

It seems to pause the isolates, but not print the VM Service URI to stdout (or in the case of using --write-service-info, to the file).

jonasfj commented 3 years ago

note: dart test should mostly work as an alias for dart run test:test.

We might want a few tweaks like:

Suggest dart pub add --dev test if you have no dependency on package:test.
Show something in dart test --help even if you have no pubspec.yaml

bkonyi commented 3 years ago

So the issue here is that the VM service is actually starting, but it's waiting for DDS to connect before outputting the service URI. However, dart test isn't wired to startup DDS so we never end up printing the service information.

We need to make sure that any command that calls VmInteropHandler.run is able to initialize DDS or refuse to run with the service enabled. We make calls to VmInteropHandler.run in the following places:

lib/src/commands/pub.dart (2 instances)
lib/src/commands/compile.dart (1 instance, used to spawn dart2js)
lib/src/commands/run.dart (1 instance)
lib/src/commands/test.dart (1 instance)

(@jonasfj @sigurdm now that pub is integrated directly, do we need lib/src/commands/pub.dart anymore?)

Of all these call sites, only the one in run.dart will behave properly with the VM service enabled. If dart test is basically an alias for dart run test:test, we should look into forwarding handling of this command to dart run after some initial parsing (assuming that's possible).

sigurdm commented 3 years ago

(@jonasfj @sigurdm now that pub is integrated directly, do we need lib/src/commands/pub.dart anymore?)

No - I thought I had removed it already. https://dart-review.googlesource.com/c/sdk/+/172960 should fix this.

sigurdm commented 3 years ago

Suggest dart pub add --dev test if you have no dependency on package:test.

This is already done: https://github.com/dart-lang/sdk/blob/master/pkg/dartdev/lib/src/commands/test.dart#L53

Show something in dart test --help even if you have no pubspec.yaml

out/ReleaseX64/dart-sdk/bin/dart test --help
Run tests in this package.

Usage: dart test [arguments]

Run "dart help" to see global options.

We do show something without a pubspec. There is not much information though...

devoncarew commented 3 years ago

I think this changes when we embedded pub directly in the tool. The current output of dart test -h - for a package w/o a test dep - is:

Run tests in this package.

Usage: dart test [arguments]

Run "dart help" to see global options.

But before the change it was:

No dependency on package:test found. In order to run tests, you need to add a dependency
on package:test in your pubspec.yaml file:

dev_dependencies:
  test: ^1.0.0

See https://pub.dev/packages/test/install for more information on adding package:test,
and https://dart.dev/guides/testing for general information on testing.

Run tests in this package.

Usage: dart test [files or directories...]

I think we should look to restore the longer message.

devoncarew commented 3 years ago

https://github.com/dart-lang/sdk/blob/f346cb7f7786f7a595beac54f57bc60956c5b0ca/pkg/dartdev/lib/src/commands/test.dart#L104

sigurdm commented 3 years ago

Hmm - a message will be output with dart test:

https://github.com/dart-lang/sdk/blob/master/pkg/dartdev/lib/src/commands/test.dart#L51

I somewhat like that the help text is static documentation - I would never expect it to contain context sensitive information... but we can restore it if desired.

devoncarew commented 3 years ago

Yeah, I think restoring it would be useful.

From memory (and the older github history), it handled:

showing reasonable help output from dart test -h when there was no test dep - helping them know why the help output is abbreviated
showing reasonable help output from dart test when there was no test dep
special cased when there was no pubspec.yaml file
special cased when there was a pubspec.yaml but no .dart_tool/package_config.json file yet

jonasfj commented 3 years ago

special cased when there was a pubspec.yaml but no .dart_tool/package_config.json file yet

In this case, we I think the semantics should be to run pub get implicitly, which is also what dart run test:test will do.

bkonyi commented 3 years ago

What's the status on this issue? Can it be closed?

DanTup commented 3 years ago

If the intention is that dart test should work the same as dart run test:test then I don't think it's resolved. If you use dart run test:test with --pause_isolates_on_start=true it runs and prints the VM service URL:

danny@Dannys-MBP dart_sample % ~/Dev/Dart\ SDKs/nightly-2021-02-16/bin/dart --enable-vm-service=0 --pause_isolates_on_start=true run test:test test/danny_test.dart
Observatory listening on http://127.0.0.1:53617/8GjZEIVsoc8=/
<paused>

However if you use dart test, it does not:

danny@Dannys-MBP dart_sample % ~/Dev/Dart\ SDKs/nightly-2021-02-16/bin/dart --enable-vm-service=0 --pause_isolates_on_start=true test test/danny_test.dart
<paused>

How significant that is, I'm not sure.

Slightly related to the output of dart test -h mentioned above, it currently instructs you to run pub run test, not sure if that's intended:

danny@Dannys-MBP dart_sample % ~/Dev/Dart\ SDKs/nightly-2021-02-16/bin/dart test -h                                                                       
Runs tests in this package.

Usage: pub run test [files or directories...] // <----

sigurdm commented 3 years ago

Slightly related to the output of dart test -h mentioned above, it currently instructs you to run pub run test, not sure if that's intended:

dart test -h shows whatever dart run test -h shows. This is a bug in package:test.

bkonyi commented 3 years ago

cc @natebosch @grouma

sigurdm commented 3 years ago

Perhaps we could do a -- separation. Any args before -- goes to the VM, any args after goes to test.

dart test <argsA> -- <argsB>

Would then be equivalent to dart run <argsA> test:test <argsB>.

DanTup commented 3 years ago

@sigurdm if there's no --, could the args all go to test? I'm thinking of a simple case invoked by a user:

dart test test/foo_test.dart

sigurdm commented 3 years ago

@sigurdm if there's no --, could the args all go to test? I'm thinking of a simple case invoked by a user:

Yes! - I forgot to make that explicit!

dart-lang / sdk

Improve command line parsing for dartdev when passing flags to the VM #44200