classification
Title: Consistency in unittest assert methods: order of actual, expected
Type: Stage: resolved
Components: Documentation, Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: ezio.melotti, georg.brandl, michael.foord, r.david.murray, rhettinger, ron_adam
Priority: high Keywords: patch

Created on 2010-11-29 00:55 by michael.foord, last changed 2011-01-28 19:52 by michael.foord. This issue is now closed.

Files
File name Uploaded Description Edit
issue10573.diff ezio.melotti, 2010-12-18 16:37
issue10573.diff ezio.melotti, 2010-12-18 20:03
expected-actual.diff michael.foord, 2011-01-28 19:44
Messages (13)
msg122752 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-11-29 00:55
The unittest documentation, argument names and implementation need to be consistent about the order of (actual, expected) for TestCase.assert methods that take two arguments. 

This is particularly relevant for the methods that produce 'diffed' output on failure - as the order determines whether mismatched items are missing from the expected or additional to the expected.

The documentation, signatures and implementation of all two argument asserts need to be checked to ensure they are the same and implemented correctly.

Personally I prefer (actual, expected).
msg124115 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-12-16 08:53
This should get fixed-up before the second beta.

Try to confirm the most common argument ordering using Google's code search to ascertain actual practice in real code.

FWIW, I also tend to use actual/expected and find the reverse to be a form Yoda-speak, "assert 5 == x", "perhaps misread the prophecy was", etc.
msg124288 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2010-12-18 16:37
The attached patch changes the following things

- assertDictContainsSubset(expected, actual, msg=None)
+ assertDictContainsSubset(subset, dictionary, msg=None)
This doesn't change the order of the args, even though the name of the method might suggest the opposite.  In the output message I left expected and actual because imho it makes sense there (e.g. "key, expected: 3, actual: 5").  This could be changed to expected/got or something else.


- assertCountEqual(expected, actual, msg=None)
+ assertCountEqual(actual, expected, msg=None)
This changes the order of the args and as a consequence the list of 'missing' and 'unexpected' elements will be swapped.  The tests don't check the error message so they all pass without modification.  It should be noted that this method didn't exist in 3.1. However 2.7 has the equivalent assertItemsEqual with the args swapped, so maybe it should be documented somewhere that while porting from 2.7 to 3.2 the order of the args should be changed accordingly.


- assertDictEqual(expected, actual, msg=None)
+ assertDictEqual(dict1, dict2, msg=None)
This is just a doc change, the actual method already uses d1 and d2 as args and their order doesn't seem to matter.

The other methods (mainly the assert*Equal) are just documented with generic first/second or foo1/foo2.  The doc for these methods could be changed to use actual/expected as a preferred order.
msg124292 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-18 16:53
If Michael and Raymond reach a consensus, this can go in before beta2.
msg124303 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-12-18 18:31
These three changes look fine.   

The argument order for assertDictContainsSubset is problematic (doesn't match its own name and doesn't match the order for other __contains__ methods elsewhere in Python), but it's too late to change it now :-(
msg124305 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-12-18 18:34
The argument order doesn't match the name (which isn't a huge deal I don't think) - but subset in dict does match the element in container order of assertIn.
msg124314 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2010-12-18 20:03
Fixed in r87389. This can be backported to 3.1/2.7 where applicable.
msg124704 - (view) Author: Ron Adam (ron_adam) * Date: 2010-12-27 04:41
The issue10573.diff file with the time stamp 20:03 has a lot of document changes that don't have corresponding code changes?
msg126689 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-01-21 04:00
Not all the arg names in the doc match with the actual name of the args (probably because the methods are expected to be used with positional args only), that's why the changes affects only the doc.
msg126911 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-01-24 00:30
There was a BDFL ruling on python-dev mailing list that assert argument names should be (first, second). 

See:

http://mail.python.org/pipermail/python-dev/2010-December/106954.html

I'm basically reverting the patch to go back to (first, second) in the docs and in assertCountEqual.
msg126912 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-01-24 00:32
Note that I looked at making the sequence comparison code symmetric - but it generates nice diffs for even nested containers by using prettyprint and difflib. Switching to a symmetric output ("in first, not in second" etc) would be very difficult without losing functionality.

Generating these diffs can be very slow though, so it may be an issue that needs revisiting.
msg127329 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-01-28 19:44
Patch to docs and minor change to assertCountEqual to not use actual / expected internally.
msg127331 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-01-28 19:50
Code patch looks good to me.  Unittest tests pass.
History
Date User Action Args
2011-01-28 19:52:41michael.foordsetstatus: open -> closed
nosy: georg.brandl, rhettinger, ron_adam, ezio.melotti, r.david.murray, michael.foord
resolution: fixed
stage: commit review -> resolved
2011-01-28 19:50:29r.david.murraysetnosy: + r.david.murray
messages: + msg127331
2011-01-28 19:44:03michael.foordsetfiles: + expected-actual.diff
nosy: georg.brandl, rhettinger, ron_adam, ezio.melotti, michael.foord
messages: + msg127329
2011-01-24 00:32:04michael.foordsetnosy: georg.brandl, rhettinger, ron_adam, ezio.melotti, michael.foord
messages: + msg126912
2011-01-24 00:30:11michael.foordsetnosy: georg.brandl, rhettinger, ron_adam, ezio.melotti, michael.foord
messages: + msg126911
2011-01-21 04:00:31ezio.melottisetnosy: georg.brandl, rhettinger, ron_adam, ezio.melotti, michael.foord
messages: + msg126689
2010-12-27 04:41:58ron_adamsetnosy: + ron_adam
messages: + msg124704
2010-12-18 20:03:09ezio.melottisetfiles: + issue10573.diff

messages: + msg124314
nosy: georg.brandl, rhettinger, ezio.melotti, michael.foord
stage: needs patch -> commit review
2010-12-18 18:34:15michael.foordsetnosy: georg.brandl, rhettinger, ezio.melotti, michael.foord
messages: + msg124305
2010-12-18 18:31:20rhettingersetnosy: georg.brandl, rhettinger, ezio.melotti, michael.foord
messages: + msg124303
2010-12-18 16:53:55georg.brandlsetnosy: + georg.brandl
messages: + msg124292
2010-12-18 16:37:29ezio.melottisetfiles: + issue10573.diff

messages: + msg124288
keywords: + patch
2010-12-16 08:53:36rhettingersetpriority: normal -> high
nosy: + rhettinger
messages: + msg124115

2010-12-15 19:08:33ezio.melottisetassignee: michael.foord -> ezio.melotti

nosy: + ezio.melotti
stage: needs patch
2010-11-29 00:55:57michael.foordcreate