Add shared IP/CIDR test data to net, update ipfamily tests, add parsing tests #334
Conversation
k8s-ci-robot
added
the
sig/network
|
@danwinship: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
| ip: "bad ip", | ||
| family: IPFamilyUnknown, | ||
| }, | ||
| func checkOneIPFamily(t *testing.T, ip string, expectedFamily, family IPFamily, isIPv4, isIPv6 bool) { |
There was a problem hiding this comment.
so, this aggregate all the repetitiva assertions on the existing test
| }, | ||
| { | ||
| desc: "a byte slice of length other than 4 or 16 is an invalid net.IP", | ||
| ips: []net.IP{ |
There was a problem hiding this comment.
net/ips_test.go
Outdated
| // This tests that if you try to parse an "ifaddr-style" CIDR string with | ||
| // ParseCIDRSloppy, the *net.IPNet return value has the bits beyond the | ||
| // prefix length masked out. |
There was a problem hiding this comment.
this is the golang std parser behavior https://github.com/golang/go/blob/eb1c7f6e69b0e62067ff22a0656cedff792c8438/src/net/ip.go#L542-L550 , it this comment makes it sound like this is something specific of our fork, but IIUIC is the same
There was a problem hiding this comment.
ah, I can clarify that
| skipFamily bool | ||
| skipParse bool |
There was a problem hiding this comment.
what if instead of using these fields that makes the test data have to understand how it is used, we use different subsets and then the test can build the final slice?
// Instead of one big slice with skip fields:
var testIPs = []testCase{
{desc: "...", skipParse: true, skipFamily: false, ...},
...
}
// Create subsets for each test:
var goodTestIPsForParse = []testCase{ ... }
var goodTestIPsForFamily = []testCase{ ... }
var badTestIPsForParse = []testCase{ ... }
var badTestIPsForFamily = []testCase{ ... }
so the test will look like
func TestParseIPSloppy(t *testing.T) {
for _, tc := range goodTestIPsForParse {
t.Run(tc.desc, func(t *testing.T) {
...
})
}
for _, tc := range badTestIPsForParse {
t.Run(tc.desc, func(t *testing.T) {
...
})
}
}
There was a problem hiding this comment.
That still "makes the test data have to understand how it is used" though, it just moves the understanding from the individual-test-case level to the test-case-array level. Right?
Also, the whole point of this was to very clearly have one set of test data. When I first started the netutilsv2 branch, I copied the parsing tests over from utilvalidation in k/k, and then wrote some new tests for the conversion functions. But I realized that there ended up being cases that got tested for IPFamilyOf and AddrFromIP but not for ParseIP, or that got tested for ParsePrefix and IsIPv4CIDR but not IPNetFromPrefix, etc. I wanted to make sure I was testing all of the edge cases for all of the functions, and that they all agreed on which IPs/CIDRs are "good" and which are "bad".
The skip fields were a necessary kludge added at the end, when I realized that we couldn't have completely uniform testing given (a) in the netutilsv1 API, the functions don't entirely agree on which values are good and bad (fixed in v2), and (b) the API expectation that ifaddr-style CIDRs are accepted-but-truncated. But there are only 4 skips out of 40 test cases, because overall, the goal is still to test all the test cases against every function.
There was a problem hiding this comment.
fair, is not that my alternative is much better
| // A /32 IPMask is equivalent to 255.255.255.255 | ||
| func() net.IP { _, ipnet, _ := net.ParseCIDR("1.2.3.4/32"); return net.IP(ipnet.Mask) }(), |
There was a problem hiding this comment.
It's a somewhat goofy case... I could remove it
There was a problem hiding this comment.
it is ok, sorry, I was going through the last pass and I got interrupted, let me finish but I think is good to merge
| } | ||
| t.Run(tc.desc, func(t *testing.T) { | ||
| for i, ipnet := range tc.ipnets { | ||
| errStr := ipnet.String() |
There was a problem hiding this comment.
I was curious about what will be this output so I printed it, basically this test that the stringified version of the an invalid ipnet is not parseable
parse_test.go:93: errStr="<nil>"
=== RUN TestParseCIDRSloppy/IPNet_containing_invalid_IP_is_invalid
parse_test.go:93: errStr="<nil>"
=== RUN TestParseCIDRSloppy/IPNet_containing_non-CIDR_Mask_is_invalid
parse_test.go:93: errStr="192.168.0.0/ff00ff00"
=== RUN TestParseCIDRSloppy/IPNet_containing_IPv6_IP_and_IPv4_Mask_is_invalid
parse_test.go:93: errStr="<nil>"
--- PASS: TestParseCIDRSloppy (0.00s)
|
/lgtm It test roundtripping of paring and multiple representations and uses a single place to have the ips and ipnets testcases that simplifies creating new cases |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, danwinship The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@danwinship the PR is blocked in this job
I do not have permissions to retrigger it, @dims do you have? otherwise I think repushing will retrigger |
|
i was able to re-trigger @aojea |
originally written for #328, later reused in #330, but anyway, we currently have no tests of the parsing code, so let's fix that
/kind test
/sig network
/assign @aojea