Add a more precise way to start multiple specific tests at the same time

[rrousselGit]

Hello! Some context:

I am working on a custom test renderer and I was trying to implement "execute only failed tests from the previous run" among other things.
I was able to get the list of failing tests, with their suites and names/ids. But I haven't found a way to start them all at the same time while skipping the rest

I was innocently thinking that we could do:

dart test test/foo_test.dart --name "a" test/bar_test.dart --name "b"

to run the test "a" from foo_test and "b" from bar_test, but this isn't the case because of various issues:

• --name filters aren't specific to a test file.
• When multiple --name queries are defined, they perform an AND filter instead of an OR filter
• There doesn't seem to be a way to have a strict name filter (as in not perform a "contains" comparison but an ==).
  So --name "hello" will run both test('hello', ...) and test('hello world', ...)

As such I think thinking: Would it be possible to add a more specific filter?

If updating --name to behave as described isn't possible due to being a breaking change, what about something like:
dart test --test file=test/foo_test.dart name="a" --test file="test/bar_test.dart name="b"

I'd be more than happy to raise a pull request about this.

[jakemac53]

This is somewhat a limitation of the args package which we use for argument parsing - which follows general standards for the meaning of argument options and such. So I don't think we could do quite exactly what you describe but there should still be some way to support it.

This doesn't have to be a particularly ergonomic thing to use since I think its mostly only useful for automated tooling, so that also makes things a bit easier.

One idea would be to use the uri fragment to indicate the name, something like --exact_test=test/some_test.dart#exact_test_name. So you would pass an argument like that for every test you want to run. It would be verbose but that doesn't matter for a tool.

[rrousselGit]

I agree, your syntax proposal sounds good to me.

So can I work on adding such feature?
If so, any thought on how to get started? As I'm not too familiar with the codebase

[jakemac53]

I do think we should validate this will really give you the UX you desire first, and we don't need a more general feature.

For instance, how would you want to handle the case where a new test case is added, or a test case is renamed or deleted?

[rrousselGit]

If a "exact_test=..." clause doesn't match a test (such that when a test is deleted), I'd expect nothing to happen

I wouldn't expect this flag to deal with "execute new/renamed tests".

If folks want to execute the renamed/new tests, we could rely on the analyer to detect @isTest/@isGroup that weren't previously found and the command could pass the exact_test=... for those.
But for an MVP, I'd expect "don't execute renamed/added tests" to be enough. Folks could disable and re-enable the "run only failing tests" flag, which would refresh the list of failing tests and consider added/renamed tests.

[jakemac53]

Ultimately what I am implying here is that what we really need is something I have wanted for a long time - an interactive mode for the test runner, intended for use by tools. That would want to be able to do a lot more than just run a specific test and get test results.

I have a strong hunch that if we add this feature, we will end up wanting to add a whole bunch of other related features for all these other scenarios. And ultimately its probably better to design a whole new test command which is specifically designed with interactive tooling at top of mind, and all the various scenarios that would entail.

If a "exact_test=..." clause doesn't match a test (such that when a test is deleted), I'd expect nothing to happen

This does indicate something going awry though and it generally goes against our existing behavior which is to fail if no tests are ran. We probably would want to at least explicitly communicate that no test was found somehow.

I wouldn't expect this flag to deal with "execute new/renamed tests".

But I think your general tool probably does want to deal with that? People probably do want new tests to be ran, since they might be failing. Or at least that would be one of the user experiences you probably want to support?

If folks want to execute the renamed/new tests, we could rely on the analyer to detect @isTest/@isGroup that weren't previously found and the command could pass the exact_test=... for those. But for an MVP, I'd expect "don't execute renamed/added tests" to be enough. Folks could disable and re-enable the "run only failing tests" flag, which would refresh the list of failing tests and consider added/renamed tests.

I do agree there would be user value extracted from just this simple feature, and not handling the edge cases. But we also have to design features with the long term in mind since we very rarely make breaking changes.

