| msg134158 - (view) |
Author: STINNER Victor (vstinner) *  |
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) * |
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) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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. |
|
|