Add expression based test filtering #117

Guiguiprim · 2022-03-18T16:48:31Z

As you are up apparently, I push this to open the discussion.

Following what you said, I was thinking maybe something along the line of:

DSL

package(name)
deps(package_name)
rdeps(package_name)
test(partial_name)
not(expr)
expr && expr / expr and expr
expr || expr / expr or expr
(expr)

Like -E "package(driver) or test(use_driver_)", -E "package(utility_package) and rdeps(utility_package)"

sunshowers · 2022-03-18T17:37:07Z

Your general design looks great. Let's do:

not x to match and and or, along with a short form ! x
x & y as the short form for x and y (not &&) because this is similar to BitAnd for sets
x | y and x + y for x or y for similar reasons
x - y for x and not y
all() for all tests and none() for no tests, sometimes useful

-E "package(utility_package) and rdeps(utility_package)"

Note that this performs the intersection of the two sets, which using the and operator honestly makes a bit less clear.

Some other questions:

What sort of parser were you planning to use? My personal suggestion would be to use a parser combinator, and if you do so, nom would be my first suggestion (especially since nextest already uses it as a transitive dependency).
How are you planning to do error handling? Ideally we'd integrate with something like miette, following this suggestion by Kat.
What is the precedence order among operators?
It should be possible to pass in multiple expressions, and I'd expect the result to be the union of all of the tests selected by them.
Sorting tests by some specific key might be an interesting thing to do in the future, but we probably shouldn't worry about that right now.
I would love it if we could support regexes as well. I think the easiest way is probably to use an re: prefix for regexes, and literal: for literals -- usable wherever a name can be specified. However, this creates an additional challenge -- how to use regexes that have () in them? I'd probably use r'' syntax for them, e.g. package(re:r'ba(r|z)')

What do you think?

Guiguiprim · 2022-03-18T17:57:41Z

You are right, I meant all the tests in package utility_package and all the tests using utility_package, we should be careful not to make it error prone.

I was thinking of nom !
I didn't though about precedence yet. But I will be careful about it.
I can look at miette
I had the same reflection about regexes, it adds parsing challenges, but yes r'..' looks right. I guess I would consider literal: default and optional in that case.
Multiple expression sound fine.

"Sorting tests by key" you meant filtering on test names ? I guess I can keep it for the end, but as it might be useful for me I'm inclined to do it.

I probably won't have much time in the days to come, so this will be somehow slow to come.

sunshowers · 2022-03-18T18:50:41Z

"Sorting tests by key" you meant filtering on test names ? I guess I can keep it for the end, but as it might be useful for me I'm inclined to do it.

I mean sorting the order in which tests are run.

Guiguiprim · 2022-03-21T20:02:50Z

I worked on a first draft of DSL definition and parsing, if you want to see where I'm going.

Nothing finished yet, but I have a first version of a full parsing.

Guiguiprim · 2022-03-22T12:01:58Z

I finally changed a bit the syntax:

package(name) => contains name
package(=name) => equal name
package(/name/) => matches regex name

This makes it easier to parse and (I think) easier to use. /../ is already a convention in some languages for regex.

The current limitations are:

equal and contains can not contains )
regex can not contains /

In both cases, this should not matter as they are both characters that can not be part of a test or package name.

Guiguiprim · 2022-03-22T16:54:17Z

I have a first draft of filtering.

deps() and rdeps() are not implemented yet, but everything else is, so the following works:

-E "package(runner)"
-E "package(=cargo-nextest)"
-E "package(/nextest-.*/)"
-E "test(parse) + test(basic) - test(env)"
-E "all()"
-E "none()"
-E "test(parse) & test(expr_)"
-E "test(expr_) | test(cargo)"

Still todo:

deps() / rdeps()
Proper parsing error handling
Support multiple -E <expr>
Unit tests around matching
Find a smart way to condition cargo metadata --no-deps usage if possible

As of now, I have issues implementing deps(), guppy::graph::DependsCache::depends_on() doesn't seem to work...

I opened an issue for this facebookarchive/cargo-guppy#632

Guiguiprim · 2022-03-22T19:19:32Z

So it looks like I need to find out why my test of -E "deps(runner)" runs no tests...

sunshowers

This is looking great -- I'm really excited about how this is progressing!

nextest-runner/src/test_filter/expression.rs

nextest-runner/src/test_filter/expression_parsing.rs

Guiguiprim · 2022-03-22T21:56:10Z

I'm done with my compilation optimization passes, if you want to have another look.

sunshowers · 2022-03-23T00:33:40Z

Thank you—I have other stuff coming up within the next couple of days but will try and take a look soon.

Guiguiprim · 2022-03-31T16:57:27Z

I worked on parsing error with miette.

it does make the parsing code more complex but the output is somewhat nice.
I also moved all the filtering stuff in a new crate.
I added some parsing tests

I still need to add tests on the matching itself.

Guiguiprim · 2022-04-02T14:48:45Z

Here we go, I:

Added support for multiple expressions
Added filtering tests
Conditionally included --no-deps in the call to cargo metadata
Made the feature experimental
Added a first draft of documentation (this is not my stronger suit)

At this point all is working, and I'm ready for reviews and critics !

sunshowers

Thank you! It might take a little while for me to get through this, have some errands today and medical things for the rest of the week. The other maintainer is busy as well :)

sunshowers

Looking fantastic! Just a couple more changes and I'm happy to finish up the rest and get this out there!

Cargo.toml

cargo-nextest/src/errors.rs

nextest-filtering/Cargo.toml