Change error message of a failing `assert_eq!`

21

Note that there would be no need to name both arguments, so all of assert_eq!(expected = foo, actual = bar), assert_eq!(foo, actual = bar), assert_eq!(expected = foo, bar), assert_eq!(actual = bar, foo), assert_eq!(bar, expected = foo), assert_eq!(foo, bar) could be accepted, which would provide 4 ways of opting into the change of wording without too much additional verbosity.

2 Likes
22

Come to think of it, the macro could just optionally accept any ident, defaulting to the current left/right.

Thus if you want

assert_eq!(calculated = x.sqrt(), reference = 1.3049523047);

you can do that too.

Or use translated labels, or whatever.

While that's technically breaking, any assert_eq! with an assignment as the expression is useless today because assignment has unit type and asserting unit equality is pointless.

9 Likes
23

One option is to wrap the assert_eq! in a custom function, so that `let user_id = 10; let actual = "Alice B."; let expected = "Alice Baker";

assert_eq!(
    actual, expected,
    "User: {} has mismatched name.  Expected {}, got {}",
    user_id, expected, actual
);`
24

Rather than fiddling around with expected vs actual and updating the macro syntax, I'd rather the message just state what the arguments in the macro were. I.e., assert_eq!(some_object.array[i], Some("foo")) could print something like 'assert some_object.array[i] == Some("foo") failed: left is XXX, right is YYY'

10 Likes
25

I appreciate the ideas mentioned

  • custom names for the sides
  • capturing the expressions for the sides

I have an anecdote that can help add some flavor to this conversation. Not saying it means we have to go a particular direction.

I maintain a snapshotting library, snapbox. You can think of it as a mix of expect-test, assert_cmd, and assert_fs (I maintain the latter two also).

snapbox started with:

  • assert_data_eq!(expected, actual)
  • OutputAssert::stdout_eq(expected)
  • Assert::eq(expected, actual)
  • Assert::subset_eq(expected_root, actual_root (for asserting that expected_root directory has a subset of the files of actual_root and those files are eq)

Note: As a reminder, as a snapshotting library, the order is not just visual but also affects whether it can detect if it is given a snapshot, a source file that it is allowed to mutate when "blessing" the current output.

Because a failed assertion is shown as a diff, this was meant to mirror diff expected actual. There might have also been a grammatical aspect to Assert::subset_eq.

Despite the analog to diff expected actual, I was regularly confused by this. OutputAssert also doesn't align with this order.

A user also opened an issue about this at Snapbox parameter order (expected, actual) breaks common convention · Issue #226 · assert-rs/snapbox · GitHub. In that, I looked at some prior art

Then in talking over the above with @Muscraft that led to "why is there a preference for one over the other" and this got me looking at prior art

Since then, we've switched the order for non-directory assertions:

  • assert_data_eq(actual, expected)
  • OutputAssert::stdout_eq(expected)
  • Assert::eq(actual, expected)
  • Assert::subset_eq(expected_root, actual_root (I assume for grammar reasons? also not as commonly used)

Since that change, snapbox has replaced cargo's bespoke assertions. I've not had any confusion and I've not heard of any confusion.

3 Likes
27

That could reveal secret information, so it's not an option for the existing assert_eq! macro. Consider an application using assert_eq!(expectedPassword, providedPassword)...

It would also require trait based specialization, to support both types that implement Debug and those that don't. So it can't be implemented at the moment.

I think assertions in production code, and assertions in tests are very different beasts, and it's not possible for one macro to be ideal for both cases. The current implementation seems fine for production assertions, and usable but not okay for test assertions.

Most of the suggestions (pattern matching, debug printing, (re)named arguments) in this thread are for the tests use-case, but those deserve their own macros (first in crates, later perhaps in std).

1 Like
28

Technically a single macro could change its behavior based on #[cfg(test)], with an option to force one behavior.

29

assert_eq!(expectedPassword, providedPassword) would reveal the secret information anyway. All a modified assert_eq would reveal would be the names of the variables that were provided to the macro. And it wouldn't need any trait specialization, either - I think you misunderstood what I was saying.

1 Like
30

It's not clear if XXX/YYY in your post is supposed to be the value of the argument or its name. But since you invoked it without specifying a name, I assumed that it's supposed to be the debug representation of the value.

What are XXX supposed to be?

31

This is the same thing that a regular assert already does, and if you have vulnerabilities or some other issue caused by information exposed by assert_eq!, there's probably a much larger problem at play.

32

They are indeed the debug representation of the arguments. Those already get printed today. The macro already requires the types of the arguments to implement Debug. The only thing that's new in their proposal is the part before, the "assert some_object.array[i] == Some("foo") failed" part.

Please check your assumptions before making claims. :slight_smile:

1 Like