libtest: add glob support to test name filters

[jonhoo]

Tests can currently only be filtered by substring or exact match. This makes it difficult to have finer-grained control over which tests to run (#46408), such as running only tests with a given suffix in a module, all tests without a given suffix, etc.

This PR adds support for glob patterns in test name filters. When at least one of the special glob characters (*, ?, or [) are present, the pattern is interpreted as a glob, and used to match against test names. These characters cannot appear in ordinary test names, so this should not cause unexpected results. This allows commands such as:

mymod::*_fast

or

foo --skip *_slow

Fixes #46408.

[rust-highfive]

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

[jonhoo]

One question that should probably be resolved: currently, globbing is enabled either if --glob is passed, or if the pattern includes a glob special character. But, consider an invocation like:

foo --skip *_slow

With the tests foo, foo_slow, and foo_fast. The user probably expects foo and foo_fast to be run, but this will actually run all three tests. foo doesn't contain any globbing characters, to globbing isn't enabled, and substring matching is used instead. This means that the skip patterns will also not use globbing, and will match the literal string *_slow, which doesn't match the name of any test. Worse yet, this isn't fixed by adding --glob; that would cause foo to be treated as a globbing pattern (not a substring match), and so only foo (and not foo_fast or foo_slow) would be matched. This suggests that to really fix this issue, we'd need to track whether each individual pattern should be a glob, which becomes overly complex.

All this taken together, I suggest we keep things the way they are currently (only infer from main pattern or presence of --glob) I believe that that's the behaviour that is least surprising and most intuitive. But I'm open to suggestions.

[jonhoo]

I changed my mind. Latest commit interprets patterns independently, so the example given before

foo --skip *_slow

will now work correctly (it'll include foo_fast and foo, but not foo_slow). I think that's more intuitive. --glob and --exact apply to all patterns.

[jonhoo]

This PR should probably also be tagged with relnotes if accepted.

[jonhoo]

Surprisingly tricky to figure out who the right person for review on libtest is :p Neither #rust-dev-tools, #rustc, or #rust-libs seems to know. Based on past PRs to libtest, let's try:
r? @Mark-Simulacrum

[Mark-Simulacrum]

Code looks good in general. However, I'm not sure why we need the --glob flag -- it seems to do nothing, as we already infer globs in all cases when they could have an effect based on the documentation: https://docs.rs/glob/0.2.11/glob/struct.Pattern.html.

Could you explain that?

In terms of review, it seems like we may want to make this unstable, but I don't know if that's possible for libtest parameters. If we can't make it unstable then we should get all of libs team to sign off on it at least.

[jonhoo]

Good point -- I removed the flag. Technically the flag changes the behaviour of strings that do not contain the special characters, since globs required a complete match. Non-glob matches are substring matches by default. But I think you're right that in practice the --glob flag wouldn't be used.

I don't know of a way to make libtest parameters unstable, except by manually adding eprintln! to the code, but that doesn't seem right. Sign-off from the libs teams seems like a good idea!

[Mark-Simulacrum]

@rust-lang/libs thoughts? This seems good but I'd want sign off from everyone since it'll probably be insta-stable.

[BurntSushi]

This seems like a fine idea in principle. I think my only concern here would be the stabilization of the glob syntax itself. Aside from the fact that the glob crate isn't 1.0 yet, one key piece of common syntax that the current glob crate doesn't support is the curly bracket syntax, e.g., {foo,bar} to mean foo or bar. For example, today, the following program prints true for both matches, but I'd expect different results with a crate that supported curly bracket syntax:

extern crate glob;

use glob::Pattern;

fn main() {
    let pat = Pattern::new("{*").unwrap();
    println!("{:?}", pat.matches("{foo"));
    
    let pat = Pattern::new("{foo}").unwrap();
    println!("{:?}", pat.matches("{foo}"));
}

Would we consider this an acceptable breaking change? If so, then I suppose it seems fine to move forward. (In the sense that I can't think of any other concerns with the public facing interface.)

[jonhoo]

@BurntSushi I'm not too concerned about that for the same reason I'm not concerned about introducing automatic interpretation of strings as globs in general: test names cannot contain {, and so no existing patterns that are used to match test names should stop working because of such a change.

[alexcrichton]

Historically the test crate actually had support for full-blown regexes and that was just removed in the lead-up to 1.0 where we moved the regex crate to crates.io instead of in the standard library/libextra. In that sense there's definitely desire for this!

That being said I'm wary of using the glob crate as I think it can be difficult to specify what's happening (glob is quite old and somewhat unmaintained at this point). I'd personally be much more comfortable with the regex crate, but I'm also not sure we'd want to pull that into the distribution. I know long-term we've always wanted custom test frameworks to be a thing but we've been saying that for years now...

So I guess tl;dr; I'd ideally prefer for the extra support we add here to be specifyable (aka have documentation and be supported) which I think rules out glob but leaves open regex perhaps? Ideally though I'd still hold off on adding regex and instead add support for custom test frameworks, but that thought may be a bit of a pipe dream at this point!

[jonhoo]

I'd love having full regex support (similar to go test), but share your concern with folding in regex. glob presented a nice alternative given that it is already used elsewhere, and probably won't go away for a while. The syntax is also pretty simple and straightforward.

Custom test runners would be nice, I agree, but as you say we have been wanting that for a long time, and it doesn't seem like much is changing any time soon. It's also nice for the default thing that is run with cargo test to support something beyond just substring matching. And I think glob fits that nicely.

I think glob is a fairly familiar, and widely-documented (outside of Rust) format for these kinds of tasks. The fact that the crate is not maintained and not super well-documented is a problem, I agree, but something that could (and should) probably be fixed separately from this.

[jonhoo]

Gentle ping @alexcrichton

[alexcrichton]

Ah sorry about that. I definitely understand where you're coming from but for me maintainability of a crate like libtest trumps almost all else, so the fact that glob isn't actually specified and is barely maintained (afaik) is a serious mark against it. The current libtest is very minimal and we've ended up getting pretty far with it. In that sense I would still personally prefer to not land major new features to libtest and continue to hold out for custom test frameworks. I realize that it's still pretty far out, but my hope is that we can add fuel to the fire of designing it instead of getting into an unfortunately unmaintainable state of libtest in a few years time.

[jonhoo]

Hmm, that's unfortunate, but I understand the reasoning. Is there any particular place to monitor the progress of custom test runners, and potentially to contribute/discuss directions going forward?

[alexcrichton]

For posterity, the conversation here was resumed on IRC

[carols10cents]

Also relevant: https://internals.rust-lang.org/t/past-present-and-future-for-rust-testing/6354/18

Putting this in the libs team's court for triage purposes :)

[jonhoo]

@carols10cents hehe, yes, that was the follow-up to the IRC follow-up. @steveklabnik mentioned that he wanted to bring it up during the work week, so hopefully we'll see some further discussion coming out of that.

[alexcrichton]

Ok I'm going to close this for no in favor of @jonhoo's thread on internals