Split the Address struct into an IpAddress and SocketAddress struct #2259
Conversation
IntegratedQuantum
left a comment
IntegratedQuantum
left a comment
There was a problem hiding this comment.
Please also replace existing IP+port parsing code (→Connection, STUN).
src/network.zig
Outdated
| } | ||
|
|
||
| fn resolve(allocator: NeverFailingAllocator, addr: []const u8, port: u16) !IpAddress { | ||
| const list = try std.net.getAddressList(allocator.allocator, addr, port); |
There was a problem hiding this comment.
Use the stackAllocator for local allocations
There was a problem hiding this comment.
This doesn't work in tests
There was a problem hiding this comment.
What is the error? It seems odd that it wouldn't work. Can't you test network code in Zig?
There was a problem hiding this comment.
What is the error? It seems odd that it wouldn't work. Can't you test network code in Zig?
The stack allocator isn't available during tests.
| } | ||
| }; | ||
|
|
||
| test "Parse address" { |
There was a problem hiding this comment.
Please also add a test for resolve
There was a problem hiding this comment.
How would I do that? Domain names can change IPs, should I just test to make sure that the parsing functions match the resolve functions for IPs?
There was a problem hiding this comment.
Not all of them. localhost is one such example
There was a problem hiding this comment.
localhost is one such example
Depending on the computer's config it might resolve differently.
There was a problem hiding this comment.
Yeah, I could edit it in /etc/hosts if I wanted to, but is that something we should worry about?
There was a problem hiding this comment.
I think we should include some of those undead services, like www.google.com
If you are not connected to the internet, then any service apart from localhost is dead.
Yeah it would be great to test if Cubyz works w/o internet connection right?
But most of the time we care about name resolution in the context of working with an internet connection, so I am proposing positive test cases first.
There was a problem hiding this comment.
You should be able to run all test cases without an internet connection.
That is a hard requirement for me, since it makes development more accessible (e.g. I can test code when I'm on the train, without having to figure out how to setup their free wifi).
There was a problem hiding this comment.
Test cases should just mark themselves as skipped if they can't be runner in current environment. With your requirement you will never be able to test anything platform specific, as you can't be running both windows and Linux as host os at once. Test cases may not be ran every time, not all at once, but they have to be checked at least in CI. It's normal, it's everywhere, let's not reinvent the wheel.
There was a problem hiding this comment.
That would be good, yes. But I would still rather avoid adding another dependency on a random service. We haven't used google for anything, so why add a dependency on it for some stupid test case?
The only online dependency we have is github at the moment, but to be honest I do not think we should increase or dependency on github for a stupid test case.
Furthermore the thing we are testing is probably something that should have been tested in the Zig repo, and not in our code. What we care about is that the hostname is forwarded to the right function, not that the function produces the right results for some arbitrary hostname. For that "localhost" is enough.
There was a problem hiding this comment.
Plus any online connection increases the chance sporadic CI failures.
We have already seen this for the existing online dependencies (see https://discord.com/channels/443805812390100992/485498631156138005/1433920016318988398)
3 participants
Required for #2180