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

Add custom message parameter to `assert_eq!` #33976

Merged
merged 1 commit into from Jun 22, 2016

Conversation

Projects
None yet
@komamitsu
Contributor

komamitsu commented May 31, 2016

assert! macro accepts a custom message parameter and it's sometimes useful. But assert_eq! doesn't have it and users need to use assert! instead of assert_eq! when they want to output a custom message even if the assertion just compares two values. This pull request will resolve those cases.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive May 31, 2016

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented May 31, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 31, 2016

Member

Nice! Squash your commits please.

Member

GuillaumeGomez commented May 31, 2016

Nice! Squash your commits please.

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu May 31, 2016

Contributor

@GuillaumeGomez Thanks. Squashed them.

Contributor

komamitsu commented May 31, 2016

@GuillaumeGomez Thanks. Squashed them.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 31, 2016

Member

It's good for me but I'd prefer to let someone from @rust-lang/core team to review it as well.

Member

GuillaumeGomez commented May 31, 2016

It's good for me but I'd prefer to let someone from @rust-lang/core team to review it as well.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 31, 2016

Member

Yes, @rust-lang/lang and @rust-lang/libs should probably sign off on something like this.

Member

steveklabnik commented May 31, 2016

Yes, @rust-lang/lang and @rust-lang/libs should probably sign off on something like this.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk May 31, 2016

Contributor

related rfc issue: rust-lang/rfcs#1604

Contributor

oli-obk commented May 31, 2016

related rfc issue: rust-lang/rfcs#1604

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu May 31, 2016

Contributor

Oops. I should've checked the issue...

Contributor

komamitsu commented May 31, 2016

Oops. I should've checked the issue...

@ollie27

This comment has been minimized.

Show comment
Hide comment
@ollie27

ollie27 May 31, 2016

Contributor

Is there any reason to use:

assert_eq!("b", xs[i], "xs[{}] should be \"b\" but {}", i, xs[i]);

rather than:

assert!("b" == xs[i], "xs[{}] should be \"b\" but {}", i, xs[i]);

?

Contributor

ollie27 commented May 31, 2016

Is there any reason to use:

assert_eq!("b", xs[i], "xs[{}] should be \"b\" but {}", i, xs[i]);

rather than:

assert!("b" == xs[i], "xs[{}] should be \"b\" but {}", i, xs[i]);

?

@brson brson added the T-libs label May 31, 2016

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson May 31, 2016

Contributor

@ollie27 Not that I can think of, since the reason assert_eq exists is to provide a better error message.

Contributor

brson commented May 31, 2016

@ollie27 Not that I can think of, since the reason assert_eq exists is to provide a better error message.

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu May 31, 2016

Contributor

@ollie27
This question sounds to be similar to "Is there any reason to use assert_eq!(a, b) rather than assert!(a == b)?"
Just kidding :)

I prefer assert_eq! to just assert! since assert_eq! seems more explicit to me than assert! when I see test codes.

In major unit test libraries (e.g. JUnit in Java, OUnit in OCaml and Test::Unit in Ruby) in other languages, there are assert_equals which support custom message. So, when I noticed that assert_eq! doesn't accept custom message while assert! supports it, I thought it's just lack of feature or something not design decision.

Contributor

komamitsu commented May 31, 2016

@ollie27
This question sounds to be similar to "Is there any reason to use assert_eq!(a, b) rather than assert!(a == b)?"
Just kidding :)

I prefer assert_eq! to just assert! since assert_eq! seems more explicit to me than assert! when I see test codes.

In major unit test libraries (e.g. JUnit in Java, OUnit in OCaml and Test::Unit in Ruby) in other languages, there are assert_equals which support custom message. So, when I noticed that assert_eq! doesn't accept custom message while assert! supports it, I thought it's just lack of feature or something not design decision.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Jun 1, 2016

Member

+1

Member

nrc commented Jun 1, 2016

+1

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu Jun 6, 2016

Contributor

@brson @GuillaumeGomez Well, should I close this PR...?

Contributor

komamitsu commented Jun 6, 2016

@brson @GuillaumeGomez Well, should I close this PR...?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jun 6, 2016

Member

I agree with @brson, it's simplest to just use assert for a custom message, since assert_eq basically exists just to give a better default message.

Member

