in tests, many tests pass arguments to assertEqual backwards as (observed,expected) instead of (expected, observed)

[Clint Byrum]

This is a minor bug but it will produce a very confusing error message if the test ever fails. Examples include but are not limited to nova.tests.api.openstack.compute.test_servers.

[Joe Gordon (jogo)]

I have noticed this too, unfortunately I don't know any easy way to detect these cases so the fix sounds pretty human intensive. And even worse I am not sure how to automatically enforce this in review.

[Matt Fischer (mfisch)]

I'll take a look at this one.

[Clint Byrum (clint-fewbar)]

Excerpts from Joe Gordon's message of 2013-12-12 11:38:14 UTC:
> I have noticed this too, unfortunately I don't know any easy way to
> detect these cases so the fix sounds pretty human intensive. And even
> worse I am not sure how to automatically enforce this in review.
>

Right, it is not easy. We do need to raise awareness though, as reviewers
should stop approving if they see it. It also doesn't have to be corrected
all in one commit. Going file by file may make more sense.

One automated thing that we can do is to enforce that literals must
be the first argument. Also None as the first argument should be
assertIsNone. That won't help with a complex "expected", but it will at
least stop the easy cases.

[Matt Fischer (mfisch)]

After some poking around the issue seems fairly extensive. I'd like to start on a few at least as I ramp up on the openstack dev process. Not sure whether fixing all these in one batch is feasible.

[Clint Byrum (clint-fewbar)]

Excerpts from Matt Fischer's message of 2013-12-12 23:03:20 UTC:
> After some poking around the issue seems fairly extensive. I'd like to
> start on a few at least as I ramp up on the openstack dev process. Not
> sure whether fixing all these in one batch is feasible.
>

I think doing one test file at a time is a good start. You can mark them
with Partial-Bug: #1260134 and they'll all get linked back here.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/62102

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/62102
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=8f25d2fd6cd79f12e0e5432f74cc2310c16dd47e
Submitter: Jenkins
Branch: master

commit 8f25d2fd6cd79f12e0e5432f74cc2310c16dd47e
Author: Matt Fischer <email address hidden>
Date: Fri Dec 13 15:16:51 2013 -0700

    barematal: Cleanup the calls to assertEqual

    Switch the ordering of assertEqual calls to match the correct
    (expected, observed) ordering. This makes test failures easier to read.

    Change-Id: Iecad55e33be4ab8f7ddff3b4cdbe08aa6c1b8660
    Partial-Bug: #1260134

[Joe Gordon (jogo)]

I don't think a cleanup like this is worth it at all.

python unittest doesn't align with testtools here. unittest assertEqual has no ordering, but testtools does.

http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertEqual
https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L313

Furthermore unittests order is 'actual, expected' and testrools is 'expected, observed'. I assume 'actual'=='observed'.

http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertItemsEqual

So with testtools contradicting unittests ordering, and the inability to automatically enforce this. I think we we should just live with this inconsistency.

[Joe Gordon (jogo)]

I am setting this to invalid, but am welcome to discuss this further and be proven wrong. Also blocking any patches on this bug until we sort out of this is something to fix or not.

https://review.openstack.org/#/c/63911/

[Clint Byrum (clint-fewbar)]

Excerpts from Joe Gordon's message of 2013-12-24 16:37:27 UTC:
> I don't think a cleanup like this is worth it at all.
>
> python unittest doesn't align with testtools here. unittest assertEqual
> has no ordering, but testtools does.
>
> http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertEqual
> https://github.com/testing-cabal/testtools/blob/master/testtools/testcase.py#L313
>
> Furthermore unittests order is 'actual, expected' and testrools is
> 'expected, observed'. I assume 'actual'=='observed'.
>
> http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertItemsEqual
>
> So with testtools contradicting unittests ordering, and the inability
> to automatically enforce this. I think we we should just live with this
> inconsistency.
>

UGGGHHH <shakes fist at testtools for not being more careful>

Nonetheless, We don't use unittest, we use testtools. So there is a
blocking bug which is that we should abolish direct usage of unittest.

Also I don't think "we can't enforce this automatically" is enough of
an argument to not fix a problem which affects the errors spit out by
the unit test infrastructure when things break.

[Joe Gordon (jogo)]

As Clint pointed out on IRC, testtools outputs outputs actual and expected when a unit test fails, which is confusing to no end.

I see this in the same boat as mox vs mock, we want to fix it -- but don't want to cause more churn just for this. New tests should do it right, and if re factoring old tests, we should fix this. But I don't think fixing this by itself.

[Matthew Gilliard (matthew-gilliard-u)]

While debugging a review of another patch I was confused by the test's behaviour which I eventually realised was simply a case of swapped actual/expected. I raised it in the review but we thought it was out of scope for that particular patch.

Like mfisch above I was using this as an easy patch to submit while getting used to the Nova dev process, and I completely understand its low importance. But, this is an error in the Nova codebase - the error messages given by failing tests are incorrect. I don't have the experience with OpenStack to know what the negative conseqences of this kind of churn are.

Anyway, if this bug is not valid then please -1 my patch :) Especially if there's precedent in mox/mock for low-impact test framework usage bugs like this.