From 07c4e33c0776ee757b4ac24f4426cab5418c414e Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Sun, 8 Jul 2012 23:06:45 +1000 Subject: Issue 14814: The new systematic tests aren't just about error reporting any more - change names accordingly. Added and tweaked some example to ensure they were covering the intended code paths --- Lib/ipaddress.py | 23 ++++++----- Lib/test/test_ipaddress.py | 98 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 85 insertions(+), 36 deletions(-) diff --git a/Lib/ipaddress.py b/Lib/ipaddress.py index 7e6f03f..c6eea7f 100644 --- a/Lib/ipaddress.py +++ b/Lib/ipaddress.py @@ -1009,15 +1009,21 @@ class _BaseV4: raise ValueError("Empty octet not permitted") # Whitelist the characters, since int() allows a lot of bizarre stuff. if not self._DECIMAL_DIGITS.issuperset(octet_str): - raise ValueError("Only decimal digits permitted in %r" % octet_str) + msg = "Only decimal digits permitted in %r" + raise ValueError(msg % octet_str) + # We do the length check second, since the invalid character error + # is likely to be more informative for the user + if len(octet_str) > 3: + msg = "At most 3 characters permitted in %r" + raise ValueError(msg % octet_str) # Convert to integer (we know digits are legal) octet_int = int(octet_str, 10) # Any octets that look like they *might* be written in octal, # and which don't look exactly the same in both octal and # decimal are rejected as ambiguous if octet_int > 7 and octet_str[0] == '0': - raise ValueError("Ambiguous leading zero in %r not permitted" % - octet_str) + msg = "Ambiguous (octal/decimal) value in %r not permitted" + raise ValueError(msg % octet_str) if octet_int > 255: raise ValueError("Octet %d (> 255) not permitted" % octet_int) return octet_int @@ -1560,18 +1566,15 @@ class _BaseV6: """ # Whitelist the characters, since int() allows a lot of bizarre stuff. - # Higher level wrappers convert these to more informative errors if not self._HEX_DIGITS.issuperset(hextet_str): raise ValueError("Only hex digits permitted in %r" % hextet_str) + # We do the length check second, since the invalid character error + # is likely to be more informative for the user if len(hextet_str) > 4: msg = "At most 4 characters permitted in %r" raise ValueError(msg % hextet_str) - hextet_int = int(hextet_str, 16) - if hextet_int > 0xFFFF: - # This is unreachable due to the string length check above - msg = "Part 0x%X (> 0xFFFF) not permitted" - raise ValueError(msg % hextet_int) - return hextet_int + # Length check means we can skip checking the integer value + return int(hextet_str, 16) def _compress_hextets(self, hextets): """Compresses a list of hextets. diff --git a/Lib/test/test_ipaddress.py b/Lib/test/test_ipaddress.py index beb2685..619fa44 100644 --- a/Lib/test/test_ipaddress.py +++ b/Lib/test/test_ipaddress.py @@ -9,19 +9,24 @@ import re import contextlib import ipaddress -class ErrorReporting(unittest.TestCase): +class BaseTestCase(unittest.TestCase): # One big change in ipaddress over the original ipaddr module is # error reporting that tries to assume users *don't know the rules* # for what constitutes an RFC compliant IP address + # Ensuring these errors are emitted correctly in all relevant cases + # meant moving to a more systematic test structure that allows the + # test structure to map more directly to the module structure + # Note that if the constructors are refactored so that addresses with # multiple problems get classified differently, that's OK - just # move the affected examples to the newly appropriate test case. - # The error reporting tests also cover a few corner cases that - # specifically *aren't* errors (such as leading zeroes in v6 - # address parts) that don't have an obvious home in the main test - # suite + # There is some duplication between the original relatively ad hoc + # test suite and the new systematic tests. While some redundancy in + # testing is considered preferable to accidentally deleting a valid + # test, the original test suite will likely be reduced over time as + # redundant tests are identified. @property def factory(self): @@ -53,7 +58,11 @@ class ErrorReporting(unittest.TestCase): return self.assertCleanError(ipaddress.NetmaskValueError, details, *args) -class CommonErrorsMixin: + def assertInstancesEqual(self, lhs, rhs): + """Check constructor arguments produce equivalent instances""" + self.assertEqual(self.factory(lhs), self.factory(rhs)) + +class CommonTestMixin: def test_empty_address(self): with self.assertAddressError("Address cannot be empty"): @@ -63,7 +72,19 @@ class CommonErrorsMixin: with self.assertAddressError(re.escape(repr("1.0"))): self.factory(1.0) -class CommonErrorsMixin_v4(CommonErrorsMixin): +class CommonTestMixin_v4(CommonTestMixin): + + def test_leading_zeros(self): + self.assertInstancesEqual("000.000.000.000", "0.0.0.0") + self.assertInstancesEqual("192.168.000.001", "192.168.0.1") + + def test_int(self): + self.assertInstancesEqual(0, "0.0.0.0") + self.assertInstancesEqual(3232235521, "192.168.0.1") + + def test_packed(self): + self.assertInstancesEqual(bytes.fromhex("00000000"), "0.0.0.0") + self.assertInstancesEqual(bytes.fromhex("c0a80001"), "192.168.0.1") def test_negative_ints_rejected(self): msg = "-1 (< 0) is not permitted as an IPv4 address" @@ -77,7 +98,7 @@ class CommonErrorsMixin_v4(CommonErrorsMixin): def test_bad_packed_length(self): def assertBadLength(length): - addr = b'\x00' * length + addr = bytes(length) msg = "%r (len %d != 4) is not permitted as an IPv4 address" with self.assertAddressError(re.escape(msg % (addr, length))): self.factory(addr) @@ -85,7 +106,23 @@ class CommonErrorsMixin_v4(CommonErrorsMixin): assertBadLength(3) assertBadLength(5) -class CommonErrorsMixin_v6(CommonErrorsMixin): +class CommonTestMixin_v6(CommonTestMixin): + + def test_leading_zeros(self): + self.assertInstancesEqual("0000::0000", "::") + self.assertInstancesEqual("000::c0a8:0001", "::c0a8:1") + + def test_int(self): + self.assertInstancesEqual(0, "::") + self.assertInstancesEqual(3232235521, "::c0a8:1") + + def test_packed(self): + addr = bytes(12) + bytes.fromhex("00000000") + self.assertInstancesEqual(addr, "::") + addr = bytes(12) + bytes.fromhex("c0a80001") + self.assertInstancesEqual(addr, "::c0a8:1") + addr = bytes.fromhex("c0a80001") + bytes(12) + self.assertInstancesEqual(addr, "c0a8:1::") def test_negative_ints_rejected(self): msg = "-1 (< 0) is not permitted as an IPv6 address" @@ -99,7 +136,7 @@ class CommonErrorsMixin_v6(CommonErrorsMixin): def test_bad_packed_length(self): def assertBadLength(length): - addr = b'\x00' * length + addr = bytes(length) msg = "%r (len %d != 16) is not permitted as an IPv6 address" with self.assertAddressError(re.escape(msg % (addr, length))): self.factory(addr) @@ -109,7 +146,7 @@ class CommonErrorsMixin_v6(CommonErrorsMixin): assertBadLength(17) -class AddressErrors_v4(ErrorReporting, CommonErrorsMixin_v4): +class AddressTestCase_v4(BaseTestCase, CommonTestMixin_v4): factory = ipaddress.IPv4Address def test_network_passed_as_address(self): @@ -162,6 +199,7 @@ class AddressErrors_v4(ErrorReporting, CommonErrorsMixin_v4): ipaddress.IPv4Address(addr) assertBadOctet("0x0a.0x0a.0x0a.0x0a", "0x0a") + assertBadOctet("0xa.0x0a.0x0a.0x0a", "0xa") assertBadOctet("42.42.42.-0", "-0") assertBadOctet("42.42.42.+0", "+0") assertBadOctet("42.42.42.-42", "-42") @@ -170,16 +208,23 @@ class AddressErrors_v4(ErrorReporting, CommonErrorsMixin_v4): assertBadOctet("1.2.3.4::", "4::") assertBadOctet("1.a.2.3", "a") - def test_leading_zeros(self): + def test_octal_decimal_ambiguity(self): def assertBadOctet(addr, octet): - msg = "Ambiguous leading zero in %r not permitted in %r" - with self.assertAddressError(msg, octet, addr): + msg = "Ambiguous (octal/decimal) value in %r not permitted in %r" + with self.assertAddressError(re.escape(msg % (octet, addr))): ipaddress.IPv4Address(addr) assertBadOctet("016.016.016.016", "016") assertBadOctet("001.000.008.016", "008") - self.assertEqual(ipaddress.IPv4Address("192.168.000.001"), - ipaddress.IPv4Address("192.168.0.1")) + + def test_octet_length(self): + def assertBadOctet(addr, octet): + msg = "At most 3 characters permitted in %r in %r" + with self.assertAddressError(re.escape(msg % (octet, addr))): + ipaddress.IPv4Address(addr) + + assertBadOctet("0000.000.000.000", "0000") + assertBadOctet("12345.67899.-54321.-98765", "12345") def test_octet_limit(self): def assertBadOctet(addr, octet): @@ -187,11 +232,11 @@ class AddressErrors_v4(ErrorReporting, CommonErrorsMixin_v4): with self.assertAddressError(re.escape(msg)): ipaddress.IPv4Address(addr) - assertBadOctet("12345.67899.-54321.-98765", 12345) assertBadOctet("257.0.0.0", 257) + assertBadOctet("192.168.0.999", 999) -class AddressErrors_v6(ErrorReporting, CommonErrorsMixin_v6): +class AddressTestCase_v6(BaseTestCase, CommonTestMixin_v6): factory = ipaddress.IPv6Address def test_network_passed_as_address(self): @@ -317,12 +362,13 @@ class AddressErrors_v6(ErrorReporting, CommonErrorsMixin_v6): with self.assertAddressError(msg, part, addr): ipaddress.IPv6Address(addr) + assertBadPart("::00000", "00000") assertBadPart("3ffe::10000", "10000") assertBadPart("02001:db8::", "02001") assertBadPart('2001:888888::1', "888888") -class NetmaskErrorsMixin_v4(CommonErrorsMixin_v4): +class NetmaskTestMixin_v4(CommonTestMixin_v4): """Input validation on interfaces and networks is very similar""" def test_split_netmask(self): @@ -352,18 +398,18 @@ class NetmaskErrorsMixin_v4(CommonErrorsMixin_v4): assertBadNetmask("1.2.3.4", "") assertBadNetmask("1.2.3.4", "33") assertBadNetmask("1.2.3.4", "254.254.255.256") + assertBadNetmask("1.1.1.1", "254.xyz.2.3") assertBadNetmask("1.1.1.1", "240.255.0.0") - assertBadNetmask("1.1.1.1", "1.a.2.3") assertBadNetmask("1.1.1.1", "pudding") -class InterfaceErrors_v4(ErrorReporting, NetmaskErrorsMixin_v4): +class InterfaceTestCase_v4(BaseTestCase, NetmaskTestMixin_v4): factory = ipaddress.IPv4Interface -class NetworkErrors_v4(ErrorReporting, NetmaskErrorsMixin_v4): +class NetworkTestCase_v4(BaseTestCase, NetmaskTestMixin_v4): factory = ipaddress.IPv4Network -class NetmaskErrorsMixin_v6(CommonErrorsMixin_v6): +class NetmaskTestMixin_v6(CommonTestMixin_v6): """Input validation on interfaces and networks is very similar""" def test_split_netmask(self): @@ -395,14 +441,14 @@ class NetmaskErrorsMixin_v6(CommonErrorsMixin_v6): assertBadNetmask("::1", "129") assertBadNetmask("::1", "pudding") -class InterfaceErrors_v6(ErrorReporting, NetmaskErrorsMixin_v6): +class InterfaceTestCase_v6(BaseTestCase, NetmaskTestMixin_v6): factory = ipaddress.IPv6Interface -class NetworkErrors_v6(ErrorReporting, NetmaskErrorsMixin_v6): +class NetworkTestCase_v6(BaseTestCase, NetmaskTestMixin_v6): factory = ipaddress.IPv6Network -class FactoryFunctionErrors(ErrorReporting): +class FactoryFunctionErrors(BaseTestCase): def assertFactoryError(self, factory, kind): """Ensure a clean ValueError with the expected message""" -- cgit v0.12