aturon commented Jun 6, 2016

I agree with @brson, it's simplest to just use assert for a custom message, since assert_eq basically exists just to give a better default message.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 10, 2016

Contributor

I think that @brson and @aturon's logic makes sense, but I confess that I have been regularly surprised that assert_eq doesn't accept a custom message -- and rewriting to use assert never occurred to me.

Contributor

nikomatsakis commented Jun 10, 2016

I think that @brson and @aturon's logic makes sense, but I confess that I have been regularly surprised that assert_eq doesn't accept a custom message -- and rewriting to use assert never occurred to me.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 10, 2016

Contributor

Like @komamitsu, I have mostly used assert_eq because it seemed more declarative (with a nicer message to boot).

Contributor

nikomatsakis commented Jun 10, 2016

Like @komamitsu, I have mostly used assert_eq because it seemed more declarative (with a nicer message to boot).

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 10, 2016

Contributor

Well, maybe another way to put it is that I got in the habit of using assert_eq long ago, and so now I just do it out of habit whenever there is an equality comparison involved.

Contributor

nikomatsakis commented Jun 10, 2016

Well, maybe another way to put it is that I got in the habit of using assert_eq long ago, and so now I just do it out of habit whenever there is an equality comparison involved.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 10, 2016

Contributor

@nikomatsakis good point that having the error message reduces the surprise factor

Contributor

brson commented Jun 10, 2016

@nikomatsakis good point that having the error message reduces the surprise factor

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Jun 10, 2016

Member

to clarify, +1 to the PR itself - while I understand the argument that assert_eq should do its own thing for the error message, I don't see that as the point of the macro - aiui, assert_eq just saves you writing == (and is implicitly self-documenting), so it makes sense to have an optional message in the same way assert has. I must admit, I've never really been a fan of assert_eq at all - perhaps this mental model is why I don't see the benefit.

Member

nrc commented Jun 10, 2016

to clarify, +1 to the PR itself - while I understand the argument that assert_eq should do its own thing for the error message, I don't see that as the point of the macro - aiui, assert_eq just saves you writing == (and is implicitly self-documenting), so it makes sense to have an optional message in the same way assert has. I must admit, I've never really been a fan of assert_eq at all - perhaps this mental model is why I don't see the benefit.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Jun 10, 2016

Contributor

As I wrote in rust-lang/rfcs#1604, I think the panic message given to assert_eq! should be in addition (concatenated) to the default one. If the default one is replaced entirely instead, that’s the same as assert!(a == b, "message").

IMO the point of assert_eq! over assert! is that it prints the two values for you, so you don’t have to. It can be desirable to have that, and also provide more information/context in the panic message.

Contributor

SimonSapin commented Jun 10, 2016

As I wrote in rust-lang/rfcs#1604, I think the panic message given to assert_eq! should be in addition (concatenated) to the default one. If the default one is replaced entirely instead, that’s the same as assert!(a == b, "message").

IMO the point of assert_eq! over assert! is that it prints the two values for you, so you don’t have to. It can be desirable to have that, and also provide more information/context in the panic message.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jun 10, 2016

Member

OK, @nikomatsakis makes a convincing case that it's worth adding something here. @SimonSapin's suggestion makes sense for added value.

cc @rust-lang/libs

Member

aturon commented Jun 10, 2016

OK, @nikomatsakis makes a convincing case that it's worth adding something here. @SimonSapin's suggestion makes sense for added value.

cc @rust-lang/libs

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 10, 2016

Member

I feel that as-is this may not add too much value as it's shorter even to use assert! and you'd get the same message, but if we were to append the message to the {left} != {right} then it seems reasonable to me.

Member

alexcrichton commented Jun 10, 2016

I feel that as-is this may not add too much value as it's shorter even to use assert! and you'd get the same message, but if we were to append the message to the {left} != {right} then it seems reasonable to me.

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu Jun 13, 2016

Contributor

Thanks for the feedbacks. I agree to appending a custom message to the default one.

I have two ideas to do that.

