Skip to content

Support filtering by specific group names instead of only full (group + test) names #908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DanTup opened this issue Jul 18, 2018 · 18 comments

Comments

@DanTup
Copy link
Contributor

DanTup commented Jul 18, 2018

Currently --plain-name matches any substring of tests which is great for users typing, but less great for editors wanting to offer a quick way of running tests. For ex., take this sample suite I got from the readme:

import "package:test/test.dart";

void main() {
  group("String", () {
    test(".split() splits the string on the delimiter", () {
      var string = "foo,bar,baz";
      expect(string.split(","), equals(["foo", "bar", "baz"]));
    });

    test(".trim() removes surrounding whitespace", () {
      var string = "  foo ";
      expect(string.trim(), equals("foo"));
    });
  });

  group("int", () {
    test(".remainder() returns the remainder of division", () {
      expect(11.remainder(3), equals(2));
    });

    test(".toRadixString() returns a hex string", () {
      expect(11.toRadixString(16), equals("b"));
    });
  });
}

I initially implemented a Run button next to group in Dart Code. However if you run the String group here, it also runs one of the int tests because it has string in its name.

It would be great to support some sort of exact-match which will only run a group or test if it matches exactly (it should support running all tests in a group if the string matches the group name exactly).

@jakemac53
Copy link
Contributor

Note that -n does support regex, so you could probably use a regex instead which is more specific (^String in this case?)

@DanTup
Copy link
Contributor Author

DanTup commented Jul 18, 2018

Oh, I'll give that a try - the help text still said it was a substring so I didn't think about that. Could I do the same with a $ (or w/e) on the end?

@jakemac53
Copy link
Contributor

Yep, it should support everything dart regex supports I would guess

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

This doesn't seem to work:

screen shot 2018-07-19 at 12 42 15 pm

Looking at the source, I can't see an obvious reason why.. I'll do some more digging..

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

Oh, I see the problem - it matches against the full name (all groups and test name joined). I'll see if I can join the group names in before I send (and I guess for a group I can put a wildcard on the end to run all the tests within).

It's a bit of a tangent, but somewhat related... There are a couple of issues we've discussed that make test less tool friendly than it could be (for ex. stable IDs for tests, being able to get a list without running, etc.). I wonder if it'd make sense to have some sort of daemon (or an analyzer plugin) that we could send commands to for running tests (and it could provide a list of tests, stable IDs, notification of the test list changing, w/e)?

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

So, the regex works, but there's a slight niggle with this solution is when running a group you can't put $ on the end, so it'll match and other group/tests that happen to have this groups full name as a prefix. Eg.:

group("test" {});
group("test 2" {});

As far I can see, you can't run group without also including group 2.

@jakemac53
Copy link
Contributor

It's a bit of a tangent, but somewhat related... There are a couple of issues we've discussed that make test less tool friendly than it could be (for ex. stable IDs for tests, being able to get a list without running, etc.). I wonder if it'd make sense to have some sort of daemon (or an analyzer plugin) that we could send commands to for running tests (and it could provide a list of tests, stable IDs, notification of the test list changing, w/e)?

I think those are all reasonable requests, but we don't have a lot of extra resources to devote to that right now. What we are working on in the short/medium term is exposing more imperative apis from the test package to help improve the situation with regards to custom platforms, plugins, runners, etc.

It could actually be that you are able to use that work in order to do all these things within your own process though - although they will all be dart apis which complicates things since the vscode plugin is written in typescript.

@jakemac53
Copy link
Contributor

So, the regex works, but there's a slight niggle with this solution is when running a group you can't put $ on the end, so it'll match and other group/tests that happen to have this groups full name as a prefix.

Ya I think you would have to explicitly specify the exact full name of each test you want to run (the nested groups + test name, joined with a space).

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

It could actually be that you are able to use that work in order to do all these things within your own process though - although they will all be dart apis which complicates things since the vscode plugin is written in typescript.

We do require there's a Dart VM, so in theory running a couple of Dart scripts isn't totally out of the question (ofcourse, the version of the VM, restoring packages, etc., could be a little more complicated).

