classification
Title: unittest fails on comparing str with bytes if python has the -bb option
Type: Stage: resolved
Components: Library (Lib), Tests, Unicode Versions: Python 3.1, Python 3.2, Python 3.3
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, ezio.melotti, michael.foord, ncoghlan, vstinner
Priority: normal Keywords: patch

Created on 2011-04-20 15:35 by vstinner, last changed 2011-05-06 11:22 by michael.foord. This issue is now closed.

Files
File name Uploaded Description Edit
unittest_str_bytes.patch vstinner, 2011-04-20 16:41 review
unittest_str_bytes-2.patch vstinner, 2011-04-30 23:40 review
Messages (9)
msg134158 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-04-20 15:35
assertEqual(), assertListEqual(), assertSequenceEqual() emits a BytesWarning warning or raise a BytesWarning exception if python has -b or -bb option. Attached patch adds tests to demonstrate this issue.
msg134893 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-04-30 23:40
unittest_str_bytes-2.patch:
 - add new tests to reproduce the bug
 - fix the bug: ignore temporary BytesWarning warnings while comparing objects and sequences
msg134950 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-02 05:05
I thought some more about this and I'm -0.5.
The reasons are:
  * the patch introduces code/complexity in _baseAssertEqual and other places, using catch_warnings to change and restore the warning filters at every call;
  * this is needed only when Python is run with -b/-bb and that is quite uncommon (afaik);
  * even if this is not fixed, a test suite that passes all the tests without -b/-bb will most likely pass with -b/-bb[0];
  * if there are failing tests with -b/-bb, it's usually possible to remove the -b/-bb and fix them before re-adding -bb[1];

[0]: the only exception I can think of is something like self.assertNotEqual(a_string, a_bytestring): this passes without -bb but fails with a BytesWarning with -bb. This tests is "wrong" though, because string are always not equal to bytestrings, regardless of their values. If one wants to make sure that a_string is not a bytestring, the correct way to do it is assertNotIsInstance(a_string, bytes).  self.assertEqual(a_string, a_bytestring) fails already without -bb so, even if with -bb the traceback is less useful, it can/should be fixed without -b/-bb.
To prove this further the whole Python test suite passes with -bb.

[1]: that might indeed not be the case (e.g. with our buildbots is not easy to change flags), but I'm still not sure it's worth patching this just to have a better traceback in case of a test failure that accidentally involves bytes/string comparison on a Python running with -bb on an environment where changing the flags is not easy (i.e. a rather obscure corner case).
msg134955 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-05-02 08:26
Le lundi 02 mai 2011 07:05:28, vous avez écrit :
>   * the patch introduces code/complexity in _baseAssertEqual and other
> places, using catch_warnings to change and restore the warning filters at
> every call;

Yes, and what is the problem? I think that it is cheap: it copies a list, 
prepends an item to a short list, and then copies a reference (the previous 
list).

I think that the patch is simple (it adds 3 "with+simplefilter") and it 
doesn't add "complexity", or you should define what complexity is :-)

> * this is needed only when Python is run with -b/-bb and that
> is quite uncommon (afaik);

Buildbots use "make buildbottest" which run python with -bb (which is a good 
idea!). So all buildbots already use -bb since long time.

I forgot the explain the usecase: I don't remember correctly, but a test 
failed on a buildbot, and I was unable to get more information because 
unittest "failed" too.

> * even if this is not fixed, a test suite that
> passes all the tests without -b/-bb will most likely pass with -b/-bb[0];

No. You have usually more failures with -bb than without any -b flag. Not in 
the test itself, but in a function called by the test.

> * if there are failing tests with -b/-bb, it's usually possible to remove
> the -b/-bb and fix them before re-adding -bb[1];

A test may only fail with -bb.

Anyway, my problem is to be able to get more informations on a failure in a 
buildbot. I cannot change (easily) -bb flags on the buildbots (and I don't 
want to do that). When something goes wrong on a buildbot, in some cases it is 
very hard to reproduce the failure on my own computer. I want as much 
information as possible.
msg134965 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-05-02 12:38
> I think that the patch is simple (it adds 3 "with+simplefilter") and it
> doesn't add "complexity", or you should define what complexity is :-)

The patch is indeed quite simple. but with it half of the code in _baseAssertEqual will be to deal with warnings for this corner case.

> Buildbots use "make buildbottest" which run python with -bb

Yes, but "make buildbottest" is used just by the buildbots afaik.

> No. You have usually more failures with -bb than without any -b flag.
> Not in test itself, but in a function called by the test.

That's what I meant, the tests will still work with -bb and the failures will be elsewhere (i.e. the patch won't change anything here).

> A test may only fail with -bb.

If the failure is in the test, I would say that the test is probably wrong (see e.g. the assertNotEqual example in my previous message), if the failure is elsewhere the patch won't change anything.

> Anyway, my problem is to be able to get more informations on a 
> failure in a buildbot. I cannot change (easily) -bb flags on the
> buildbots [...]

On this I agree (that's why I'm -0.5 and not -1), but as I said it's a very specific situation, and my gut feeling is that it might not be worth fixing it.
msg134966 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-05-02 13:20
Rather than fiddling with the warnings filters, wouldn't it be easier to just catch BytesWarning and return False in that case?
msg134969 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-05-02 13:49
> Rather than fiddling with the warnings filters, wouldn't it be easier
> to just catch BytesWarning and return False in that case?

It's another possible solution, but it would display a warning using python -b.
msg134975 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-05-02 14:12
I'm OK with that - I'd actually suggest explicitly *emitting* a warning when the error is suppressed under -bb, as Ezio is right that even tests should be keeping their bytes/str separation straight. I don't like completely suppressing warnings/errors when a user has explicitly requested them.

I'd also be OK with a filtering approach that coerced the handling to "default" rather than "ignore" (although that require a bit more work to keep unittest from acting as though -b was always set on the command line).
msg135277 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2011-05-06 11:22
_baseAssertEqual should not unconditionally catch and ignore all BytesWarnings.

If tests raise warnings/exceptions because they are doing comparisons that raise warnings/exceptions then those tests should be fixed rather than just ignoring the warnings.
History
Date User Action Args
2011-05-06 11:22:41michael.foordsetstatus: open -> closed
resolution: rejected
messages: + msg135277

stage: resolved
2011-05-02 14:12:08ncoghlansetmessages: + msg134975
2011-05-02 13:49:57vstinnersetmessages: + msg134969
2011-05-02 13:20:21ncoghlansetnosy: + ncoghlan
messages: + msg134966
2011-05-02 12:38:17ezio.melottisetmessages: + msg134965
2011-05-02 08:26:09vstinnersetmessages: + msg134955
2011-05-02 05:05:27ezio.melottisetmessages: + msg134950
2011-04-30 23:40:12vstinnersetfiles: + unittest_str_bytes-2.patch

messages: + msg134893
2011-04-29 17:32:11michael.foordsetnosy: + michael.foord
2011-04-20 16:41:41vstinnersetfiles: + unittest_str_bytes.patch
keywords: + patch
2011-04-20 15:35:52vstinnercreate