-
Notifications
You must be signed in to change notification settings - Fork 1
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
Unwrapped value for assertions on Result
#8
Comments
Thanks for messaging. Let me see if I understand what you're asking... first allow me to demonstrate three kinds, so I can reference them below. Suppose you want to verify the inner is 123.
assert_ok!(result);
assert_ok_eq!(result, Ok(123));
assert_ok_eq_expr!(result, 123); What I think you're asking is for something new, where the Ok inner value is perhaps a structure, maybe something like this example? struct MyStruct { a: i8, b: i8 }
pub fn foo() -> Result<MyStruct, String> {…}
let result = foo(); And you're wanting to test an inner property, such as Currently the assertables crate can do this: assert_ok!(result);
let inner = result.unwrap();
assert_eq!(inner.a, 123); What I think you're asking is for something that returns the inner value, perhaps more like this? let inner = assert_ok!(result);
assert_eq!(inner.a, 123); How close am I to what you're asking? |
This is exactly what I meant! I think that not having to explicitly unwrap the pure value is a good improvement for tests readability, considering also that in the current macro implementation, unwrapping is actually done by the |
Good. What you're asking would presumably have ripple effects throughout the crate, yes? For example, continuing your idea, a macro such as It would be a big change to the code and to the docs, and it's definitely different than what other Rust assert macros do, and would presumably need new macro forms because many IDEs warn if code returns output that's not used or soaked up with an underscore. So here's what I suggest, and see what you think of this. I suggest this issue here is a place that you and I can gather some experienced developers' opinions about this, and if/how it could work well. Do you think you can gather a few experienced Rust developers to take a look? |
In my opinion, there is no need to change a lot of macros. I think that only the macros explicitly asserting on a monad could be changed, for example
I'd really like to, but unfortunately none of the programmers I know uses Rust, yet... |
So you're proposing upgrading these macros and their corresponding modules, yes? let (inner1, inner2) = assert_ok_eq!(result1, result2);
let (inner1, inner2) = assert_some_eq!(option1, option2);
let (inner2, inner2) = assert_ready_eq!(poll1, poll2); And you're proposing not upgrading these macros and their corresponding modules, yes? let (value1, value2) = assert_infix(expr1, expr2);
let (string1, string2) = assert_fs_read_to_string_eq!(path1, path2);
let (string1, string2) = assert_io_read_to_string_eq!(reader1, reader2);
let (stdout1, stdout2) = assert_command_stdout_eq!(command1, command2);
let (stdout1, stdout2) = assert_program_args_stdout_eq!(program1, args1, program2, args2); The macros What you're asking is a tricky use case IMHO because the first group of macros above currently have a quick easy to get at their inner values by using unwrap(), whereas the second group of macros above currently don't have any way to get at their inner values. As one example, if a developer uses an
How about doing some outreach? My opinion is you've got a really good idea, and it's worth pursuing smarter people to get advice. My guess is that the people wouldn't even need to be Rust developers, because my guess is some other programming language has tried some ways to solve these kinds of concepts you're aiming for, by doing assert-with-return or something like that. Want to post to Hacker News, or reddit/r/rust, or lobste.rs, etc.? |
No, I would left all the Here's a more detailed explanations of what I'm proposingGiven this production code: struct Person {
first_name: String,
last_name: String,
age: u32
}
fn get_person() -> Result<Person, String> {
todo!()
}
I propose to change the macro #[test]
fn verify_person() {
let result = get_person()
let inner = assert_ok!(result);
# here inner is a (): I can't anything with that
} could be rewritten to #[test]
fn verify_person() {
let result = get_person()
let person = assert_ok!(result);
# here person is a Person: I can add additional assertions on this value
assert_eq!(42, person.age);
} and I would like to repeat the same idea to all the other assertions that simply verify the subtype of a monad: assert_ok!()
assert_err!()
assert_some!()
assert_none!()
...plus other similar macros I'm not aware of... while leaving the other more specific macros unmodified. Why?Instead of the If you have to use the If you could instead leverage on the |
Really good explanation and example. When you look at the bigger picture, what do you recommend for these areas?
let _ = assert_ok!(foo); Because the behavior would become different than all other typical Rust asserts, including in similar crates such as Given your example, my intuition is that your idea is much bigger i.e. what can become viable to help developers, when assert macros in general change from just doing You might want to take a look at For example, compare two JSON structs for eq, and return the keys that are different, or the keys that are the same yet have different values. This kind of feature is akin to returning a structured diff, and is the #1 request from developers who talk with me about the For another example, return the inner value and let it be chainable, such as: assert_ok!(person).assert_attr_eq!(age, 42); |
No, it wouldn't, because the macro is currently returning
That's not needed: developers could just invoke the macro without capturing the received value and it would be simply discarded, there is no need to explicitly save it into an unnamed variable.
Actually, my idea is much more simple: just return the pure value unwrapped from a monad, so that I can subsequently assert on it safely from a typing point of view. As for other assertions, I don't have any specific idea, it's up to you if you want to explore more advanced features. |
Thanks, I believe I understand with what you're asking. My perspective is to ideally get opinions from at least a few more developers and/or experts, and ideally compare some similar kinds of crates, to see if/how these crates handle it. Here's what's on my radar, if you want to take a look to see if/how these implement a monad return, or suggest others: https://github.com/SixArm/assertables-rust-crate/tree/main/help/comparisons And in the meantime, you're able to do this code below, which is guaranteed safe because you're checking Ok: assert_ok!(result);
let person = result.unwrap(); // guaranteed safe, because you've already checked Ok
assert_eq!(42, person.age); Or if you prefer a message: assert_ok!(result);
let person = result.expect("person");
assert_eq!(person.age, 42); Or if you prefer if...let syntax then something like this: assert_ok!(result);
if let Ok(person) = result {
assert_eq!(person.age, 42);
} |
Another workaround to leverage on the #[macro_export]
macro_rules! assert_ok {
( $x:expr ) => {{
assertables::assert_ok!($x);
$x.unwrap()
}};
}
#[macro_export]
macro_rules! assert_err {
( $x:expr ) => {{
assertables::assert_err!($x);
$x.unwrap_err()
}};
} But I would strongly prefer to not have to copy-paste this trick in all my projects 😅 |
Good info today on Hacker News about this topic. https://news.ycombinator.com/item?id=41856815 I intend to build the features you want for the next release. Estimate 2-4 weeks. |
Ready for you to try now, using Assertables version 9. Heads up there are a lot of changes under the covers. Read the upgrade notes here: https://github.com/SixArm/assertables-rust-crate/tree/main/help/upgrades/upgrade-version-8-to-9 Can you let me know if this works well for you? And also any other findings, suggestions, etc.? Thanks! |
The last changes published in version 9.x made a progress, but now I found another issue. I can now write the assertions without an explicit - assert_eq!(my_expected, my_actual);
+ assert_eq!(&my_expected, my_actual); Not a big issue, indeed, but the access by reference is not really required and, most importantly, applying this change is a tedious work, especially if you have a lot of tests to fix. I discovered that the reason for this is the implementation of the This is a possible implementation for the #[macro_export]
macro_rules! assert_ok_as_result {
($a:expr $(,)?) => {{
match ($a) {
Ok(any) => {
Ok(any)
},
_ => {
Err(format!(
concat!(
"assertion failed: `assert_ok!(a)`\n",
"https://docs.rs/assertables/", env!("CARGO_PKG_VERSION"), "/assertables/macro.assert_ok.html\n",
" a label: `{}`,\n",
" a debug: `{:?}`",
),
stringify!($a),
$a
))
}
}
}};
} If you compare it with the current implementation, you would see that only a single In my opinion, this is a fix that should be applied, because in an assertion we should not deal with borrowing and other technical details, but just highlight the business requirement. About the other changes in the new releaseMy personal opinion on function naming: using a number (in this case it is
Now we have instead:
This is not self-explaining and we need to read the documentation to understand what is the actual difference between these two macros. I hope that you'll find my opinion useful. |
Great feedback, thank you. Yes your suggestion makes sense given the new returns. The original purpose of that first match is to cause the eval of the expression, so it only needs to be done once; your way makes more sense now. I'll work on that. E.t.a. 48 hours for an update. Yes naming is hard. :-) I'm open to ideas. There are 3 areas that need names.
I'll read some similar crates to see if they've come up with solutions. In general, the consistent feedback from developers is "we loathe long names" and "we want to compare an item to an expression, not two items to each other".
Yes very much appreciated. Keep it up! <3 |
Version 9.1.0 is ready for you to try, and I yanked version 9.0.0 so it won't confuse more people.
Now named Example version 9.1.0: assert_ok_eq!(Ok(1), Ok(1)); // same as version 8
assert_ok_eq_x!(Ok(1), 1); // same as version 8 assert_ok_eq_expr Next up is fixing the reference issues that you found. E.t.a. 48 hours. |
I would like to propose an improvement on the assertions proposed in this library.
Scenario:
Result
Ok
With the current implementation, I can use
assert_ok_eq!()
, but this requires me to build a full expected object and this adds some "noise" to the test definition, because I need to explicitate some data that is not actually needed.I would like to write something like this, instead:
Basically, is would be required for
assert_ok!()
to return the value wrapped into theOk
structure instead of just returning()
, so that we can subsequently perform additional assertions on that value.The same feature would be useful for
Err
and its related assertions.What do you think about it?
The text was updated successfully, but these errors were encountered: