Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upIntroduce Custom Test Frameworks #53410
Conversation
rust-highfive
assigned
alexcrichton
Aug 15, 2018
This comment has been minimized.
This comment has been minimized.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
rust-highfive
added
the
S-waiting-on-review
label
Aug 15, 2018
djrenren
force-pushed the
djrenren:custom-test-frameworks
branch
2 times, most recently
from
2c23431
to
1eae7e9
Aug 15, 2018
This comment was marked as outdated.
This comment was marked as outdated.
|
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
djrenren:custom-test-frameworks
branch
from
1eae7e9
to
f85330c
Aug 15, 2018
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
Aha! I think this may be a bug in the testing attributes there, this line may need to be renamed from |
This comment has been minimized.
This comment has been minimized.
|
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
djrenren:custom-test-frameworks
branch
from
3745ca2
to
51ca027
Aug 17, 2018
djrenren
changed the title
WIP: Introduce Custom Test Frameworks
Introduce Custom Test Frameworks
Aug 18, 2018
djrenren
force-pushed the
djrenren:custom-test-frameworks
branch
from
4e2b175
to
b5914f4
Aug 20, 2018
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton That was definitely the issue. Should be all fixed up! |
This comment has been minimized.
This comment has been minimized.
|
|
djrenren
referenced this pull request
Aug 21, 2018
Open
Tracking issue for eRFC 2318, Custom test frameworks #50297
bjorn3
reviewed
Aug 22, 2018
src/libsyntax_ext/Cargo.toml
Outdated
| rustc_target = { path = "../librustc_target" } | ||
| log = "0.4" |
This comment has been minimized.
This comment has been minimized.
alexcrichton
reviewed
Aug 22, 2018
|
This is looking pretty awesome, thanks so much for all this! I'm pretty excited in eventually stabilizing low-level tools like Some overall comments I have are:
|
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. |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 22, 2018
Member
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?
This comment has been minimized.
This comment has been minimized.
djrenren
Aug 22, 2018
Author
Contributor
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); |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 22, 2018
Member
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!
This comment has been minimized.
This comment has been minimized.
djrenren
Aug 22, 2018
Author
Contributor
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; | |||
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 22, 2018
Member
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?
This comment has been minimized.
This comment has been minimized.
djrenren
Aug 22, 2018
Author
Contributor
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
djrenren:custom-test-frameworks
branch
from
b5914f4
to
44db252
Aug 22, 2018
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
@bors: try I'll take a bit closer look responses soon |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Aug 22, 2018
alexcrichton
reviewed
Aug 22, 2018
|
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 After this change that is somewhat different. Instead all 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" { | |||
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 22, 2018
Member
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.
This comment has been minimized.
This comment has been minimized.
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") { |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Aug 22, 2018
Member
Could this use ? and early returns on invalid cases to reduce the indentation a bit?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
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 |
This comment has been minimized.
This comment has been minimized.
|
@craterbot ping |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@rust-lang/infra mind scheduling a crater run for this PR? |
bors
added
the
S-waiting-on-bors
label
Sep 4, 2018
This comment has been minimized.
This comment has been minimized.
|
This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
bors
added
S-waiting-on-author
and removed
S-waiting-on-bors
labels
Sep 5, 2018
djrenren
added some commits
Jul 21, 2018
djrenren
force-pushed the
djrenren:custom-test-frameworks
branch
from
56986a9
to
0593dc7
Sep 5, 2018
This comment has been minimized.
This comment has been minimized.
|
Got a merge conflict on |
This comment has been minimized.
This comment has been minimized.
|
@bors: r+ delegate+ |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
|
bors
added
S-waiting-on-bors
and removed
S-waiting-on-author
labels
Sep 5, 2018
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Sep 5, 2018
This comment has been minimized.
This comment has been minimized.
|
|
bors
merged commit 0593dc7
into
rust-lang:master
Sep 5, 2018
This was referenced Sep 5, 2018
petrochenkov
referenced this pull request
Sep 5, 2018
Merged
resolve: Future proof resolutions for potentially built-in attributes #53913
petrochenkov
referenced this pull request
Sep 8, 2018
Merged
resolve: Introduce two sub-namespaces in macro namespace #54069
bors
added a commit
that referenced
this pull request
Sep 10, 2018
bors
added a commit
that referenced
this pull request
Sep 14, 2018
petrochenkov
referenced this pull request
Sep 22, 2018
Closed
[1.30 beta] Macros recursion limit detection changed #54464
pietroalbini
referenced this pull request
Sep 22, 2018
Closed
[1.30 beta] tester crate broken #54466
apoelstra
referenced this pull request
Oct 15, 2018
Closed
`cargo miri test` no longer correctly detects tests #479
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.
djrenren commentedAug 15, 2018
•
edited
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