Add NetworksWithin #65
Conversation
traverse_test.go
Outdated
| @@ -46,3 +47,262 @@ func TestNetworksWithInvalidSearchTree(t *testing.T) { | |||
| assert.NotNil(t, n.Err(), "no error received when traversing an broken search tree") | |||
| assert.Equal(t, n.Err().Error(), "invalid search tree at 128.128.128.128/32") | |||
| } | |||
|
|
|||
| func TestNetworksWithinV4SearchInV4Db(t *testing.T) { | |||
There was a problem hiding this comment.
These probably could have been a single table driven test given that the structure of each test is pretty much identical.
There was a problem hiding this comment.
I've cleaned these up. The tests are now much more succinct.
| "1.1.1.1/32", | ||
| "1.1.1.2/31", | ||
| "1.1.1.4/30", | ||
| "1.1.1.8/29", |
There was a problem hiding this comment.
I think it would be good to have a test that selects multiple records, but only a subset of the ones within an address space. For instance, 81.2.69.128/26 in the GeoIP2 Country test database.
traverse_test.go
Outdated
| } | ||
|
|
||
| func TestNetworksWithinV6SearchInV6Db(t *testing.T) { | ||
| var network = &net.IPNet{IP: make(net.IP, 16), Mask: net.CIDRMask(0, 128)} |
There was a problem hiding this comment.
The next three test functions largely duplicate the Networks() test. I think we would get better coverage of the expected behaviors by searching within limited subnets for them.
| return networks | ||
| } | ||
|
|
||
| func (r *Reader) NetworksWithin(network *net.IPNet) *Networks { |
There was a problem hiding this comment.
This needs documentation. I think your earlier PR had documentation. Also, an example similar to this one for Networks() would be nice:
maxminddb-golang/example_test.go
Line 56 in 7abc676
This would get included in the generated documentation.
|
I think I have addressed all of the comments. I'm happy to make more changes if needed. |
| } | ||
|
|
||
| // NetworksWithin returns an iterator that can be used to traverse all networks | ||
| // in the database which are contained in a given network. |
There was a problem hiding this comment.
A note clarifying what happens when the provided network is within a network in the database would be worthwhile, e.g., something like, "If the provided network is contained within a network in the database, the iterator will iterate over exactly one network, the containing network." I am not attached to the wording so feel free to make it clearer.
There was a problem hiding this comment.
Works for me! Added in 5e7ecc7
| Expected []string | ||
| } | ||
|
|
||
| var tests = []networkTest{ |
There was a problem hiding this comment.
Looks good. I notice that we don't have any tests for networks with no records. Could you add a test case for that?
in the case where the network being searched is contained within another network.
No description provided.