Open Stack

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
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>