Skip to content
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

propose assert_ne #1653

Merged
merged 4 commits into from
Jul 27, 2016
Merged

propose assert_ne #1653

merged 4 commits into from
Jul 27, 2016

Conversation

ashleygwilliams
Copy link
Member

this is an rfc to add a assert_ne macro to compliment assert_eq. it exists as a crate: https://crates.io/crates/assert_ne.

@KalitaAlexey
Copy link

@ashleygwilliams How often do you need assert_ne? I didn't want it once

@ashleygwilliams
Copy link
Member Author

ashleygwilliams commented Jun 17, 2016

@KalitaAlexey i found that when writing tests i wanted it pretty often and on tweeting that i made the crate i got feedback that people were excited and wanted me to try to get it into the language. hence this rfc.

[summary]: #summary

`assert_ne` is a macro that takes 2 arguments and panics if they are equal. It
works and is implemented identically to `assert_eq` and serves as its compliment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/compliment/complement/

(And below.)

@brendanzab
Copy link
Member

brendanzab commented Jun 17, 2016

I personally have found it annoying sometimes fall back on assert!(x != y), since it yeilds a much less helpful error message on failure. 👍 That said, I now expectest for unit test expectations.

@jdm
Copy link

jdm commented Jun 17, 2016

I have often come across cases where I would prefer to use assert_ne.

@ereichert
Copy link

Yes, to this. assert_ne! is easier to read, less error prone, and more ergonomic. I usually wind up writing some form of this in a lot of my projects.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 17, 2016

Bikeshed alert:

I have a slight preference for a more informative suffix than _ne. I would be happy with e.g. assert_neq!

  • (I would also be happy with assert_not_eq!, but that is a bit more difficult to type especially with the second underscore.)

I think that assert_neq! is more likely to be immediately comprehensible than assert_ne!, at least for newcomers.


I understand where ne comes from, and ne makes a lot of sense in the context of e.g. bash test conditions ([ "$x" -ne "$y" ]), or assembly programming (e.g. repne in x86, bne in mips and alpha, etc).

But in either of those cases, there tends to be an emphasis on both:

  1. brevity and
  2. regularity (thus encouraging all predicates have same length in characters).

In Rust we have certainly de-emphasized 1 in favor of familiarity (e.g. the shift from next to continue). I'm not sure how much emphasis we place on 2, but I don't think it trumps familiarity.

Update: (I admit I have not proven whether neq is more "familiar" to our audience than ne.)

@ashleygwilliams
Copy link
Member Author

