classification
Title: incorrect error text for int(base=1000, x='1')
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, chris.jerdonek, ezio.melotti, inglesp, serhiy.storchaka, vstinner
Priority: low Keywords: easy (C), patch

Created on 2012-09-26 19:41 by chris.jerdonek, last changed 2017-11-14 09:36 by vstinner.

Files
File name Uploaded Description Edit
issue16055.patch inglesp, 2012-09-28 11:17 review
issue-16055-1-failing-test.patch chris.jerdonek, 2012-09-28 13:37
issue16055-fix-with-tests.patch inglesp, 2012-09-30 10:57 review
Pull Requests
URL Status Linked Edit
PR 4376 merged CuriousLearner, 2017-11-12 09:55
PR 4389 merged python-dev, 2017-11-13 21:51
Messages (21)
msg171357 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-26 19:41
The following error text is incorrect in at least one way:

>>> int(base=1000, x='1')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: int() arg 2 must be >= 2 and <= 36

The *base* argument can also be 0.  Secondly, the text should probably say "base arg" instead of "arg 2" since 1000 is not in position 2.

The 2.7 code does not have the second issue:

>>> int(base=1000, x='1')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: int() base must be >= 2 and <= 36
msg171359 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-26 19:44
Locations:

Objects/longobject.c
1994:                        "int() arg 2 must be >= 2 and <= 36");
4273:                        "int() arg 2 must be >= 2 and <= 36");
msg171437 - (view) Author: Peter Inglesby (inglesp) * Date: 2012-09-28 11:17
The attached patch updates the error message to:

>>> int(base=100, x='123')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: int() base must be >= 2 and <= 36, or 0
msg171444 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-28 11:59
I should have said that I had started working on this issue.  I think failing tests for both messages should accompany the patch (otherwise I would have already submitted a patch).  The tricky one is the error message for PyLong_FromString(), which I believe can only be tested from C (e.g. by adding to Modules/_testcapimodule.c).
msg171453 - (view) Author: Peter Inglesby (inglesp) * Date: 2012-09-28 13:03
Ah, sorry about that.  Are you happy for me to write the test?

Poking around the C API docs suggests that I should call PyErr_Fetch() to get the value of the a raised exception, but I can't see any precedent for this in existing test code.  Can you point me to something I could use for inspiration?
msg171460 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-28 13:31
> Are you happy for me to write the test?

I had started working on that, but sure, be my guest. :)

You seem to be on the right track.  I didn't find precedent nearby either.  We basically want a C version of unittest's assertRaisesRegex() (but it can be a straight string match).  CHECK_INVALID comes close to that.

I would suggest defining a helper function that accepts an exception type and message text, and that clears the current error if it matches.  If the error doesn't match, you can restore the existing one with PyErr_Restore(), or else call raiseTestError() if there isn't one (similar to CHECK_INVALID).

I was thinking of putting the test right after the call to TESTNAME() in test_long_api(), but maybe you know a better location:

http://hg.python.org/cpython/file/default/Modules/_testcapimodule.c#l313

I'll upload what I had for the pure Python failing test since that portion was finished.
msg171463 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-28 13:37
Attaching failing test for pure Python portion.
msg171473 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-28 14:26
> +        with self.assertRaises(ValueError) as e:
> +            int('100', 1)
> +        self.assertEquals(str(e.exception),
> +                          "int() arg base must be 0 or >= 2 and <= 36")

Why not use assertRaisesRegex()?

        self.assertRaisesRegex(ValueError,
                               r'^int\() arg base must be 0 or >= 2 and <= 36$',
                               int, '100', 1)
msg171481 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-28 14:47
It could be done that way.  It just seems simpler to me to do a simple string check when regex's aren't necessary.  Then you don't have to worry about escaping characters, etc.
msg171504 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-09-28 16:50
Can't you test the PyLong_FromString code path by passing bytes to int()? int(b"42")
msg171506 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-28 17:04
Benjamin, unless I'm overlooking something, long_new() does its own check of the base argument before calling PyLong_FromString():

http://hg.python.org/cpython/file/default/Objects/longobject.c#l4251

So long_new() won't let you pass a bad base to PyLong_FromString() (which is one of the code paths to be checked).
msg171508 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-09-28 17:08
Ah, sorry, I misread what you wanted to be testing.
msg171624 - (view) Author: Peter Inglesby (inglesp) * Date: 2012-09-30 10:57
Ok, I've now attached a patch with tests.
msg171645 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-30 15:25
A few comments on the patch:

1) You should also check the exception type (e.g. by using PyErr_ExceptionMatches()).

