test: cover ipaddr module #29

Merged

Nandika-Gupta merged 1 commit from tests/ipaddr into master

2026-10-06 13:54:45 -03:00

Nandika-Gupta commented

2026-10-05 16:27:00 -03:00

(Migrated from github.com)

Closes #24

Added tests/test_ipaddr.py and put ipaddr in the package list in tests.yml. Didn't touch ipaddr.py.

Tests cover:

is_legal_ip: good addresses, wrong number of octets, octet over 255 or negative, non-numbers, and "192.0.2.+1" (passes the octet check but inet_aton rejects it)
IP: from a string, an int and another IP, str/repr, + - & | ^, and the Error for a bad address
IPRange: from_cidr("192.0.2.0/24"), from address + netmask, in (network and broadcast count as outside), fmt_cidr, str and repr

Ran the coverage commands from CI: 951 passed, 7 skipped, ipaddr.py at 100%.

Side note: cidr is calculated with math.log, so some prefixes come out wrong (/1 gives 0, /3 gives 2). Left it alone since this issue is tests only, and the tests just use /24. Can open an issue for it if you want.

Closes #24 Added tests/test_ipaddr.py and put ipaddr in the package list in tests.yml. Didn't touch ipaddr.py. Tests cover: - is_legal_ip: good addresses, wrong number of octets, octet over 255 or negative, non-numbers, and "192.0.2.+1" (passes the octet check but inet_aton rejects it) - IP: from a string, an int and another IP, str/repr, + - & | ^, and the Error for a bad address - IPRange: from_cidr("192.0.2.0/24"), from address + netmask, `in` (network and broadcast count as outside), fmt_cidr, str and repr Ran the coverage commands from CI: 951 passed, 7 skipped, ipaddr.py at 100%. Side note: cidr is calculated with math.log, so some prefixes come out wrong (/1 gives 0, /3 gives 2). Left it alone since this issue is tests only, and the tests just use /24. Can open an issue for it if you want.

marcos-mendez commented

2026-10-06 13:54:30 -03:00

(Migrated from github.com)

Hi @Nandika-Gupta, really nice work again!

I ran it: all 27 new tests pass, the full suite passes, and ipaddr.py is at 100% line and branch coverage, untouched. Great choice using the RFC 5737 documentation ranges.

Two things you found deserve a mention, because they're the real value of tests like these:

"192.0.2.+1" gets past the octet check (int("+1") is 1) but inet_aton rejects it. A test that pins that down protects the next refactor.
in on an IPRange excludes the network and broadcast addresses. You documented it in a comment instead of "fixing" it, which is exactly right: the screens use it to validate host addresses, so the strict < is intentional.

One more thing your tests surface, for a follow-up rather than this PR: IP() accepts any integer without checking the range, so IP(2**32) or IP(-1) are created fine and only fail later, in str(), with a confusing struct.error. Would you like to open an issue describing it? A clear error at construction (Error("ip out of range")) would be the likely fix.

I'll merge once CI is green. Thanks!

Hi @Nandika-Gupta, really nice work again! I ran it: all 27 new tests pass, the full suite passes, and ipaddr.py is at 100% line and branch coverage, untouched. Great choice using the RFC 5737 documentation ranges. Two things you found deserve a mention, because they're the real value of tests like these: "192.0.2.+1" gets past the octet check (int("+1") is 1) but inet_aton rejects it. A test that pins that down protects the next refactor. in on an IPRange excludes the network and broadcast addresses. You documented it in a comment instead of "fixing" it, which is exactly right: the screens use it to validate host addresses, so the strict < is intentional. One more thing your tests surface, for a follow-up rather than this PR: IP() accepts any integer without checking the range, so IP(2**32) or IP(-1) are created fine and only fail later, in str(), with a confusing struct.error. Would you like to open an issue describing it? A clear error at construction (Error("ip out of range")) would be the likely fix. I'll merge once CI is green. Thanks!

Nandika-Gupta commented

2026-10-06 15:55:28 -03:00

(Migrated from github.com)

Thanks a lot for the detailed review and for merging! I've raised a follow-up PR for the IP() range check based on your suggestion: #30