implement "only-<platforms>" for test headers #47487
Conversation
This patch implements "only-<platforms>" for tests headers using which one can specify just the platforms on which the test should run rather than listing all the platforms to ignore using "ignore-<platforms>". This is a fix for issues rust-lang#33581 and rust-lang#47459.
src/tools/compiletest/src/header.rs
Outdated
| true | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
if x { true } else { false } can simply be x...
kennytm
added
the
S-waiting-on-review
Previous patch introduced something like if x {true} else {false} which can be
simply replaced by returning x here.
Thanks to @kennytm for spotting it.
src/tools/compiletest/src/header.rs
Outdated
| @@ -564,6 +566,13 @@ impl Config { | |||
| } | |||
| } | |||
|
|
|||
| fn parse_cfg_prefix(&self, line: &str, prefix: &str) -> bool { | |||
There was a problem hiding this comment.
Can you rename this to something like has_cfg_prefix? It's not really parsing right now.
There was a problem hiding this comment.
Sure. Can you point what's wrong with parsing? Thanks.
There was a problem hiding this comment.
There's nothing wrong, just that a function returning bool seems like it shouldn't be called a parse_* function since it's not actually returning the results of the parsing, so to speak. I don't feel too strongly about this but has_cfg_prefix seems clearer to me.
There was a problem hiding this comment.
I misunderstood, I will update the function name.
src/tools/compiletest/src/header.rs
Outdated
| @@ -44,6 +44,8 @@ impl EarlyProps { | |||
| props.ignore = | |||
| props.ignore || | |||
| config.parse_cfg_name_directive(ln, "ignore") || | |||
| (config.parse_cfg_prefix(ln, "only") && | |||
| !config.parse_cfg_name_directive(ln, "only")) || | |||
There was a problem hiding this comment.
Why do we need to call parse_cfg_prefix here but not for ignore? What makes only special?
There was a problem hiding this comment.
This checks is there any //only-<> specification present and if so, does it matches with current platform.
- If there exists a //only-<> and the platform does not matches, this should be skipped
- If there exists a //only-<> and the platform does match, this should not be skipped
For ignore, we don't care if any other //ignore-<> exists. Like platform 'bar' won't care of //ignore- but it should care if //only- exists.
There was a problem hiding this comment.
Ah, that makes sense. Thanks! Can we put that into a (short) comment in the code perhaps?
There was a problem hiding this comment.
Sure that will be helpful. I will add that. I am also not sure what should interactions between //only-<> and //ignore-<> should result into. There can be some interesting cases there.
Mark-Simulacrum
added
S-waiting-on-author
The function parse_cfg_prefix() is not really parsing. It's just checking whether the prefix is present or not. So the new function name as suggested by @Mark-Simulacrum is better.
|
LGTM, thanks! @bors r+ |
|
📌 Commit bd70f0f has been approved by |
kennytm
added
S-waiting-on-bors
implement "only-<platforms>" for test headers This patch implements "only-<platforms>" for tests headers using which one can specify just the platforms on which the test should run rather than listing all the platforms to ignore using "ignore-<platforms>". This fixes rust-lang#33581 and fixes rust-lang#47459.
implement "only-<platforms>" for test headers This patch implements "only-<platforms>" for tests headers using which one can specify just the platforms on which the test should run rather than listing all the platforms to ignore using "ignore-<platforms>". This fixes rust-lang#33581 and fixes rust-lang#47459.
Document only-X test header This was added in rust-lang#47487 without documentation.
Document only-X test header This was added in rust-lang#47487 without documentation.
Document only-X test header This was added in rust-lang#47487 without documentation.
This patch implements "only-" for tests headers using which one can
specify just the platforms on which the test should run rather than listing all
the platforms to ignore using "ignore-".
This fixes #33581 and fixes #47459.