I don’t have pretty much any experience in what should be tested in a code. I have an idea that every function should test just it’s own functionality and avoid testing the functionality of the function it calls. Is this a good approach?
Here is an example what I mean by that. I have these functions:
/// Used for adding endpoints to the pool,
/// Complies with address family selected at start.
pub fn add(
self: *Self,
io: std.Io,
gpa: std.mem.Allocator,
addr: std.Io.net.IpAddress,
) error{ Duplicate, OutOfMemory, WrongFamily }!u32 {
// Check if address family matches first
_ = try requireFamily(addr, self.addr_family);
// Using lockUncancelable for writing
self.lock.lockUncancelable(io);
defer self.lock.unlock(io);
// Checking duplicates after the lock to avoid racing erros
if (self.indexOfUnsafe(addr, null) != null) return error.Duplicate;
return try self.addUnsafe(gpa, addr);
}
/// Returns a family if address matches the wanted type or an error
pub fn requireFamily(
addr: std.Io.net.IpAddress,
want: std.Io.net.IpAddress.Family,
) error{WrongFamily}!std.Io.net.IpAddress {
const got: std.Io.net.IpAddress.Family = addr; // tagged union coerces to its tag
return if (got == want) addr else error.WrongFamily;
}
/// `hint` is an index the operator read from `list` or `find`
/// for a slot holding `addr`, or null.
/// It is checked first, and the sweep starts from there.
/// If not found, search wraps so a near-miss costs a couple of steps instead of N/2.
/// A wrong hint is never an error, only a slower lookup: the address is the identity.
fn indexOfUnsafe(self: Self, addr: std.Io.net.IpAddress, hint: ?u32) ?u32 {
const pool_len: u32 = @intCast(self.slots.items.len);
if (pool_len == 0) return null;
// Safe against out of bounds
const start: u32 = if (hint) |h| (if (h < pool_len) h else 0) else 0;
if (self.matchUnsafe(start, addr)) return start;
for (1..pool_len) |k| { // start was already checked
const index: u32 = @intCast((start + k) % pool_len);
if (self.matchUnsafe(index, addr)) return index;
}
return null;
}
/// Backend function for adding an endpoint
fn addUnsafe(self: *Self, gpa: std.mem.Allocator, addr: std.Io.net.IpAddress) error{OutOfMemory}!u32 {
// Pick last freed slot
if (self.free_head != free_end) {
const i = self.free_head;
const slot = &self.slots.items[i];
self.free_head = slot.next_free; // Move freed before to last freed
slot.next_free = free_end;
slot.endpoint = .{ .addr = addr };
self.live += 1;
return i;
}
try self.slots.append(gpa, .{ .endpoint = .{ .addr = addr } });
self.live += 1;
return @intCast(self.slots.items.len - 1);
}
By following my principle, add should just be tested for error.Duplicate. addUnsafe() should be tested for following the order of population and changing the live count and requireFamily() should be checked for error.WrongFamily. I have a difficulty adding this to my mental model though:
/// Mark the active endpoint healthy.
/// No-op if there is none or the handle is stale.
pub fn markCurrentGood(self: *Self, io: std.Io) void {
// Using lockUncancelable for writing
self.lock.lockUncancelable(io);
defer self.lock.unlock(io);
const addr = self.current orelse return;
const i = self.indexOfUnsafe(addr, self.hint) orelse return;
self.hint = i;
if (self.atUnsafe(i)) |ep| ep.health = .good;
}
/// Backend function that returns an endpoint
/// based upon requested index of the slot.
fn atUnsafe(self: *Self, index: u32) ?*Endpoint {
if (index >= self.slots.items.len) return null;
return if (self.slots.items[index].endpoint) |*e| e else null;
}
If I have a test like this then:
test "markCurrentGood: touches just the current endpoint" {
var c: TestCtx = .init(.ip4);
defer c.deinit();
const a = try c.add(1001);
const b = try c.add(1002);
try testing.expect(c.pool.setCurrent(testing.io, null, addr4(1001)));
c.pool.markCurrentGood(testing.io);
try testing.expectEqual(EndpointPool.Health.good, c.pool.atUnsafe(a).?.health);
try testing.expectEqual(EndpointPool.Health.untried, c.pool.atUnsafe(b).?.health);
}
indexOfUnsafe() should translate current address to index, and atUnsafe() should translate index to endpoint. Should I even have something like this? atUnsafe() should be tested for out-of-bounds and returning an empty Endpoint. matchUnsafe() should be tested for matching an index with a slot:
/// Check if slot index matches the address
fn matchUnsafe(self: Self, i: u32, addr: std.Io.net.IpAddress) bool {
if (self.slots.items[i].endpoint) |e| return std.Io.net.IpAddress.eql(&e.addr, &addr);
return false;
}
How would you handle stuff like this?