That isn't to say I don't think we should do this, just that we should make sure it still makes sense in the bigger picture and for the long term.

[rrousselGit]

I understand where you're coming from.
Although that sounds like a lot more work than a slightly different way of filtering tests, in which case I may not have the time to work on this. One thing I like with this proposal is that it's decently flexible yet also seem simple to implement.

But if we're making a wish-list, I think it'd be ideal to offer a Dart API to manipulate test instead of having to spawn dart test.

It could be useful to be able to do something like:

void main() async {
  List<Suite> suites = await getSuites();
  List<Test> tests = suites.expand((suite) => suite.tests).where((test) => <custom test filter>);

  await runTests(tests);
}

Although that's a different project.

But I think your general tool probably does want to deal with that? People probably do want new tests to be ran, since they might be failing. Or at least that would be one of the user experiences you probably want to support?

Yes but as I mentioned, I'd expect the analyzer to support such edge-case

But in general I wouldn't worry to much about it. The "don't execute new/renamed tests" behaviour is how Jest works, and it's a good experience.
When folks use the "run only failing tests" mode, they don't typically rename/add new tests, since the goal of the flag is to quickly iterate over a few specific tests.

Supporting new/renamed tests would be for a different kind of filter, like file/name filters or Jest's git filter – which only executes tests impacted by the uncommited git diffs. But the filter is at the suite level, so the existing file filter is enough.

[jakemac53]

@natebosch wdyt?

Note we have similar but I think not overlapping discussions in #1579, where I proposed a similar solution. We will want to at least make sure these can be compatible (but I think that won't be an issue, they would use different option names).

[natebosch]

I'd expect the analyzer to support such edge-case

I don't think we'll ever be in a place where analyzer can have a full understanding of what tests exist. It would be too breaking to move from the test model of defining cases by running code into a more statically defined set of test cases.

I think putting a test name in the URI fragment is probably an OK approach, but it does seem like it has drawbacks.

Note we have similar but I think not overlapping discussions in #1579, where I proposed a similar solution.

We could consider using line numbers, or even an opaque ID that we control completely, if we want to support the use case even more directly by owning the translation from failure to test identity. Naming would need some thought, but as a strawman we could add flags like dart test --write-failing-test-list=some_file.json dart test --run-previously-failing=some_file.json.

[jakemac53]

I think putting a test name in the URI fragment is probably an OK approach, but it does seem like it has drawbacks.

What sorts of drawbacks?

[natebosch]

What sorts of drawbacks?

Mainly that it's hard to get right. Characters need to be URI escaped and the vast majority of test cases will have spaces. We could instead say we aren't using a URI fragment, but just a # followed by the name and then maybe users only need to worry about using quotes to make sure the args get split up right.

In any case, my intuition is that round tripping a test ID which we own 100% may be less brittle than defining a scheme that needs to be maintained backwards-compatible for a long time.

[jakemac53]

Mainly that it's hard to get right. Characters need to be URI escaped and the vast majority of test cases will have spaces. We could instead say we aren't using a URI fragment, but just a # followed by the name and then maybe users only need to worry about using quotes to make sure the args get split up right.

We would just be talking about a small number of tools (not package:test users) using this option. So I don't think its much of an issue to require the fragment be properly encoded.

In any case, my intuition is that round tripping a test ID which we own 100% may be less brittle than defining a scheme that needs to be maintained backwards-compatible for a long time.

We can't make a stable test ID afaik though in the face of new tests being added and such, unless we also base it off the name. So I don't really see what it buys us? But we could give the user a URL safe ID in the json protocol for each test, and then require them to pass that for the fragment instead?

[natebosch]

We can't make a stable test ID afaik though in the face of new tests being added and such, unless we also base it off the name. So I don't really see what it buys us?

Owning the definition of the file format would leave some room for adding more understanding of how tests change over time.

But we could give the user a URL safe ID in the json protocol for each test, and then require them to pass that for the fragment instead?