Is there any info on the work being done that I could review?

Ya I think you would have to explicitly specify the exact full name of each test you want to run (the nested groups + test name, joined with a space).

I don't think that'll work, the args help says

If passed multiple times, tests must match all substrings.

This seems less useful than the opposite to me (since tags allow you to run groups if that's what you wanted?)

@jakemac53
Copy link
Contributor

We do require there's a Dart VM, so in theory running a couple of Dart scripts isn't totally out of the question (ofcourse, the version of the VM, restoring packages, etc., could be a little more complicated).

Is there any info on the work being done that I could review?

Not sure if we have any issues filed right now, @grouma ?

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

Ok, well do ping me if you start on anything you think might be of interest :-)

@jakemac53
Copy link
Contributor

jakemac53 commented Jul 19, 2018

Regarding the regex that is unfortunate that it must match all of them - doesn't seem right on the surface but also probably not something we can change (and I don't know the original reasoning).

I think you could however construct a regex using non-capturing groups, for instance something like this ^(?:(?:foo)?(?:bar)?)+$ will match only exactly foo, bar, and also unfortunately the empty string, but probably some way around that too and its a pretty rare edge case to have an unnamed test (or should be at least!)

edit: hmm actually that regex also matches things like foobar and foofoo, i am no regex expert but probably ways around that as well.

@lrhn
Copy link
Member

lrhn commented Jul 19, 2018

Ob-RegExp comment:
You can match either foo or bar just as ^(?:foo|bar)$. The RegExp above is overkill and also matches "foobar".

If the --name matcher is matched against the full test name, then there is no simple way to figure out the original separation into group and test names. Also, test names are not necessarily unique:

group("a", () { test("b", () {}); }
test("a b", (){});

One option might be to allow the name test to be applied group names and individual test names as well as full test names. If it matches a group's name, then the entire group is included. That does require some way to find the group of a test (and the group of a group).

@jakemac53
Copy link
Contributor

thanks @lrhn for the regex-fu!

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

Heh, that might work - though I'm slightly worried about how fragile this might get. There are extra steps this is going through:

  • I'm having to parse strings out of the analysis server outline for the Code Lens links and it gives me a string like test("blah") so there's all sorts of escaping issues I might have trying to get the name out (I don't really want to duplicate and maintain Dart's string parsing logic in TypeScript)
  • This is executed via a shell, so everything in the names/regex need to be escaped correctly across all platforms (and between nodejs and Windows)

Though maybe I can avoid some of these issues by replacing all non-alphanumerics with a . for the regex... I'll do some more testing.

I wonder if a relatively simple way to handle this is if hashes of the names are provided (as discussed in the "stable IDs" issue) and then there's a way to run tests by hash. That could also support multiple without having to change the name ones:

pub run test --hash abc --hash def

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

If the --name matcher is matched against the full test name, then there is no simple way to figure out the original separation into group and test names

Yeah, I think the best way can do without changes is to minimise, but not eliminate, the false runs.

If we can agree it's a reasonable option, I'd be happy to take a stab at adding some sort of hash to each test/group (based on the name) and an extra flag that allows specifically running them. To reduce the collisions we could hash with some separator between the group/test names and/or keep a hash set and add a incrementing suffixing if we come across a collision.

@DanTup
Copy link
Contributor Author

DanTup commented Jul 19, 2018

Hmmm, that stable IDs thing doesn't actually help a lot with some of what I was implementing here. I'm using the analysis servers Outline data to inject Run/Debug links into the document - so I don't have any data from the test framework.

It'd still probably help improve the tree, but I think "Code Lens" is still going to be a bit hit and miss.

@natebosch natebosch changed the title Support a version of --plain-name that does exact matching (for tooling) Support filtering by specific group names instead of only full (group + test) names Jul 19, 2018
@natebosch
Copy link
Member

I updated the issue title to what I think reflects the current request. Today you can't match by group names, only by the full string that results after merging groups and test names. I do think it's reasonable to want to filter also by group names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants