On 11/26/2014 07:54 AM, Jay Pipes wrote: > On 11/26/2014 06:20 AM, Nicolas Trangez wrote: >> On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote: >>> I think pointing out that the default failure >>> message for testtools.TestCase.assertEqual() uses the terms >>> "reference" >>> (expected) and "actual" is a reason why reviewers *should* ask patch >>> submitters to use (expected, actual) ordering. >> >> Is there any reason for this specific ordering? Not sure about others, >> but I tend to write equality comparisons like this >> >> if var == 1: >> >> instead of >> >> if 1 == var: >> >> (although I've seen the latter in C code before). >> >> This gives rise to >> >> assert var == 1 >> >> or, moving into `unittest` domain >> >> assertEqual(var, 1) >> >> reading it as 'Assert `var` equals 1', which makes me wonder why the >> `assertEqual` API is defined the other way around (unlike how I'd write >> any other equality check). > > It's not about an equality condition. > > It's about the message that is produced by > testtools.TestCase.assertEqual(), and the helpfulness of that message > when the order of the arguments is reversed. > > This is especially true with large dict comparisons. If you get a > message like: > > reference: <large_dict> > actual: <large_dict> > > And the arguments are reversed, then you end up wasting time looking in > the test code instead of the real code for the thing that is different.
And my argument is that you're going to have to check the test code anyway, because without some sort of automated enforcement you can never be sure that the test author got it right. I don't personally see having to open a failing unit test as a huge burden anyway - I generally need to do that to see what the unit test is calling anyway. That said, I'm not personally bothered by this. I learned early on not to trust the expected, actual ordering so it makes no difference what the failure message is to me. I think we could save less experienced developers some aggravation by not claiming something that may not be true, but if people disagree I'm not inclined to spend a bunch of time bikeshedding about it either. :-) -Ben > > Anyway, like I said, it's not something that we can write a simple > hacking check for, and therefore, it's not something that should have > much time spent on. But I do recommend that reviewers bring it up, > especially if the patch author has been inconsistent in their usage of > (expected, actual) in multiple assertEqual() calls in their patch. > > Best, > -jay > > _______________________________________________ > OpenStack-dev mailing list > [email protected] > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
