Add support for -D arguments

[rxlabz]

even if dart-lang/sdk#44562 is closed, it seems actually not possible to define environment variables fort the dart test command ?

[lrhn]

The flag should be named --define or -D, not --dart-define (that's Flutter's name, Dart itself doesn't need to say that).

Have you tried dart -Dfoo=bar test?

[rxlabz]

yes also tried with -D but the result is Could not find an option with short name "-D".

( Dart SDK version: 2.12.3 (stable) (Wed Apr 14 11:02:39 2021 +0200) on "macos_x64" )

[rxlabz]

the workaround seems to be :

export DART_VM_OPTIONS='-Dfoo=true' && pub run test

but it's not working with dart test,

export DART_VM_OPTIONS='-Dfoo=true' && dart test

final foo = const bool.fromEnvironment('foo'); // with dart test foo is false

[bkonyi]

cc @natebosch @grouma

[natebosch]

dart -Dfoo=true test will do what you are hoping.

Importantly pass the -D after dart, not after test in the command.

[bkonyi]

dart test -Dfoo=true should also be valid, as that support has been added for other commands (see dart run and the various dart compile commands).

[natebosch]

I'm not eager to support general dart arguments in the test runner - it would start cascading into complexity across all platforms.

I'll leave this issue open in case we decide later to add support for -D defines. If we decide to handle this at the test runner level it would imply a breaking change and some work needed for test platforms like flutter.

@jonahwilliams - does flutter test support -D arguments, and in what positions?

[jonahwilliams]

It does:

flutter test -h

...

    --dart-define=<foo=bar>           Additional key-value pairs that will be available as constants from the
                                      String.fromEnvironment, bool.fromEnvironment, int.fromEnvironment, and
                                      double.fromEnvironment constructors.
                                      Multiple defines can be passed by repeating "--dart-define" multiple times.

Note that if you keep the cached dill floating around you need to account for the exact defines used for it.

[bkonyi]

If dart -Dfoo=bar test foo.dart works then I think all we have to do is define -D/--define in the test runner argument parser so it's considered a valid flag. The VM looks for VM flags in the command argument list, so as long as the command doesn't complain about an unrecognized option it should just work. It's also not clear that -D can't be provided to dart test since dart help test doesn't delegate to the test runner argument parser.

[jakemac53]

I don't think that with the new test loading strategy this would work either way? It may have accidentally worked previously, on the vm only, because we used data isolates and they inherited the defines. But that approach is actually setting the defines for the test runner itself as well which really isn't right.

We also initialize from previously compiled dills and it is likely that we don't properly account for changes to the -D variables (well I know we don't do anything, idk if the frontend server notices a change automatically or not and invalidates the dill).

[natebosch]

I think all we have to do is define -D/--define in the test runner argument parser so it's considered a valid flag. The VM looks for VM flags in the command argument list, so as long as the command doesn't complain about an unrecognized option it should just work.

This is not the way the VM works today. If I ignore the -D in the test runner argument parsing the VM will ignore it if it comes after test. dart -DFOO=bar test and dart test -DFOO=bar do not have the same behavior.

It's also not clear that -D can't be provided to dart test since dart help test doesn't delegate to the test runner argument parser.

I don't think it's a good idea in general to support different arguments between pub run test and dart test, but you could add that support in the dart test argument parser if you have a strong preference.

[bkonyi]

I'm not too familiar with how the test runner works, but the VM will parse VM options provided after commands before starting the CLI isolate. This is completely independent of the argument parser for the specified command, but the argument parser needs to accept the VM option as a valid option for the command so we don't error out. If the test runner just spawns another isolate in the original VM process, this should be the only change needed in the dart test argument parser.

I don't think it's a good idea in general to support different arguments between pub run test and dart test, but you could add that support in the dart test argument parser if you have a strong preference.

While I agree on principle, the plan is to deprecate pub run test in favor of dart test, is it not? I don't think it's terribly important for the standalone tooling to support this, but the CLI commands should be consistent with one another.

The other, slightly unrelated, issue is that the dart test argument parser is extremely bare bones and doesn't actually verify arguments / have a useful usage message.

[natebosch]

the plan is to deprecate pub run test in favor of dart test, is it not?

If I recall correctly that was not a plan that our team signed off on, but it may have become the plan since it was last discussed.

The other, slightly unrelated, issue is that the dart test argument parser is extremely bare bones and doesn't actually verify arguments / have a useful usage message.

Yes, we are signed on to improve the UX for things like the usage output. I think we have some open issues around that but I haven't dug deeply yet.

[bkonyi]

If I recall correctly that was not a plan that our team signed off on, but it may have become the plan since it was last discussed.

I think this is covered by dart-lang/pub#2736? Would there be any strong reason not to deprecate pub run test in favor of dart test in the long run? We don't need to drop support immediately, but ideally we'd nudge users to use the CLI instead of standalone scripts.

Yes, we are signed on to improve the UX for things like the usage output. I think we have some open issues around that but I haven't dug deeply yet.

Okay cool, just making sure! Thanks :-)

[natebosch]

Would there be any strong reason not to deprecate pub run test in favor of dart test in the long run?

I think the main reason is the implementation of the excutable will always be in the pub package (so that we can make breaking changes with versioning under the user's control). If we want to hide it such that dart pub run test doesn't work we'd need to write a new mechanism to invoke pub executables that only the dart command can use which doesn't seem to carry it's weight IMO.

If the "deprecation" is limited to a message saying "you may want to invoke this with dart test instead" then that should be fine.

[bkonyi]

If the "deprecation" is limited to a message saying "you may want to invoke this with dart test instead" then that should be fine.

Yeah, that's basically it. We won't be removing functionality from tools, just removing the standalone scripts in favor of the CLI commands at some point in the future.