macro_rules! assert_eq {
    ($left:expr , $right:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                           (left: `{:?}`, right: `{:?}`)", left_val, right_val)
                }
            }
        }
    });
    ($left:expr , $right:expr, $msg:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                            (left: `{:?}`, right: `{:?}`): {}", left_val, right_val, $msg)
                }
            }
        }
    });
    ($left:expr , $right:expr, $fmt:expr, $($arg:tt)*) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!(concat!("assertion failed: `(left == right)` \
                                   (left: `{:?}`, right: `{:?}`): ", $fmt),
                           left_val, right_val, $($arg)*)
                }
            }
        }
    });
}

// assert_eq!(1, 2);     >>> assertion failed: `(left == right)` (left: `1`, right: `2`)
// assert_eq!(1, 2, "Type is u32");
//                       >>> assertion failed: `(left == right)` (left: `1`, right: `2`): Type is u32
// assert_eq!(1, 2, "concurrency = {concurrency}, counter = {counter}",
//                   concurrency = c, counter = i);
//                       >>> assertion failed: `(left == right)` (left: `1`, right: `2`): concurrency = 8, counter = 65536
macro_rules! assert_eq {
    ($left:expr , $right:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                           (left: `{:?}`, right: `{:?}`)", left_val, right_val)
                }
            }
        }
    });
    ($left:expr , $right:expr, $msg:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                            (left: `{:?}`, right: `{:?}`): {}", left_val, right_val, $msg)
                }
            }
        }
    });
}

// assert_eq!(1, 2);     >>> assertion failed: `(left == right)` (left: `1`, right: `2`)
// assert_eq!(1, 2, "Type is u32"); 
//                       >>> assertion failed: `(left == right)` (left: `1`, right: `2`): Type is u32
// assert_eq!(1, 2, format!("concurrency = {concurrency}, counter = {counter}",
//                             concurrency = c, counter = i));
//                       >>>  assertion failed: `(left == right)` (left: `1`, right: `2`): concurrency = 8, counter = 65536

The first one can format strings by itself, but it's a bit verbose. The implementation of the second one is simpler, but it doesn't support formatting. I'm wondering which one is better...

Contributor

komamitsu commented Jun 13, 2016

Thanks for the feedbacks. I agree to appending a custom message to the default one.

I have two ideas to do that.

macro_rules! assert_eq {
    ($left:expr , $right:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                           (left: `{:?}`, right: `{:?}`)", left_val, right_val)
                }
            }
        }
    });
    ($left:expr , $right:expr, $msg:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                            (left: `{:?}`, right: `{:?}`): {}", left_val, right_val, $msg)
                }
            }
        }
    });
    ($left:expr , $right:expr, $fmt:expr, $($arg:tt)*) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!(concat!("assertion failed: `(left == right)` \
                                   (left: `{:?}`, right: `{:?}`): ", $fmt),
                           left_val, right_val, $($arg)*)
                }
            }
        }
    });
}

// assert_eq!(1, 2);     >>> assertion failed: `(left == right)` (left: `1`, right: `2`)
// assert_eq!(1, 2, "Type is u32");
//                       >>> assertion failed: `(left == right)` (left: `1`, right: `2`): Type is u32
// assert_eq!(1, 2, "concurrency = {concurrency}, counter = {counter}",
//                   concurrency = c, counter = i);
//                       >>> assertion failed: `(left == right)` (left: `1`, right: `2`): concurrency = 8, counter = 65536
macro_rules! assert_eq {
    ($left:expr , $right:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                           (left: `{:?}`, right: `{:?}`)", left_val, right_val)
                }
            }
        }
    });
    ($left:expr , $right:expr, $msg:expr) => ({
        match (&($left), &($right)) {
            (left_val, right_val) => {
                if !(*left_val == *right_val) {
                    panic!("assertion failed: `(left == right)` \
                            (left: `{:?}`, right: `{:?}`): {}", left_val, right_val, $msg)
                }
            }
        }
    });
}

// assert_eq!(1, 2);     >>> assertion failed: `(left == right)` (left: `1`, right: `2`)
// assert_eq!(1, 2, "Type is u32"); 
//                       >>> assertion failed: `(left == right)` (left: `1`, right: `2`): Type is u32
// assert_eq!(1, 2, format!("concurrency = {concurrency}, counter = {counter}",
//                             concurrency = c, counter = i));
//                       >>>  assertion failed: `(left == right)` (left: `1`, right: `2`): concurrency = 8, counter = 65536