Yeah I think no matter what we need some way for users to have us tell them exactly what IDs would work (assuming the test cases also remain stable) including whatever URI escaping needs to happen. Whether that's in the JSON protocol or somewhere else should be fine.

[rrousselGit]

I'm not fond of using IDs, because that eliminates the possibility for users to use this filter option

I could see myself using the "exact_test" proposal without passing by a tool. I more than once had to comment the other tests in a file besides one specific test because --name executes more than this test due to performing a "contains".

[jakemac53]

I could see myself using the "exact_test" proposal without passing by a tool. I more than once had to comment the other tests in a file besides one specific test because --name executes more than this test due to performing a "contains".

Note that we do accept regex for --name so you can limit it that way - or you can use the the solo: true option on tests which is in some ways the easiest way to just run a single test temporarily.

[DanTup]

I'd be interested in something like this too. VS Code's new test runner lets a user multi-select a bunch of tests and run them, and today I had to make this split them by file and run multiple debug sessions to pass the names - which is a lot slower than running them all in a single process (not to mention clumsy for some other reasons, like if the user hits Stop on the debug toolbar it only stops one of them).

VS Code currently only uses regexes, though there's been talk of supporting line/col in another issue - if something was added, it could be nice to support both of those (since in the short-term, VS Code will likely continue to use names, but would probably migrate to line/col).

On the subject of an interactive/server process, it may be worth keeping an eye on microsoft/vscode#134455. VS Code has a new test runner and it seems to me like it would make a nice "Test Adapter Protocol" similar to LSP/DAP that could be run out-of-process and communicated with via JSON. LSP and DAP both have gotten good traction, so a common interface that other languages/editors might use could be a better option than something completely custom.

[rrousselGit]

On that note, is there anything I can do in the short term?

I originally wanted to fork test to add custom features, but I quickly realised that it'd basically require forking Flutter too

[jakemac53]

From my perspective here I would be happy with the --exact-test=<file-path>#<uri-encoded-test-name> option. But we would want to get @natebosch on board with that as well.

[DanTup]

