Implement a concrete test ArgParser in dartdev

[jwren]

test only prints the help text if there is a pubspec.yaml file in the current working directory.

[jakemac53]

What is the use case that you are trying to support here?

This package only intends to support pub style packages afaik.

[jwren]

1. User installs the Dart SDK
2. In the CLI the user is in the default directory that is opened for them, say their home dir
3. User types in dart help to explore what the dart (dartdev) tool can do
4. User types dart analyze --help/ dart format --help - sees help output
5. User types dart test --help - user an error

The message that test works in the current working directory is only visible if you happened to cd to such a directory.

1 more thoughts:

dart test --help - because it goes to verify that a pubspec.yaml is on disk, the help is returned much slower than other help commands/ output -- on the order of ~10 seconds instead of 0.07 seconds

[grouma]

Something else seems to be going on. As far as I know package:test doesn't read the pubspec.yaml. I took a look through the repo and it reads the pubspec.lock file here.

The main entrypoint for which we parse arguments and print usage is here. I would suggest playing around with the entrypoint to figure out what's going on.

Does dart test simply run pub run test? If so, is the requirement for a pubspec.yaml actually coming from pub? It would also explain the slow behavior because it's actually snapshotting the test runner.

[jwren]

Thanks Gary, transferred it to area-dart-cli, I'll take a look.

@jonasfj for the pub aspect

[jonasfj]

It would also explain the slow behavior because it's actually snapshotting the test runner.

Yeah, it runs pub run test as a subprocess. And pub run test will snapshot the source for the test runner. We used to generate this snapshot during pub get, but as of 2.8.0 snapshotting is lazy (this is good because some packages delivered binaries that users never called).

[mit-mit]

@jwren are you fixing this?

[jwren]

@mit-mit I was, but after the comment from @jonasfj I don't know what a solution here would be. Is it trivial to change the lazy-ness for certain packages in the SDK?

[mit-mit]

I don't understand well enough how dart test is implemented. But I do wonder if the test command be restructured to process the help option first, and then once it knows it's not just printing help, then do whatever it does which is so time consuming?

[jonasfj]

Is it trivial to change the lazy-ness for certain packages in the SDK?

By certain packages do you mean package:test, because users don't use it from the SDK. To use package:test and pub run test users have a dev_dependency on package:test. They can't run pub run test or pub run test --help before running pub get.

The test framework is versioned in the pubspec.yaml. So you can't print help before running pub get as you don't know what version of package:test the project will be using.

If there is no pubspec.yaml, pubspec.lock or .dart_tool/package_config.json file, or .dart_tool/package_config.json doesn't contain an entry for package:test. Then all dart test can reasonably do, is tell users to add a dev_dependency on package:test in pubspec.yaml and run dart pub get.

[jonasfj]

Unless we pin package:test in the dart-sdk for all packages (something like what flutter does).

I suspect the best thing to do is to simply check if following conditions are satisfied:

• pubspec.yaml exists,
• pubspec.lock exists,
• .dart_tool/package_config.json exists and contains an entry for package:test.

If neither of these is the case, we instruct users to:

Add package test under dev_dependencies in pubspec.yaml and run dart pub get.

[jonasfj]

dart test --help - because it goes to verify that a pubspec.yaml is on disk, the help is returned much slower than other help commands/ output -- on the order of ~10 seconds instead of 0.07 seconds

As for dart test --help being slow I don't think we can fix that. Short of making the compiler faster :)

package:test imports other packages that is resolved in pub get, we can either do snapshotting in pub get or pub run test, but that adds up the same. If we do it for executable scripts in pub get it's slower because not all executable scripts are used.

[devoncarew]

I think there are two issues here:

• we should provide a good message if dart test -h is run from a directory w/o a pubspec
• dart test -h runs slower than the other help commands; @bkonyi has an idea to improve that w/ how we spawn the pub snapshot

[mit-mit]

We could fix this by adding an command/args parser for dart test just for the purpose of providing help. cc @jwren

[jonasfj]

dart test -h runs slower than the other help commands

Fundamentally, we can't do much about the fact that the initial run of dart test has to compile a snapshot for package:test.

has an idea to improve that w/ how we spawn the pub snapshot

We're currently, looking into embedding pub as a library in dartdev.

[jwren]

https://dart-review.googlesource.com/c/sdk/+/158701

[devoncarew]

Hey, a drive by comment on the above CL:

I see that we're embedding the args parser from package:test in dartdev. That addresses the issue, but will mean that we (periodically? regularly?) get out of sync with the actual commands that package:test supports.

I think the main issue is that we don't fail well when there's no pubspec; dart test -h currently shows:

Could not find a file named "pubspec.yaml" in "/Users/devoncarew/projects/...".

We do handle the case where there is a pubspec, but the user does not yet have a dependency on package:test. We special case that, and print:

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#-installing-tab- for more information on
adding package:test, and https://dart.dev/guides/testing for general
information on testing.

I think a viable alternative to duplicating the package:test args parser into dartdev is to improve the failure message for cases where there's no pubspec (and likely also, the case where there is a pubspec and a test dep, but the user has not yet run pub get). We'd want to indicate what the command was for, how to satisfy whatever was currently wrong, ... It would be brief, helpful help text for dart test -h for the cases where we can't actually run pub run test -h.

[mit-mit]

As soon as we ship the first dart CLI version to stable I think we can go ahead and deprecate pub run test thus alleviating the need to keep them in sync?

[devoncarew]

Unless we make bigger changes, we'll still be running pub run test behind the scenes, we just may not recommend that path to users.

We don't know what version of package:test the user's version solve will select. It's tough to know which version of the test CLI UI to copy into dartdev; the tool will fail on new CLI args that test understands but that dartdev doesn't, and, fail on old CLI commands that dartdev claims to support but that test doesn't.

[mit-mit]

I'm indeed assuming we'll make bigger changes; actually bring the pub sources into dart pub and dart test and deprecate the existing pub client. @jonasfj might that happen in Q4?

[devoncarew]

I think for the test command what's more topical for a restructuring is how package:test loads and executes tests (as opposed to where the code for pub lives). The CLI UI and args processing comes in the the version solve w/ package:test; w/ my current understanding, we'd have to choose to split test execution logic between dartdev and the package:test version used by the user's code.

[jwren]

@devoncarew I didn't see your message here until after I pushed the CL. The other issue that is solved is that dart help test now returns in in milliseconds instead of seconds. Also, since the help is now native to dartdev itself, permutations with dart test --help versus dart help test versus other permutations with subcommands, etc, won't be encountered.

Having to update the help text to keep it in sync seems to me a small cost for the benefit.

[jonasfj]

@jonasfj might that happen in Q4?

It might, @sigurdm is working on it.

[devoncarew]

@jwren, can this be closed?

[jwren]

Yes, closing now.