Incomplete test coverage of PyNumber_ToBase

[JustAnotherArchivist]

I discovered that the tests for PyNumber_ToBase only ever check one small int value's conversions. The function description and implementation makes it quite clear that the intent is to support arbitrary ints, as would be expected for a function working with Python ints. Tests should be added for some large negative and positive values in different bases to ensure that that's indeed the case and also for the PyNumber_Index conversion part.

(This has likely contributed to a bug in PyPy, namely that the function only accepts values in the int64_t range in PyPy: https://foss.heptapod.net/pypy/pypy/-/issues/3765)

[mdickinson]

the tests for PyNumber_ToBase only ever check one small int value's conversions

Please could you clarify which tests you're looking at? I'm not seeing any direct C-level tests of PyNumber_ToBase (e.g., like those in test_capi), but there are indirect tests via tests for the bin, oct and hex built-in functions and for string formatting, including tests for numbers larger than 2**64.

[CharlieZhao95]

I guess you are trying to add the following test cases. I have run those cases on Windows and they work well. :)
Do these test cases need to be supplemented?

.\python.bat -m unittest -v Lib.test.test_capi.CAPITest.test_pynumber_tobase

    def test_pynumber_tobase(self):
        from _testcapi import pynumber_tobase
        self.assertEqual(pynumber_tobase(123, 2), '0b1111011')
        self.assertEqual(pynumber_tobase(123, 8), '0o173')
        self.assertEqual(pynumber_tobase(123, 10), '123')
        self.assertEqual(pynumber_tobase(123, 16), '0x7b')
        self.assertEqual(pynumber_tobase(-123, 2), '-0b1111011')
        self.assertEqual(pynumber_tobase(-123, 8), '-0o173')
        self.assertEqual(pynumber_tobase(-123, 10), '-123')
        self.assertEqual(pynumber_tobase(-123, 16), '-0x7b')
        self.assertRaises(TypeError, pynumber_tobase, 123.0, 10)
        self.assertRaises(TypeError, pynumber_tobase, '123', 10)
        self.assertRaises(SystemError, pynumber_tobase, 123, 0)

        # Test for large number(out of range of a longlong,i.e.[-2**63, 2**63-1])
        large_number = 2**64
        self.assertEqual(pynumber_tobase(large_number, 2),
                         '0b10000000000000000000000000000000000000000000000000000000000000000')
        self.assertEqual(pynumber_tobase(large_number, 8),'0o2000000000000000000000')
        self.assertEqual(pynumber_tobase(large_number, 10),'18446744073709551616')
        self.assertEqual(pynumber_tobase(large_number, 16),'0x10000000000000000')
        # Test for large negative number
        self.assertEqual(pynumber_tobase(-large_number, 2),
                         '-0b10000000000000000000000000000000000000000000000000000000000000000')
        self.assertEqual(pynumber_tobase(-large_number, 8),'-0o2000000000000000000000')
        self.assertEqual(pynumber_tobase(-large_number, 10),'-18446744073709551616')
        self.assertEqual(pynumber_tobase(-large_number, 16),'-0x10000000000000000')

[mdickinson]

@JustAnotherArchivist @CharlieZhao95 I think you're both referring to test cases that are in PyPy's test suite, not CPython's. In CPython the PyNumber_ToBase C-API function is well-tested through the various pieces of Python code that use it. In contrast, it looks as though in PyPy, PyNumber_ToBase would not be being tested through hex, oct, bin, string formatting and the like, so it makes sense that PyPy would want extra tests here.

In particular, I don't think there's a whole lot of value adding direct C-API tests for PyNumber_ToBase in CPython: those C-API tests would take the form of writing a Python wrapper for PyNumber_ToBase and testing that wrapper, but the hex, oct and bin functions are already direct wrappers for PyNumber_ToBase, and string formatting is a slightly less direct test for the decimal case.

That said, the tests for hex, bin and oct are pretty minimal; a few more testcases might not hurt. But IIUC extra testcases for those functions wouldn't have caught the PyPy issue.

[JustAnotherArchivist]

@mdickinson The Lib/test/test_capi.py tests (in CPython, not PyPy) are what I was referring to.

@CharlieZhao95 That's pretty much what I had in mind, yes. Plus tests for the PyNumber_Index part (edited that into my message above after initial submission), which I guess requires defining a little class with an __index__ method like here. The index test should also be run for both small and large numbers. And an error test for __index__ not returning an int is probably a good idea as well.

[JustAnotherArchivist]

(Side note: PyNumber_Index(o) in C is the equivalent to o.__index__() in Python, right? The docs currently don't mention this, unlike for PyNumber_Float etc.)

[mdickinson]

@mdickinson The Lib/test/test_capi.py tests (in CPython, not PyPy) are what I was referring to.

Thanks! Sorry about that - I missed that I was looking at an old checkout locally.

[mdickinson]

Side note: PyNumber_Index(o) in C is the equivalent to o.__index__() in Python, right?

More or less, yes. There's a difference for subclasses of int. See #61776 (comment) for a full description of the behaviour.

[CharlieZhao95]

The index test should also be run for both small and large numbers. And an error test for __index__ not returning an int is probably a good idea as well.

@JustAnotherArchivist I find that those cases you're talking about are already mentioned in file Lib\test\test_index.py. Here are some code snippets.

# bad index
def test_error(self):
    self.o.ind = 'dumb'
    self.n.ind = 'bad'
    self.assertRaises(TypeError, operator.index, self.o)
    self.assertRaises(TypeError, operator.index, self.n)
    self.assertRaises(TypeError, slice(self.o).indices, 0)
    self.assertRaises(TypeError, slice(self.n).indices, 0)
....

# large numbers
class OverflowTestCase(unittest.TestCase):

    def setUp(self):
        self.pos = 2**100
        self.neg = -self.pos

    def test_large_longs(self):
        self.assertEqual(self.pos.__index__(), self.pos)
        self.assertEqual(self.neg.__index__(), self.neg)
    ....

Maybe we could focus more on testing the PyNumber_Index C-API function than __index__, but I'm not sure it's beneficial.

[JustAnotherArchivist]

@CharlieZhao95 Yeah, they're tested directly, but I meant testing that PyNumber_ToBase correctly retrieves and handles the index for objects that aren't int (or a subclass of int), including values beyond the usual C limits. Otherwise, an implementation that e.g. overflows when handling the index would not be caught by the tests.

As for the TypeError, I've been bitten by functions accidentally not handling errors occurring in C API functions they call, so I figured it might be good to test that the possible exception from the PyNumber_Index call correctly bubbles up through PyNumber_ToBase. In other words, test coverage for the if (!index) return NULL; branch.