Introduce Custom Test Frameworks #53410
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
djrenren
force-pushed
the
custom-test-frameworks
branch
2 times, most recently
from
2c23431
to
1eae7e9
Compare
This comment has been minimized.
This comment has been minimized.
djrenren
force-pushed
the
custom-test-frameworks
branch
from
1eae7e9
to
f85330c
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Aha! I think this may be a bug in the testing attributes there, this line may need to be renamed from |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
djrenren
force-pushed
the
custom-test-frameworks
branch
from
3745ca2
to
51ca027
Compare
djrenren
changed the title
djrenren
force-pushed
the
custom-test-frameworks
branch
from
4e2b175
to
b5914f4
Compare
|
@alexcrichton That was definitely the issue. Should be all fixed up! |
|
☔ The latest upstream changes (presumably #53471) made this pull request unmergeable. Please resolve the merge conflicts. |
10 tasks
bjorn3
reviewed
Aug 22, 2018
src/libsyntax_ext/Cargo.toml
Outdated
| rustc_target = { path = "../librustc_target" } | ||
| log = "0.4" |
There was a problem hiding this comment.
This is looking pretty awesome, thanks so much for all this! I'm pretty excited in eventually stabilizing low-level tools like #[test_case] to help custom test frameworks/runners execute.
Some overall comments I have are:
- Could documentation for these attributes be added to the unstable book? This'll help keep track of their current implementation as well as their intended directions.
- One risk I see here is that if you accidentally mix
#[test]with a custom test runner that isn't expecting the data structures in thetestcrate it'll give some wonky errors, right? Or if not, could a UI test be added for this to see what it looks like? - I think this'll definitely solve
wasm-bindgen's use case in the sense that wasm-bindgen largely just needs the ability to aggregate tests, and I think we could work with this. Definitely something I'd like to make sure to game out before stabilization, but we've got quite some time to do that! - This is touching a very heavily used aspect of rustc, so we should definitely have a crater run before landing. If you want to rebase I'll kick off the try run so we can get that going!
- Could some more tests be added perhaps exercising different sorts of test frameworks, mixing them together, expanding to different data structures, etc? It'd also be great to have a "pretty complete" example which involves usage of a procedural macro as well to help ensure that all the plumbing is hooked up
src/librustc_resolve/macros.rs
Outdated
| let path = { | ||
| let attr = attr.as_ref().unwrap(); | ||
|
|
||
| // A hack to make #[test] not shadowable. This is in line with legacy behavior. |
There was a problem hiding this comment.
This check seems a bit suspect to me in the sense that we're in general going towards a world where it's fine to shadow built-in attributes (for the most part). Instead of this though I'd prefer to make it an error to define or import a macro called test rather than forcing resolution always go to the macro prelude.
Could you elaborate a bit though on how this came up?
There was a problem hiding this comment.
Sure thing, there's this phenomenon where people have defined macro_rules macros called test! that they use as some part of generating tests. For example:
rust/src/libsyntax_pos/analyze_source_file.rs
Lines 288 to 297 in c24f27c
We see this in many places even just within the compiler but a cursory github search also reveals there are many.
No one would be happier than me to remove this hack, but I figured it best to start conservative and remove later if we can.
src/libsyntax/test.rs
Outdated
| self.tests.push(item.ident); | ||
|
|
||
| // Ensure this item is capable of being reexported | ||
| item.vis = respan(item.vis.span, ast::VisibilityKind::Public); |
There was a problem hiding this comment.
This seems slightly suspect to me in the sense that it messes with the module system in a user-visible fashion, but I haven't finished reading this PR yet so I'll keep going!
There was a problem hiding this comment.
Ah this is actually a mistake. This is already handled in expand.rs where we both gensym and make public so it's not user-visible. This line essentially a no-op currently.
| @@ -907,7 +907,8 @@ mod tests { | |||
|
|
|||
| #[cfg(test)] | |||
| mod bench { | |||
| use Bencher; | |||
| extern crate test; | |||
| use self::test::Bencher; | |||
There was a problem hiding this comment.
This seems a little odd, but this is the whole "how does test test itself" problem, right? If so, could these tests be moved to an external location like src/libtest/benches/stats.rs?
There was a problem hiding this comment.
Yeah so now libtest is being tested by the compiler's libtest.
The first two are actual benchmarks of the stats sum method, but I think all 3 are really just smoke tests and may make most sense as a compiletest.
djrenren
force-pushed
the
custom-test-frameworks
branch
from
b5914f4
to
44db252
Compare
|
Just pushed a rebase, so we can get the crater run going. (also removed the redundant publicizing). I'll work on moving the libtest tests and producing more tests next. |
|
@bors: try I'll take a bit closer look responses soon |
bors
added a commit
that referenced
this pull request
Aug 22, 2018
Introduce Custom Test Frameworks Introduces `#[test_case]` and `#[test_runner]` and re-implements `#[test]` and `#[bench]` in terms of them. Details found here: https://blog.jrenner.net/rust/testing/2018/08/06/custom-test-framework-prop.html
There was a problem hiding this comment.
So just to make sure I understand everything that's going on here as well, there's currently logic which uses a "clever series" of modules to make all the #[test]-annotated functions available at the root of the crate. We then generate an entry point which takes all these functions, assembles some structures, and then passes a list of them to the test crate.
After this change that is somewhat different. Instead all #[test_case] annotated items are reexported to the root of the crate verbatim. Then a mutable reference to all #[test_case] items is passed to the "test runner", which takes over. The #[test] attribute generates const values which are the struct that libtest expects.
Does that sound like what's going on?
src/librustc_resolve/macros.rs
Outdated
| @@ -475,6 +475,10 @@ impl<'a, 'cl> Resolver<'a, 'cl> { | |||
| return def; | |||
| } | |||
|
|
|||
| if kind == MacroKind::Attr && *&path[0].as_str() == "test" { | |||
There was a problem hiding this comment.
cc @petrochenkov, do you know how this might be best implemented?
The gist of the change here is that #[test] is becoming a procedural macro attribute, but this is in practice conflicting with a lot of macro_rules! macros called test. I believe the intention here is that test as an attribute is always resolved to the macro attribute, but otherwise it goes through normal resolution rules.
@djrenren I think what we probably want, long term, here is to have a forward-compatibility warning issued. Crates which require this overriding behavior of "#[test] trumps test!" will want to get a warning that eventually we'd like to remove this hack in the future, and the macro should be renamed.
src/libsyntax/test.rs
Outdated
| vec![field("desc", desc_expr), | ||
| field("testfn", testfn_expr)]) | ||
| fn get_test_runner(sd: &errors::Handler, krate: &ast::Crate) -> Option<ast::Path> { | ||
| if let Some(test_attr) = attr::find_by_name(&krate.attrs, "test_runner") { |
There was a problem hiding this comment.
Could this use ? and early returns on invalid cases to reduce the indentation a bit?
There was a problem hiding this comment.
Ah yeah totally could (and will)
|
☀️ Test successful - status-travis |
Yeah that's what's going on. Though further reflection and talking to @nrc, makes me think we might want to use immutable references at first and see if we need mutability |
|
@craterbot ping |
|
🔒 Error: you're not allowed to interact with this bot. 🔑 If you are a member of the Rust team and need access, add yourself to the whitelist. |
|
@rust-lang/infra mind scheduling a crater run for this PR? |
|
@bors: r+ delegate+ |
|
✌️ @djrenren can now approve this pull request |
|
📌 Commit 0593dc7 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Sep 5, 2018
Introduce Custom Test Frameworks Introduces `#[test_case]` and `#[test_runner]` and re-implements `#[test]` and `#[bench]` in terms of them. Details found here: https://blog.jrenner.net/rust/testing/2018/08/06/custom-test-framework-prop.html
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Sep 5, 2018
bors
added a commit
that referenced
this pull request
Sep 10, 2018
resolve: Split macro prelude into built-in and user-defined parts This is a refactoring that will help to remove `unshadowable_attrs` when #53410 lands. UPDATE: The second commit actually removes `unshadowable_attrs`.
bors
added a commit
that referenced
this pull request
Sep 14, 2018
resolve: Introduce two sub-namespaces in macro namespace
Two sub-namespaces are introduced in the macro namespace - one for bang macros and one for attribute-like macros (attributes, derives).
"Sub-namespace" means this is not a newly introduced full namespace, the single macro namespace is still in place.
I.e. you still can't define/import two macros with the same name in a single module, `use` imports still import only one name in macro namespace (from any sub-namespace) and not possibly two.
However, when we are searching for a name used in a `!` macro call context (`my_macro!()`) we skip attribute names in scope, and when we are searching for a name used in attribute context (`#[my_macro]`/`#[derive(my_macro)]`) we are skipping bang macro names in scope.
In other words, bang macros cannot shadow attribute macros and vice versa.
For a non-macro analogy, we could e.g. skip non-traits when searching for `MyTrait` in `impl MyTrait for Type { ... }`.
However we do not do it in non-macro namespaces because we don't have practical issues with e.g. non-traits shadowing traits with the same name, but with macros we do, especially after macro modularization.
For `#[test]` and `#[bench]` we have a hack in the compiler right now preventing their shadowing by `macro_rules! test` and similar things. This hack was introduced after making `#[test]`/`#[bench]` built-in macros instead of built-in attributes (#53410), something that needed to be done from the start since they are "active" attributes transforming their inputs.
Now they are passed through normal name resolution and can be shadowed, but that's a breaking change, so we have a special hack basically applying this PR for `#[test]` and `#[bench]` only.
Soon all potentially built-in attributes will be passed through normal name resolution (#53913) and that uncovers even more cases where the strict "macro namespace is a single namespace" rule needs to be broken.
For example, with strict rules, built-in macro `cfg!(...)` would shadow built-in attribute `#[cfg]` (they are different things), standard library macro `thread_local!(...)` would shadow built-in attribute `#[thread_local]` - both of these cases are covered by special hacks in #53913 as well.
Crater run uncovered more cases of attributes being shadowed by user-defined macros (`warn`, `doc`, `main`, even `deprecated`), we cannot add exceptions in the compiler for all of them.
Regressions with user-defined attributes like #53583 and #53898 also appeared after enabling macro modularization.
People are also usually confused (#53205 (comment), #53583 (comment)) when they see conflicts between attributes and non-attribute macros for the first time.
So my proposed solution is to solve this issue by introducing two sub-namespaces and thus skipping resolutions of the wrong kind and preventing more error-causing cases of shadowing.
Fixes #53583
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 19, 2021
…ou-se Expose `Concurrent` (private type in public i'face) rust-lang#53410 introduced experimental support for custom test frameworks. Such frameworks may wish to build upon `library/test` by calling into its publicly exposed API (which I entirely understand is wholly unstable). However, any that wish to call `test::run_test` cannot currently do so because `test::options::Concurrent` (the type of its `concurrent` parameter) is not publicly exposed.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 19, 2021
…ou-se Expose `Concurrent` (private type in public i'face) rust-lang#53410 introduced experimental support for custom test frameworks. Such frameworks may wish to build upon `library/test` by calling into its publicly exposed API (which I entirely understand is wholly unstable). However, any that wish to call `test::run_test` cannot currently do so because `test::options::Concurrent` (the type of its `concurrent` parameter) is not publicly exposed.
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
May 19, 2021
…ou-se Expose `Concurrent` (private type in public i'face) rust-lang#53410 introduced experimental support for custom test frameworks. Such frameworks may wish to build upon `library/test` by calling into its publicly exposed API (which I entirely understand is wholly unstable). However, any that wish to call `test::run_test` cannot currently do so because `test::options::Concurrent` (the type of its `concurrent` parameter) is not publicly exposed.
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces
#[test_case]and#[test_runner]and re-implements#[test]and#[bench]in terms of them.Details found here: https://blog.jrenner.net/rust/testing/2018/08/06/custom-test-framework-prop.html