@jakemac53 is it possible to support regex too? VS Code always uses regex right now (because it might be trying to run tests named "my $foo" where we don't know what foo is) but it'd be nice to fix the limitation of spawning multiple debug sessions for selections across files.

[DanTup]

(and by that I just mean a single regex for the file, since I'm already building a regex that covers all names like ^test name one$|^test name two$|^group name one)

[rrousselGit]

Alternatively would it make sense to try and fix the limitation of args to allow dart test test/foo_test.dart --name "a" test/bar_test.dart --name "b"?

[jakemac53]

Alternatively would it make sense to try and fix the limitation of args to allow dart test test/foo_test.dart --name "a" test/bar_test.dart --name "b"?

I don't think so, it is following pretty well established conventions for how command line options work. Options only accept a single value (not potentially multiple). Generally ordering of options is also not supposed to matter.

[rrousselGit]

There is a precedent though:

dart pub global activate --source path <path-to-source>

where --source takes two arguments and is based on the order

edit actually I'm not sure. Thinking about it, I never tried to use the command with different combinations

[jakemac53]

Fwiw I am always super confused by the way that --source works for pub global activate lol. Their implementation also wouldn't work for multiple options - the args package does support trailing arbitrary arguments at the very end of the list, which is how they deal with it. But that only works for a single option like that, and it has to be the final option supplied as well.

[rrousselGit]

From my perspective here I would be happy with the --exact-test=<file-path>#<uri-encoded-test-name> option. But we would want to get @natebosch on board with that as well.

Coming back to this, why would the test name be URI encoded?

I suppose you meant --exact-test=test/foo_test.dart#hello%20world instead of --exact-test=test/foo_test.dart#"hello world"

Is there a special reason?

[jakemac53]

There could be symbols in there that need to be encoded or else they would have other meaning in bash etc such as @. I suppose we wouldn't have to require you to encode it in the case that Uri.parse will work anyways, but tools should encode it.

[rrousselGit]

Oh interesting, I didn't know we could do Uri.parse('<file path>#name')

But if we encode the test name, wouldn't this hinder the ability to use regex as name as @DanTup wanted?

[DanTup]

I don't think the encoding affects using regex or not. There may need a way to distinguish what is/isn't a regex, but I think the encoding is just to ensure the arguments parse correctly (and aren't tripped up by the wonderful characters people like to put in their test names - which FWIW can include newlines :-) )

Perhaps one option would be to use the query string:

<file>#<name>
<file>?regex=<name regex>

This might also work nice for line/col as discussed previously:

<file>?line=1&col=4#<name>
<file>?line=1&col=4&regex=<name regex>

(with all the correct URI encoding, ofc)

[jakemac53]

Using the query string does afford us a lot more flexibility and allow us to solve these identical issues with a common solution so I think that is great.

Should we just allow query strings on general file names, so they are passed as rest arguments and not via an extra option then?

dart test test/my_test.dart?name=foobar etc? It looks like this would also allow passing the same query string multiple times and we can get that out using Uri.queryParametersAll. So that is another win.

[DanTup]

It looks like this would also allow passing the same query string multiple times

I presume you mean so we can do:

dart test test/my_test.dart?name=foo&name=bar

? It might be a bit strange for cases where there's some relationship between them:

# Which are ANDs and which ORs?
dart test test/my_test.dart?line=1&col=1  # expect and?
dart test test/my_test.dart?name=foo&name=bar # expect or?

Although if the rules are clear and we could also pass them as separate URIs if it didn't fit the needs, sgtm :-)

[rrousselGit]

I don't quite understand why passing multiple name/lines in the same --exact-test flag is desirable, since we can pass the flag multiple times

[rrousselGit]

TODO: Should we allow multiple name query parameters?

If they perform an and, sure, as this is how --name behaves. If we're expecting an or, that'd be confusing I think

[DanTup]

SGTM.

I also agree that having multiple name in a query being or'd would be confusing, so if it is allowed, I think it makes sense to and. I'm not sure if that's terrible useful, but it seems least surprising/requiring less explanation/documentation. I suspect the significant majority of user of this will come from tools, where duplicating the filename is not any extra effort but avoids the ambiguities.

[jakemac53]

STGM to amend the proposal to allow multiple values for name specifically, but all must match. Probably won't see much use but doesn't hurt. The others have no value at all in allowing multiples though (assuming "and").

[jakemac53]

@natebosch thoughts on the above #1598 (comment)?

[jakemac53]

It occurs to me that we also need to define how these options interact with the global options. I think I would say the global options also must match. So they are respected as well, and these file level filters apply to the results left over after the global filters.

[rrousselGit]

I would expect this option to be incompatible with others

So that if you specify --exact-test, the command fails if you specify files or other options

[jakemac53]

So that if you specify --exact-test, the command fails if you specify files or other options

Note that my updated proposal removes this argument - it just adds query parameter support to the existing file path arguments which are just rest arguments.

I think we do want to support the other global arguments still, it shouldn't add much complexity (actually may even be simpler in some ways).

[DanTup]

If you mean dart test foo.dart?name=foo --name bar then I also agree both should match. I'm not sure it'll get much use either, but it seems logical to me.

It may be nice to print a good message when no tests match that describes how the conditions were applied, to make it obvious.

[rrousselGit]

Oh, I didn't realise that the syntax changed.

I see, then dart test <path>?query sounds good to me. That's closer to what I originally wanted

[rrousselGit]

Would this be considered as a breaking change though? As file paths could technically contain ?

Someone could name a file "foo.dart?name=42" 🤷

[jakemac53]

Would this be considered as a breaking change though? As file paths could technically contain ?

That should be uncommon enough for us to not consider it breaking? Granted people do weird things...but that would be very weird, especially in a dart file path.

[natebosch]

This proposal LGTM overall.

We could also consider allowing the abbreviated argument flags dart test test/some_test.dart?n=foo.

• line: Run all tests whose root_line == line.
• col: Run all tests whose root_col == col.

We aren't proposing to implement this today are we? I think we'd implement support for the name regex and full string matching now and leave space for new query types in the future.

That should be uncommon enough for us to not consider it breaking?

We could also have a fallback if the argument can't be parsed as a URI to check if it exists as a file path. I think the only way we'd change existing (passing) behavior is if you happen to have a file path with a ? which is also a valid URI.

[jakemac53]

We aren't proposing to implement this today are we? I think we'd implement support for the name regex and full string matching now and leave space for new query types in the future.

We could wait, but we have another open issue to support that so we could roll it into the same release imo.

[natebosch]

I do think if you use this approach the full argument should be URI safe, not just the query string. It would mean on windows you'd need to use / instead of \, but it would cut down surface area for bugs and misinterpretation. If there is any weirdness in the path like a ?, it can be handled by the user by URI encoding the path components as well.

[natebosch]

we could roll it into the same release imo.

That's fine if we can get them both done, but I wouldn't gate one on the other.

[jakemac53]

@rrousselGit were you still interested in taking a shot at implementing this?

[natebosch]

It may be nice to print a good message when no tests match that describes how the conditions were applied, to make it obvious.

#651

[rrousselGit]

@rrousselGit were you still interested in taking a shot at implementing this?

Definitely. Although I don't really care about line/col options for now. I'm only after the name/full_name variants

[jakemac53]

Definitely. Although I don't really care about line/col options for now. I'm only after the name/full_name variants

That is fine, if you wanted to just implement the name/full_name variants we can follow up with line/col later

[rrousselGit]

As for the implementation, I suppose it involves modifying Configuration.path and _Parser.parse.patterns.
Then update Runner._loadSuites to handle the new Configuration.path logic. Is that correct?

[jakemac53]

As for the implementation, I suppose it involves modifying Configuration.path and _Parser.parse.patterns.
Then update Runner._loadSuites to handle the new Configuration.path logic. Is that correct?

Essentially yes, the paths should now be Uris instead of Strings, and the filtering should happen inside the suite.filter callback inside of _loadSuites.

It might be a little bit funky to pass the extra information along, you might want to merge it into the SuiteConfiguration object that is created for each suite (_config.suiteDefaults).

[jakemac53]

It might be a little bit funky to pass the extra information along, you might want to merge it into the SuiteConfiguration object that is created for each suite (_config.suiteDefaults).

Ya it looks like you could just use the existing patterns field (create an exact match regex for full_name). Create a new suite config with just that option specified and merge it with the _config.suiteDefaults. That would automatically give us the merging of global + individual options we want also.

[rrousselGit]

Coming back to this, I may be missing something but it seems like dart test test/my_test.dart?name=foo causes problems with zsh

We have to do:

dart test "test/my_test.dart?name=foo"

[rrousselGit]

Other than that, I think I got it to work. I'll raise a PR, but I'm not familiar with how to write tests for it.
Some pointers would be nice :)

[DanTup]

FWIW, I'm not desperate for line/col either so also won't hold this up for it. It would allow me to handle some edge cases better when users have duplicate names, but since we're not supporting them in the tree, I'd generally encourage users not to have them (I used it as an example above just to highlight some benefit of using a querystring).

it seems like dart test test/my_test.dart?name=foo causes problems with zsh

I'm not very familiar with it, but I guess there's some sort of shell expansion going on? For tools this probably isn't an issue (eg. when you spawn a process from Dart or nodejs, you pass an array, and each item should come through normally to the program). I think it's probably reasonable to expect users to quote/escape things correctly if doing this themselves in a shell?

[jakemac53]

Ya it doesn't surprise me you would have to explicitly quote it, a bit annoying but 🤷‍♂️