2) If the exception doesn't match, you should restore the original exception so that the pure Python test framework will in turn raise it and the caller can inspect it.  Currently, the caller will find out only that it doesn't match but not why.  Take a look at CHECK_INVALID() to see an example of this pattern being used.

3) I would expose the functionality that checks an exception's type and text as a helper function so that it can be used throughout _testcapimodule.c.  The function is nontrivial enough that we wouldn't want to be copying and pasting it throughout if we want to check exception texts for other parts of the C API.
msg289166 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-07 15:44
Since the first parameter of int() is now positional-only, this issue looks outdated.
msg289167 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-07 15:46
> Since the first parameter of int() is now positional-only, this issue looks outdated.

Right, but Python 3.7 still has this issue: "The *base* argument can also be 0." The error message should be:

"ValueError: int() base must be >= 2 and <= 36 or 0" (add "or 0")
msg296327 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-19 13:02
Serhiy Storchaka: "set	keywords: + easy (C)"

Thanks for making easy issues as easy and no fixing them :-) But in my experience, even if the issue seems easy to *you*, you should explain which file has to be patched, and what steps should be done to write the proper change (ex: change the error message raising by this statement: "...").
msg296403 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-20 06:29
Isn't the information provided in your previous message enough?
msg306170 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-13 21:49
New changeset 28b624825eb92cb8c96fbf8da267d8d14a61a841 by Victor Stinner (Sanyam Khurana) in branch 'master':
bpo-16055: Fixes incorrect error text for int('1', base=1000) (#4376)
https://github.com/python/cpython/commit/28b624825eb92cb8c96fbf8da267d8d14a61a841
msg306193 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-14 09:35
New changeset 58cbae22930486814cc01cf9f981d9fe5e0c68f9 by Victor Stinner (Miss Islington (bot)) in branch '3.6':
bpo-16055: Fixes incorrect error text for int('1', base=1000) (GH-4376) (#4389)
https://github.com/python/cpython/commit/58cbae22930486814cc01cf9f981d9fe5e0c68f9
msg306194 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-14 09:36
Thank you Sanyam Khurana, I merged your PR and backported the fix automaticalled to Python 3.6. Can you please try to backport the fix to Python 2.7 and create a new PR?
History
Date User Action Args
2017-11-14 09:36:41vstinnersetmessages: + msg306194
2017-11-14 09:35:19vstinnersetmessages: + msg306193
2017-11-13 21:51:01python-devsetpull_requests: + pull_request4338
2017-11-13 21:49:33vstinnersetmessages: + msg306170
2017-11-12 09:55:10CuriousLearnersetstage: needs patch -> patch review
pull_requests: + pull_request4325
2017-06-20 06:29:36serhiy.storchakasetmessages: + msg296403
2017-06-19 13:02:43vstinnersetmessages: + msg296327
2017-06-18 12:15:04serhiy.storchakasetkeywords: + easy (C)
stage: test needed -> needs patch
type: behavior -> enhancement
versions: + Python 3.7, - Python 2.7, Python 3.3, Python 3.4
2017-03-07 15:46:46vstinnersetstatus: pending -> open
nosy: + vstinner
messages: + msg289167

2017-03-07 15:44:02serhiy.storchakasetstatus: open -> pending

messages: + msg289166
2013-12-09 16:00:23serhiy.storchakasetpriority: normal -> low
versions: + Python 3.4, - Python 3.2
2012-09-30 15:25:25chris.jerdoneksetmessages: + msg171645
2012-09-30 10:57:46inglespsetfiles: + issue16055-fix-with-tests.patch

messages: + msg171624
2012-09-28 17:08:47benjamin.petersonsetmessages: + msg171508
2012-09-28 17:04:47chris.jerdoneksetmessages: + msg171506
2012-09-28 16:50:58benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg171504
2012-09-28 14:47:32chris.jerdoneksetmessages: + msg171481
2012-09-28 14:26:09serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg171473
2012-09-28 13:37:33chris.jerdoneksetfiles: + issue-16055-1-failing-test.patch

messages: + msg171463
2012-09-28 13:31:03chris.jerdoneksetmessages: + msg171460
2012-09-28 13:03:16inglespsetmessages: + msg171453
2012-09-28 11:59:00chris.jerdoneksetkeywords: - easy

messages: + msg171444
2012-09-28 11:17:54inglespsetfiles: + issue16055.patch

nosy: + inglesp
messages: + msg171437

keywords: + patch
2012-09-26 19:44:44chris.jerdoneksetmessages: + msg171359
2012-09-26 19:41:00chris.jerdonekcreate