Open Stack

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.

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