-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Termination trait in tests #48143
Termination trait in tests #48143
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
src/libsyntax/test.rs
Outdated
ecx.lambda( | ||
span, | ||
vec![], | ||
// ::std::Termination::assert_unit_test_successful(..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this function be in libtest
? I do think I like the closure though - it gets coerced into a fn
pointer, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more about what you think I should do here? You want me to generate a closure that invokes a fn in libtest
that invokes std::Termination::assert_unit_test_successful
? (Or I guess potentially remove assert_unit_test_successful
? I'd rather not do the latter, but then it is an ad-hoc extension to the spec. It's one that makes a lot of sense to me though, since I imagine that people may want to write custom result types that print out differently in unit test failures than otherwise. But I guess there is no strong reason that said people shouldn't use a different type in their unit tests.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a trait in libtest
that can be specialized if need be in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so roughly what there is no, but indirecting through some random trait instead, and that trait is implemented for all T: Termination
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good default, yes. I'd really want to keep testing infrastructure out of libstd
where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb I guess it'd be mildly easier (and equally future proof) to add a top-level function to libtest, like assert_unit_test_success(foo())
. Internally, it can use Termination
for now -- we can just use it indiscriminately actually, since Termination
is defined for ()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ok, I'll add something like that.
src/libsyntax/lib.rs
Outdated
@@ -105,6 +105,7 @@ pub mod syntax { | |||
pub use ext; | |||
pub use parse; | |||
pub use ast; | |||
pub use tokenstream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is not required anymore, as you don't use the quote!
anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need to remove this?
cc @Manishearth and custom test frameworks just in case. |
Hmm, this is another example of something that needs compile-time expansion tweaking to work. |
src/libsyntax/test.rs
Outdated
// like this: | ||
// | ||
// fn wrapper() { | ||
// assert_eq!(0, real_function().report()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure you mean Termination::assert_unit_test_successful(real_function())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that comment is dated I guess
Reviewed. Everything looks reasonable, modulo things already mentioned by other people. |
@eddyb so I tried to do what you suggested (see most recent commit) but now I get these errors from libtest:
This despite the fact that we have the feature gate enabled. I'm not really sure what's going on there... |
OK, so, I'm inclined to revert back to my working version and file a FIXME to extract this logic into libtest. I don't really feel like taking the time to root out this problem, and the amount of logic in question here is quite small. @eddyb @petrochenkov -- any objection? |
Extend `Termination` trait with a method to determine what happens with a unit test. This commit incorporates work by Bastian Köcher <git@kchr.de>.
0ceff7b
to
2029084
Compare
r? @eddyb |
Also make `std::termination` module public and rename feature. The lib feature needs a different name from the language feature.
2029084
to
5f1e78f
Compare
src/rfc-1937-termination-trait/termination-trait-for-result-box-error_ok.rs
@bors r+ |
📌 Commit 0a5f4ae has been approved by |
@bors r- The test
|
@bors r=eddyb |
📌 Commit 10f7c11 has been approved by |
…ests, r=eddyb Termination trait in tests Support the `Termination` trait in unit tests (cc rust-lang#43301) Also, a drive-by fix for rust-lang#47075. This is joint work with @bkchr.
…ests, r=eddyb Termination trait in tests Support the `Termination` trait in unit tests (cc rust-lang#43301) Also, a drive-by fix for rust-lang#47075. This is joint work with @bkchr.
Support the
Termination
trait in unit tests (cc #43301)Also, a drive-by fix for #47075.
This is joint work with @bkchr.