Comments (19)
I just ran into this on accident, I wasn't expecting test_case
to not function like the Rust standard #[test]
in this situation.
from test-case.
I recommend forking the repository. It is the essence of open source, after all.
You don't have to worry that the issue has been open for so long by forking the repository.
Of course, it requires an effort to patch the bug.
And while we made this library open-source and available for everyone, we (usually) are still driven by what we need in our personal and professional projects.
From my experience, I didn't fix this bug simply because it had no severity.
I rarely replaced #[test]
with #[test_case]
, and when I did, I used unwrap()
instead of the Result
.
So while we value the input from other users, we have a limited amount of time for OSS projects.
That's why I didn't make any promises about this fix. If someone else thinks this is a critical issue, I'm more than happy to guide how to fix it and review and merge the PR.
from test-case.
1.0.x support is old branch with some incompatible changes. @frondeus we probably should create similar branch for 1.2.1 from 1.2.1 tag since master is already at 2.0.0-rc.
I guess we need to come up with some proper branching strat.
from test-case.
Hmm, we definitely need to look into that. For now you can work around it with #[test_case(5 => matches Ok(_); "Failing")]
.
Most of tests that I wrote for such scenarios included explicit unwrap without returning Result from test function, but being compatible with rust test is probably way to go.
from test-case.
This really should be tracked as a Bug not an Enhancement: moving from #[test]
-> #[test_case(...)]
shouldn't break the existing test.
This has lead to a large number of tests passing when they should have failed for us. We're now looking for an alternative to test-case
because with this issue being open for this length of time, with no progress we just don't trust using this crate any more.
from test-case.
Yeah, you have a point that it should be a bug. Unfortunately I don't know when I'll have time to implement a fix. Maintaining this lib almost alone with work and studies in parallel ain't easiest thing to do & I don't see many people lining up trying to work on open issues.
from test-case.
I appreciate the responses on this. And yes, I do understand the nature of OSS and that everyone's time is limited for these sorts of things 🙂 Also I'd like to apologise for my original comment being as confrontational as it is: hitting this bug added to an already stressful day and I shouldn't have taken that out here.
As some additional context for why I'm finding this behaviour surprising: my main issue with this particular bug is that test-case
is changing the semantics of returning results from tests as documented in the Rust book. This is not made clear in the README anywhere as far as I can tell, when an API change like that really should be called out quite clearly (please do correct me if I'm wrong on that!).
Given that all we want/need is parameterised tests, I've done as you suggest and written a minimal crate that just handles that side of things.
from test-case.
One short-term option might be to cause #[test_case]
compilation to abort with an error if a return-type exists, which will at least remove the undetected failure problem. The following change to test-case
seems to do what we need:
diff --git a/src/lib.rs b/src/lib.rs
index 8f38b09..2b30a87 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -231,7 +231,7 @@ extern crate proc_macro;
use proc_macro::TokenStream;
-use syn::{parse_macro_input, ItemFn};
+use syn::{parse_macro_input, ItemFn, ReturnType};
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
@@ -310,6 +310,18 @@ pub fn test_case(args: TokenStream, input: TokenStream) -> TokenStream {
let test_case = parse_macro_input!(args as TestCase);
let mut item = parse_macro_input!(input as ItemFn);
+ match item.sig.output {
+ ReturnType::Type(_, ret) => {
+ return syn::Error::new(
+ ret.span(),
+ format!("Test function {} has a return-type. This is currently unsupported.", item.sig.ident),
+ )
+ .to_compile_error()
+ .into();
+ },
+ _ => {}
+ }
+
let mut test_cases = vec![test_case];
let mut attrs_to_remove = vec![];
for (idx, attr) in item.attrs.iter().enumerate() {
This causes this error for a #[test_case]
attribute on a function with any return type:
error: Test function equals_5 has a return-type. This is currently unsupported.
--> src/lib.rs:8:36
|
8 | fn equals_5(a: i32, b: i32) -> Result<(), String> {
| ^^^^^^
error: could not compile `test-case-test` due to previous error
Thoughts on this? If this approach seems OK I could put together a PR.
from test-case.
That unfortunately would be a major breaking change.
In test case an user can write a test_case like:
#[test_case(1, 1 => 2)]
fn adding(a: usize, b: usize) -> usize {
a + b
}
As you can see the return type is supported in that case and it is asserted with assert_eq!(func_result, 2)
.
However, I think we could do such a detection with extra condition:
"If return-type is present AND the =>
syntax is not used, then return the error".
from test-case.
Because it is a bug, we would have to patch it in the 1.x branch.
I think the best place to check the return-type
would be https://github.com/frondeus/test-case/blob/1.0.x-support/src/test_case.rs#L85-L95
from test-case.
Thanks @frondeus. Here's an alternate version targeting the 1.0.x-support
branch below. I'm not sure about the expected types test, input welcome.
diff --git a/src/lib.rs b/src/lib.rs
index 26b5210..48e3857 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -217,7 +217,7 @@ extern crate proc_macro;
use proc_macro::TokenStream;
-use syn::{parse_macro_input, ItemFn};
+use syn::{parse_macro_input, ItemFn, ReturnType};
use quote::quote;
use syn::parse_quote;
@@ -305,6 +305,10 @@ pub fn test_case(args: TokenStream, input: TokenStream) -> TokenStream {
.into()
}
};
+ if let Err(err) = return_test(&test_case, &item) {
+ return err;
+ }
+
test_cases.push(test_case);
attrs_to_remove.push(idx);
}
@@ -317,6 +321,21 @@ pub fn test_case(args: TokenStream, input: TokenStream) -> TokenStream {
render_test_cases(&test_cases, item)
}
+fn return_test(test_case: &TestCase, item: &ItemFn) -> Result<(), TokenStream> {
+ let fn_ret = &item.sig.output;
+ match fn_ret {
+ ReturnType::Type(_, ret_type) if !test_case.expects_return() => {
+ Err(syn::Error::new(
+ ret_type.span(),
+ format!("Test function {} has a return-type but no exected clause in the test-case. This is currently unsupported.", item.sig.ident),
+ )
+ .to_compile_error()
+ .into())
+ },
+ _ => Ok(())
+ }
+}
+
fn render_test_cases(test_cases: &[TestCase], item: ItemFn) -> TokenStream {
let mut rendered_test_cases = vec![];
diff --git a/src/test_case.rs b/src/test_case.rs
index 5a39f58..3c6968e 100644
--- a/src/test_case.rs
+++ b/src/test_case.rs
@@ -121,6 +121,16 @@ impl TestCase {
crate::utils::escape_test_name(case_desc)
}
+ pub fn expects_return(&self) -> bool {
+ match self.expected {
+ Some(Expected::Pat(_)) => true,
+ Some(Expected::Panic(_)) => false,
+ Some(Expected::Ignored(_)) => false,
+ Some(Expected::Expr(_)) => true,
+ None => false,
+ }
+ }
+
pub fn render(&self, mut item: ItemFn) -> TokenStream2 {
let item_name = item.sig.ident.clone();
let arg_values = self.args.iter();
Now given these test cases:
#[cfg(test)]
mod tests {
use test_case::test_case;
#[test_case(2, 2; "Wrong")]
#[test_case(2, 3; "Right")]
fn return_no_expect(a: i32, b: i32) -> Result<i32, String> {
let result = a + b;
return if result == 5 {
Ok(result)
} else {
Err("Not equal 5".to_owned())
}
}
#[test_case(2, 2 => Err("Not equal 5".to_owned()); "Wrong")]
#[test_case(2, 3 => Ok(5); "Right")]
fn return_with_expect(a: i32, b: i32) -> Result<i32, String> {
let result = a + b;
return if result == 5 {
Ok(result)
} else {
Err("Not equal 5".to_owned())
}
}
}
You get this:
error: Test function return_no_expect has a return-type but no exected clause in the test-case. This is currently unsupported.
--> src/lib.rs:8:44
|
8 | fn return_no_expect(a: i32, b: i32) -> Result<i32, String> {
| ^^^^^^
error: could not compile `test-case-test` due to previous error
from test-case.
That looks like an awesome fix. Thank you for your contribution :)!
Could you create a PR?
from test-case.
No worries, I'll put it together of the next few days.
from test-case.
@frondeus There doesn't actually appear to be a v1 support branch to target; the 1.0.x-support
branch is behind the head of the v.1.2.1
tag. Where should I target the PR?
from test-case.
I will prepare proper target branch today
from test-case.
Ok, had some free time, I created branch 1.2.x-support
. You can target it. I'll take care of porting the change to master when I'll return to working on it.
I see now that it'd be better if I worked on 2.0 release on some different branch than master. Well, will remember to do so in the future.
from test-case.
As @bspradling noticed this code:
#[test_case("ISBN:1839485743"; "isbn10")]
#[test_case("ISBN:1839485743123"; "isbn13")]
#[tokio::test]
async fn test_bibliography_key_serde(expected: &str) -> Result<(), Box<dyn Error>> {
let key: BibliographyKey = serde_json::from_str(expected)?;
let actual = serde_json::to_string(&key)?;
assert_eq!(expected, actual);
Ok(())
}
cannot be simply fixed by adding => Ok(())
.
Because Err
in this case is Box<dyn Error>
which does not implement PartialEq
.
In that case the author doesn't want to check the error message, he just want to make sure the function does not return any errors at all.
But Rust doesn't know that.
The only solution I see for now is to do something like this:
#[test_case("ISBN:1839485743"; "isbn10")]
#[test_case("ISBN:1839485743123"; "isbn13")]
#[tokio::test]
async fn test_bibliography_key_serde(expected: &str) {
async fn inner(expected: &str) -> Result<(), Box<dyn Error>> {
let key: BibliographyKey = serde_json::from_str(expected)?;
let actual = serde_json::to_string(&key)?;
assert_eq!(expected, actual);
Ok(())
}
inner(expected).await.unwrap();
}
But it requires a lot of boilerplate and it's just... ugly.
Therefore I think this case proves we should increase the priority of this issue. It can be really inconvenient and the workaround is not easy as I wish it would be.
Also keep in mind that I believe matching the Err matching could be possible but the Rust does not support it either.
I mean you cannot do this:
#[test]
#[should_panic(expected = "123")]
fn my_test() -> Result<(), Box<dyn Error>> { ... }
but perhaps test_case
could handle it by simply... unwrapping the result and using should_panic
from test-case.
About the last paragraph:
I would imagine something like:
#[test_case(1, 2 => panics "fail")]
fn test_me(a: usize, b: usize) -> Result<usize, Box<dyn Error>> {
....
}
To work just fine.
from test-case.
Extra: #86 (comment)
from test-case.
Related Issues (20)
- Uppercase and lowercase generated test function collide HOT 4
- Does not fully work with `pretty_assertions`, fix OK? HOT 2
- feature request: test_case generators HOT 1
- Getting current test case description from within the test function HOT 4
- Expose `TestCase` struct publicly for reuse HOT 1
- please include LICENSE file in all published crates HOT 2
- Set test case function `Ident` spans to corresponding `test_case` spans HOT 2
- Is return value in test required? HOT 8
- On nightlies and betas, `cargo +nightly clippy` creates `items_after_test_module` warnings HOT 9
- Type as test case argument HOT 9
- When generating assert the right side should be the "correct" side. HOT 19
- New clippy lint in Rust 1.71.0, `items_after_test_module`, affecting integration tests HOT 3
- upgrade dependency proc-macro2 HOT 1
- error[E0432]: unresolved import `test_case_macros::test_matrix` using 3.2.0 HOT 3
- Test case macro should not rely on implementation details of the built-in test framework HOT 3
- `syn::Result::ok` silently discards errors
- macro for bench cases HOT 1
- Conditional flags for ignore and inconclusive HOT 3
- Iterable args: a way to expand a given argument of `test_matrix`
- Example usage of test_matrix broken HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from test-case.