@pnkfelix totally hear this and mostly agree! i chose assert_ne based on the overwhelming preponderance of ne in the source (https://github.com/rust-lang/rust/search?utf8=%E2%9C%93&q=ne&type=Code).

would be totally open to changing it if others agreed.

@brendanzab
Copy link
Member

brendanzab commented Jun 17, 2016

@pnkfelix: I agree with your sentiments, but also think about other things in the API, like PartialEq::ne - I would favor consistency in this case.

@BurntSushi
Copy link
Member

@bjz One rarely types or reads ne for PartialEq though, instead using !=. (Of course, this doesn't necessarily rebuke your consistency argument, but maybe dulls it slightly.)

@oli-obk
Copy link
Contributor

oli-obk commented Jun 17, 2016

There are many other operations that would make sense like all the Ord ops etc. If one new macro is added, it would make sense to add the others, too. But there's also an alternative:

If assert! were rewritten as a syntax extension instead of as a macro as it is now, then it would be possible to automatically detect assert!(x < y), assert!(x == y), assert!(x != y), ...

This has the major advantage of not needing textual identifiers, but using the actual operation and still producing a nice error message. It would probably be possible to improve the errors produced by these macros, since the syntax extension can produce better spans than macros.

@dgrunwald
Copy link
Contributor

I think making assert! a syntax extension that inspects the condition is the way to go.

Pytest does the same thing (demo) and I've found that extremely helpful.

@mcarton
Copy link
Member

mcarton commented Jun 17, 2016

assert_eq!(x, y) requires x: Debug and y: Debug while assert!(x == y) does not.
An assert! syntax extension could not require x: Debug and y: Debug for compatibility so it wouldn't do much.

@dgrunwald
Copy link
Contributor

A syntax extension could convert assert!(x == y) into a function call assert_eq(&x, &y), and that function can then use specialization to act differently depending on the types of x and y.

@brendanzab
Copy link
Member

Given that we already have an assert_eq! macro, would it not make more sense to plug that gap in the interim before embarking on a more ambitious change to the semantics of the assert! macro?

@wycats
Copy link
Contributor

wycats commented Jun 18, 2016

@ashleygwilliams Thanks so much for this. I have wanted this occasionally, and falling back to bare assert is annoying as you said. It's encouraging that your crate is already getting use, and that people on this thread have expressed demand for the feature.

I agree that a more ambitious refactor of assert! is a good thing to expore, but as @bjz said, given that we already have an assert_eq!, adding this functionality seems like a good medium-term approach to adding the necessary functionality, without adding too much avoidable cruft.

I'm for this 😄

@kennytm
Copy link
Member

kennytm commented Jun 18, 2016

Why not include assert_{gt|lt|ge|le}! as well?

@luser
Copy link

luser commented Jun 18, 2016

I rolled my own assert_neq! by copy-pasting assert_eq out of the standard library, so I would have liked to have this:
https://github.com/luser/sccache2/blob/fd5da79fe896fbdc39a711e257dd370644b0d526/src/test/utils.rs#L37
(I can't remember why I chose neq instead of ne.)

@tbu-
Copy link
Contributor

tbu- commented Jun 21, 2016

I don't think we need this in the standard library, this should be fine to live in a crate. You could use this: more-asserts.

@joshtriplett
Copy link
Member

joshtriplett commented Jun 21, 2016

I specifically wished for this a few days ago when writing test code.

I'd love to see a smarter assert!, too, but I'd like to have assert_ne! available for consistency in the interim.

Thanks for proposing this!

@Evrey
Copy link

Evrey commented Jun 21, 2016

Like @kennytm I'd prever having all comparison operators represented by assert macros, i.e.:

assert_eq!, assert_ne!, assert_ge!, assert_gt!, assert_le!, and assert_lt!, plus their debug siblings. So, in a way, I propose making the crate more_asserts standard stuff.

Having only assert_eq! and nothing else does not just feel lazy, but I, too, also use various comparing assertions besides equality testing in my tests. And having not just assert! but also assert_eq! is more than enough justification for me to import all the others.

@ashleygwilliams
Copy link
Member Author

i'm happy to write the rest of these as per @Evrey , i actually have a crate called asserts where i'm collecting this functionality. (looks like this was published the exact same day i wrote that crate and opened this rfc? by a mozilla employee no less- were we in the same room?)

in other news, i've updated my assert_ne crate to reflect the recent addition of custom messages to assert_eq https://github.com/rust-lang/rust/pull/33976/files

@ashleygwilliams
Copy link
Member Author

i would propose that using a crate is not ideal, if only because when updates like the one i just mentioned happen the behavior is out of sync. it'd be great to just be able to edit and improve them all at once.

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jun 21, 2016
@pnkfelix
Copy link
Member

@tbu- ah I forgot that the alternative format of assert! allows for arbitrary format inputs after the string literal.

In a quick search over the rustc code base, I didn't see too many instances of that particular pattern; there were variations of it, like this: https://github.com/rust-lang/rust/blob/master/src/librustc_back/dynamic_lib.rs#L133 but even that includes a little bit more context (the argument to cos), and as you say, even it could be written with assert! instead.

@lambda
Copy link
Contributor

lambda commented Jul 19, 2016

One good way to look at this is to look at examples from a language that has a richer set of assertions built in, and has for a long time so people have had a chance to use them.

Python has a fairly rich set available for unit tests. If I grep the Django codebase for anything that looks like a call to one of the unit test assertions, I see the following top 20. Note that several of these are custom assertions, not the built in ones:

$ git grep -E 'self.assert[a-zA-Z0-9_]+\(' | grep -Eo 'self.assert[a-zA-Z0-9]+' | sort | uniq -c | sort -nr | head -20
12279 self.assertEqual
1375 self.assertQuerysetEqual
1192 self.assertRaises
1050 self.assertTrue
 842 self.assertIn
 727 self.assertRaisesMessage
 672 self.assertFalse
 664 self.assertContains
 445 self.assertHTMLEqual
 443 self.assertIsNone
 431 self.assertIs
 380 self.assertNumQueries
 356 self.assertIsInstance
 298 self.assertNotIn
 180 self.assertRedirects
 152 self.assertOutput
 144 self.assertNoOutput
 137 self.assertNotContains
 135 self.assertNotEqual
 133 self.assertTableNotExists

assertEqual is by far the most common, with an order of magnitude more uses than the next. The next appears to be a Django specific equality assertion. After that comes assertRaises for testing that some thunk raises an exception, followed by the generic assertTrue, and then assertIn for testing for membership in a collection. There are a variety of uses of both built in and custom methods after that.

assertNotEqual is down at number 19. In total, if I replace the head -20 with wc -l, there are 122 different assertions used, most of them custom. Python itself comes with 30 different assertions; assertNotEqual is 11 out of the assertions that Python provides (or 12 if Django used the built-in but newer assertRaisesRegexp rather than its custom assertRaisesMessage).

I'll also note that I first tried this on Flask, but only found one use of the above assertions. It turns out that Flask uses Pytest, which instead provides support for printing values of subexpressions directly from the assert statement.

I've also run these same search on my dayjob codebases, and see similar results; more than an order of magnitude more use of assertEqual than anything else, followed by assertRaises (and assertFailure, the async Twisted version of assertRaises), then a mix of build in ones like assertIn, assertTrue, assertFalse, assertNotIn and assertNotEqual along with a few custom ones, then a long tail of handfuls of uses of more obscure built in ones and custom ones.

Given the large proliferation of both built-in assertions, and custom assertions, in Python codebases using the standard unittest framework, I'd probably be more in favor of moving towards a pytest/power-assert style assert! macro that prints subexpressions (probably conditionally based on #[debug] or #[test] to avoid bloat in production code) and possibly adds some kind of custom diff display, as defining these endless custom assertions mostly for the sake of just displaying the values of the subexpressions seems like a lot of wasted time and effort.

@lambda
Copy link
Contributor

lambda commented Jul 19, 2016

I may have sounded a little too negative in the previous comment, so want to clarify.

I support either adding richer assertions, or making assert! smarter; I find it frustrating when I have to reinvent the wheel, just use assert! and deal with adding extra code to print out the subexpressions every time, or add more dependencies on external crates for trivial things like this. I don't think that adding assert_ne! should be blocked by bikeshedding or slippery-slope arguments on proliferation of assertions; it is a demonstrably useful assertion to have, I have needed it recently, so I would like to see it included, unless there is a more immediate plan to go with a more powerful assert!.

If we're not going with a more powerful assert!, then I'd probably want to propose more custom assert_<predicate>! macros for the same reason, but that can go in a separate RFC. I would have a slight preference for improving assert! over adding more individual assert_<predicate>!, but I find them both acceptable ways of solving the problem.

@withoutboats
Copy link
Contributor

I don't think it matters that assert_ne is needed less frequently than asset_eq. Its my expectation that when I have an "eq" form of an action, its paired with an "ne" form as well. For the occasion when "ne" is what I need, it is frustrating and disruptive to my work that the macro isn't at hand.

@Amanieu
Copy link
Member

Amanieu commented Jul 19, 2016

I think the key here is that the majority of not-equal assertions have the form assert!(x != CONSTANT). This means that if the assertion fails, you immediately know that x == CONSTANT, so there is no need to print the values being compared.

@joshtriplett
Copy link
Member

@Amanieu ...unless you're testing a PartialEq implementation that defines "semantic equivalence" for values that aren't structurally equal. In which case you'd like to know what actual values you had on both sides.

@lambda
Copy link
Contributor

lambda commented Jul 20, 2016

@Amanieu Where are you getting your "majority" from? When I look at one of my (Python) codebases, I find out of 20 assertNotEqual uses, only 6 of them have a constant on one side. Knowing what the values are, when the discrepancy in what you're expecting could come from either side of the comparison, can be useful.

When I look at Django, I see a higher percentage of comparing against a primitive constant, but I also see a good number in which neither side is a constant, and a number of tests which are testing a custom equality predicate to ensure that particular different objects don't compare as equal, while other ones do compare as equal, like @joshtriplett mentions.

@Amanieu
Copy link
Member

Amanieu commented Jul 20, 2016

@lambda In one of my C codebases I have 27 asserts which contain !=. Of these, 24 involve comparing against a constant.

@alexcrichton
Copy link
Member

The libs team got a chance to discuss this RFC during triage yesterday, and the decision was to merge. This macro helps reduce the "surprise factor" when looking at the standard library's macros and also isn't much of a maintenance burden, so it seems like an all-around win. We have also decided to not merge #1662 for now (the other family of assertion macros), but I'll write more over there in a second.

@ashleygwilliams could you also update this to include the debug_assert_ne macro akin to the debug_assert_eq macro as well? After that I'll merge.

@steveklabnik
Copy link
Member

@alexcrichton

@ashleygwilliams asked me to watch out for this decision, because she's in a few meetings today. As it turns out, she's in one literally right now. She says she'll update it later today, once she's out of said meeting.

@tbu-
Copy link
Contributor

tbu- commented Jul 26, 2016

@alexcrichton Could the response by the libs team acknowledge that inequality assertions are used less frequently? I'm fine with the decision, but I think it's not nice to only mention the arguments in favor of the decision of the libs team, rather it should state relevant counter-arguments and say that the benefits outweigh them.

Sorry to ask for this in a thread that I have participated in as well, but I think the proclaimed "all-around win" is maybe a bit disingenuous.

@tbu-
Copy link
Contributor

tbu- commented Jul 26, 2016

(Unless the libs team doesn't think that frequency of use is a relevant argument.)

@alexcrichton
Copy link
Member

@tbu- ah yes I forgot to mention that we discussed that although this seems to be used not as often as the equality macro, for example, it's still a surprise that it doesn't exist. That, when coupled with the fact that the maintenance overhead is basically zero, convinced us that it'd be fine to merge.

@ashleygwilliams
Copy link
Member Author

@alexcrichton updated. lemme know if you'd like me to squash/rebase etc.

@ashleygwilliams
Copy link
Member Author

also: is the next step that i make a PR on the language repo?

@tbu-
Copy link
Contributor

tbu- commented Jul 27, 2016

@ashleygwilliams Yup.

@alexcrichton
Copy link
Member

Awesome, thanks again @ashleygwilliams! We actually prefer to avoid squashes on RFCs to preserve history, so as-is was fine. Also yeah the next step is to make a PR against rust-lang/rust, referencing the...

Tracking issue

@ashleygwilliams
Copy link
Member Author

🎉 i'm on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assertions Proposals relating to assertions. A-panic Panics related proposals & ideas A-test Proposals relating to testing. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.