The first one can format strings by itself, but it's a bit verbose. The implementation of the second one is simpler, but it doesn't support formatting. I'm wondering which one is better...

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 16, 2016

Member

Ah yeah I'd vote for the first where the message itself can have more arguments and such like assert! can

Member

alexcrichton commented Jun 16, 2016

Ah yeah I'd vote for the first where the message itself can have more arguments and such like assert! can

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu Jun 18, 2016

Contributor

Updated the PR

Contributor

komamitsu commented Jun 18, 2016

Updated the PR

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 21, 2016

Member

The libs team got a chance to discuss this PR during triage today and the conclusion was to merge. @komamitsu if you want to update with @ollie27's suggestion, I'll r+!

Member

alexcrichton commented Jun 21, 2016

The libs team got a chance to discuss this PR during triage today and the conclusion was to merge. @komamitsu if you want to update with @ollie27's suggestion, I'll r+!

@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu Jun 21, 2016

Contributor

@ollie27 Thanks. That's what I wanted to do.

@alexcrichton Okay. Updated the PR

Contributor

komamitsu commented Jun 21, 2016

@ollie27 Thanks. That's what I wanted to do.

@alexcrichton Okay. Updated the PR

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 21, 2016

Member

@bors: r+ 45a63d3

Thanks!

Member

alexcrichton commented Jun 21, 2016

@bors: r+ 45a63d3

Thanks!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 21, 2016

Member

@bors: retry force clean

  • appears... stuck
Member

alexcrichton commented Jun 21, 2016

@bors: retry force clean

  • appears... stuck
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 21, 2016

Contributor

💣 Buildbot returned an error: <span>exceptions.KeyError</span>: <span>'auto-linux-32cross-opt'</span>

Contributor

bors commented Jun 21, 2016

💣 Buildbot returned an error: <span>exceptions.KeyError</span>: <span>'auto-linux-32cross-opt'</span>

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 21, 2016

Contributor

⌛️ Testing commit 45a63d3 with merge a32aaea...

Contributor

bors commented Jun 21, 2016

⌛️ Testing commit 45a63d3 with merge a32aaea...

bors added a commit that referenced this pull request Jun 21, 2016

Auto merge of #33976 - komamitsu:assert_eq_with_msg, r=alexcrichton
Add custom message parameter to `assert_eq!`

`assert!` macro accepts a custom message parameter and it's sometimes useful. But `assert_eq!` doesn't have it and users need to use `assert!` instead of `assert_eq!` when they want to output a custom message even if the assertion just compares two values. This pull request will resolve those cases.
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 21, 2016

Member

@bors: retry force clean

  • bots disappeared?
Member

alexcrichton commented Jun 21, 2016

@bors: retry force clean

  • bots disappeared?
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 22, 2016

Contributor

⌛️ Testing commit 45a63d3 with merge 3ee3267...

Contributor

bors commented Jun 22, 2016

⌛️ Testing commit 45a63d3 with merge 3ee3267...

bors added a commit that referenced this pull request Jun 22, 2016

Auto merge of #33976 - komamitsu:assert_eq_with_msg, r=alexcrichton
Add custom message parameter to `assert_eq!`

`assert!` macro accepts a custom message parameter and it's sometimes useful. But `assert_eq!` doesn't have it and users need to use `assert!` instead of `assert_eq!` when they want to output a custom message even if the assertion just compares two values. This pull request will resolve those cases.

@bors bors merged commit 45a63d3 into rust-lang:master Jun 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@komamitsu

This comment has been minimized.

Show comment
Hide comment
@komamitsu

komamitsu Jun 22, 2016

Contributor

@alexcrichton Thanks!

Contributor

komamitsu commented Jun 22, 2016

@alexcrichton Thanks!

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Feb 3, 2017

Is this mentioned somewhere in the docs? I looked here and thought it would not be possible to append a custom message. I'm glad it is possible.

donaldpipowitch commented Feb 3, 2017

Is this mentioned somewhere in the docs? I looked here and thought it would not be possible to append a custom message. I'm glad it is possible.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Feb 3, 2017

@donaldpipowitch

This comment has been minimized.

Show comment
Hide comment
@donaldpipowitch

donaldpipowitch Feb 4, 2017

Okay, Thank you.

donaldpipowitch commented Feb 4, 2017

Okay, Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment