| msg195238 - (view) |
Author: Vajrasky Kok (vajrasky) * |
Date: 2013-08-15 07:58 |
| Test enum json in Lib/test/test_json/test_enum.py is ignorant of infinity values. Also, NaN, but since NaN is a weirdo, let's not take that into account. The unit test should represent of what will work in every case. For example: def test_floats(self): for enum in FloatNum: self.assertEqual(self.dumps(enum), repr(enum.value)) This will fail if enum is infinity. This wisdom about infinity was bestowed upon me when I was reading Lib/test/test_json/test_float.py. def test_floats(self): for num in [1617161771.7650001, math.pi, math.pi**100, math.pi**-100, 3.1]: self.assertEqual(float(self.dumps(num)), num) self.assertEqual(self.loads(self.dumps(num)), num) def test_ints(self): for num in [1, 1<<32, 1<<64]: self.assertEqual(self.dumps(num), str(num)) self.assertEqual(int(self.dumps(num)), num) As you can see, in float case, we don't use str(num) because it does not work with infinity. Attached the patch to refactor the test to handle infinity value. For the completeness sake, I added the case of negative infinity and NaN as well. |
|
|
| msg195447 - (view) |
Author: Ethan Furman (ethan.furman) *  |
Date: 2013-08-17 02:06 |
| Tests look good to me. |
|
|
| msg196353 - (view) |
Author: Ethan Furman (ethan.furman) * |
Date: 2013-08-28 07:15 |
| The added tests for inf, -inf, and nan are good. The refactoring of the dictionary tests are not good. The reason is that before _json.c was fixed () it would return the string value of an IntEnum instead of the string value of the IntEnum member's value attribute. --> class Number(IntEnum): ... one = 1 ... two = 2 --> json.dumps(Number.one) 'Number.one' # should be '1' As a constant we get a failure when trying to round-trip: --> json.loads(json.dumps(Number.one)) Traceback (most recent call last): ... ValueError: Expecting value: line 1 column 1 (char 0) But as a dictionary we do not (because keys get morphed into strings): --> json.dumps({Number.one: 'one'}) '{"Number.one": "one"}' # should be '{"1": "one"}' Which round-trips fine, but yields the wrong result: >>> loads(dumps({Number.one: 'one'})) {'Number.one': 'one'} # should be {'1': 'one'} So the dictionary tests (and the list tests) are there to make sure that json.dumps is converting them properly, not to make sure that they round-trip, and by changing from repr to loads/dumps the point of the test is lost. Updated tests attached. |
|
|
| msg196698 - (view) |
Author: Ethan Furman (ethan.furman) * |
Date: 2013-09-01 02:27 |
| Fixed formatting in patch. ;) |
|
|
| msg196755 - (view) |
Author: Eli Bendersky (eli.bendersky) * |
Date: 2013-09-01 23:48 |
| lgtm |
|
|
| msg196769 - (view) |
Author: Vajrasky Kok (vajrasky) * |
Date: 2013-09-02 03:51 |
| To test the infinity, negative infinity, and NaN (Not a Number), you named the test as test_weird_floats. So implicitely, you admitted that they are floats but just weird. Yet, you named the sample data test class as NotNum. Implicitely, you were saying they are not numbers. This is contradictory. In my personal opinion, I disagree slightly with the naming because infinity and negative infinity are numbers. Why not name it WeirdNum (or SpecialNum) to make it more mathematically correct and consistent with the test naming? |
|
|
| msg196770 - (view) |
Author: Ethan Furman (ethan.furman) * |
Date: 2013-09-02 03:57 |
| From my layman's perspective they are all not numbers -- you can't add 5 to any of them and get a number back that is 5 more than you started with. By I have no problem with your proposed name -- I'll make the change. |
|
|
| msg196773 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-09-02 08:15 |
| New changeset 50e583f20d78 by Ethan Furman in branch 'default': Close #18745: Improve enum tests in test_json for infinities and NaN. http://hg.python.org/cpython/rev/50e583f20d78 |
|
|