Proposal: flip expected and actual in std.testing.expectEqual #4437
Comments
|
An alternative would be to create a function in |
This is expected so that
Write: |
daurnimator
added
proposal
|
Ok, when other testing frameworks (Junit, NUnit, etc.) also follow these guidelines, it should be ok to just explicitly cast the expected values to the correct type. |
|
I actually favor this proposal. Adding those extra coercions is pretty gross and imo negatively affects readability. So either we're stuck with the correct but clunky Alternately, we could see about coercing the literal to the other type if possible. |
|
I did helpers in my zigimg library to reserve the order of expected and actual because it annoyed me so much: https://github.com/mlarouche/zigimg/blob/master/tests/helpers.zig |
|
I'll reopen this issue to because there seems some support for this. |
|
#4438 proposes a different API that may solve this problem |
|
I just noticed I do it the wrong way around in all my test code. 😐 Because I read it like this: expect x to be 1 and not expect 1 to be x I guess. |
|
It's not just with optional types this is annoying. For example: const std = @import("std");
test "math" {
const num: f64 = 1.5;
std.testing.expectEqual(1.0, std.math.floor(num));
}Which leads to this error: As has been pointed out, this can be fixed with an explicit cast, which is ugly, or by swapping the arguments, which leads to the wrong error messages when a test fails. Because testing is so easy to do in Zig, and because all the documentation uses tests to demonstrate concepts, new users are quite likely to use them. It may not be obvious to them that the correct solution is to cast the expected value. When I first encountered it, I "solved" the problem by swapping the arguments, which works to catch failing tests, but leads to confusing error messages. |
|
Another solution would be to make the type explicit, so it would look like: |
|
I ended up with this wrapper as a workaround, which preserves the intended parameter order and doesn't clutter the parameter list: const testing = @import("std").testing;
pub fn expectEqual(expected: anytype, actual: anytype) void {
testing.expectEqual(@as(@TypeOf(actual), expected), actual);
}
test "expectEqual correctly coerces types that std.testing.expectEqual does not" {
const int_value: u8 = 2;
expectEqual(2, int_value);
const optional_value: ?u8 = null;
expectEqual(null, optional_value);
const Enum = enum { One, Two };
const enum_value = Enum.One;
expectEqual(.One, enum_value);
} |
The builtin `std.testing.expectEqual(expected, actual)` breaks type inference for optionals and integers by forcing the `actual` value to match the type of the `expected` value instead of vice-versa: this causes code like `expectEqual(2, int_variable)` to fail to compile, insisting that `int_variable` must be a `comptime_int` rather than interpreting `2` as whatever type `int_variable` has. Workarounds are to explicitly cast the expected value with `@as(actual_type, expected_value)`, which is clunky, or to flip the order of `actual` and `expected`, which causes misleading errors on failed assertions. This wrapper fixes this while preserving the intended parameter order. See ziglang/zig#4437 (comment) for further discussion.
|
FWIW, after working on several languages with good support for testing, my brain is hardwired for this: const got = foo();
const expected = 11;
expect(got).toBe(expected); |
|
I tried replacing the first line of It shows that actually a lot of standard library tests are using the https://github.com/gwenzek/zig/pull/2/files I suppose there is also a lot of unit tests in the wild using flipped arguments, so we should try to fix this ASAP. |
|
another example: #10326 (comment) |
|
adding a new function such as also I'm willing to take this on should it be accepted |
|
I've also found multiple examples of people messing up the order in the wild given how unintuitive it is |
|
There are examples in the standard library of it being backward, too. e.g. std/mem.zig assumes it in places (like test "indexOfDiff")... The natural thing is you type the value you're expecting, and then you say... hmm.. what should it be? All the other test infrastructures I've used do the (actual,expected) too. That is also the right way for the type propagation to go... for all the DX reasons above (comptime ints and floats, optional values). In the rare case where the actual has the type wrong, many other things are going to flag it first (e.g. probably won't compile). I don't see any need to create a new one and deprecate the old.... no working assertion would ever even see the difference in the error. Although an alias |
|
It's pretty clear that a big flaw is that the order is difficult to remember, and it's natural to put actual first when not referencing the function signature. But putting The problem pointed out in this issue does not seem like a real problem to me: // Doesn't work as the type of the expected value is comptime_int
// std.testing.expectEqual(1, x);You have to make the expected value have the correct type: std.testing.expectEqual(@as(?u8, 1), x);Shouldn't you know what type you are expecting to find? Similarly: std.testing.expectEqual(@as(?u8, null), y);This function used to take As for what we should do going forward, I do think something should change because as is people, including myself, occasionally get the order backwards. However I don't think simply flipping the order the other way around is a sufficient change to address the problem. |
|
that is indeed the correct fix, but afaik the main issue of bikeshedding was if that should be the only change or if there should be a transition period of some sort so folks can fix their current usage and ensure its in the right order |
|
Yes, but in most cases, it would just mess up test reporting, and the test would fail anyway. Also, as noted, I'm already using reversed order in some places, just because I find it way more readable than |
"just". :)
I tried doing the same but the wrong order in the reporting kept tripping me up. So while I fully support @whatisaphone's proposal, I agree with @nektro here that some care needs to be taken when releasing this. Maybe call the new version |
|
FWIW it's not uncommon for Zig std lib pre-1.0
to break backward compatibility. If changing
the parameters' order is the right choice,
API stability shouldn't be an excuse.
|
|
I couldn't agree more... Test reporting is wrong now for me, because I use this as actual,expected all the time, because the types just work, and it's more natural, and the way it works in many other unit-test environments. It would be very nice if this got fixed sometime. |
|
@McSinyx I am aware of that but one could still try to make migration as smooth as possible if it hardly costs anything. |
|
When I try to call (I doubt that's correct, perhaps another bug?) However if I reverse the params e.g. const T = struct {
raw: []const u8,
};
try expectEqual(t.raw.len, 20);
# zig build test
[...]
expected 21, found 20While I agree with everyone above, An easy option would be to simplify the language of the error message to not use expected, or actual. Which might help punt until someone smarter than I am can come up with a better fix? |
No that is the correct behavior, although the arrow may be misleading. So I can for example do: |
// reversed order, taken from zigimg
// I expect a + b to equal 42
pub fn expectEq(actual: anytype, expected: anytype) !void {
try std.testing.expectEqual(@as(@TypeOf(actual), expected), actual);
} |
|
Sorry for bringing more into the bikeshed, but I haven't seen anyone suggest invoking peer type resolution yet. fn expectEqual(expected: anytype, actual: anytype) !void {
const T = @TypeOf(expected, actual);
const expected_val = @as(T, expected);
const actual_val = @as(T, actual);
// do comparisons with expected_val and actual_val
} |
|
I don't understand why you would ever want
Or basically 99% of what you would be using in practice. Also I'd argue that it doesn't seem reasonable™️ to test types like this. Personally, I think tests should be written to test values and types should be tested when you compile your code and things compile because that's what type safety is and is a beautiful self updating test of sorts. If you want to test types you should explicitly do |
|
I just want to add my 2 cents here, as it's difficult to voice my opinion just through reactions to existing comments, even though others already have addressed the points I'm about to make. I'm fine with the order of the parameters. I'm already used to it. Others can probably get used to it. Often, the I think Here are a couple cases in my test code where swapping what coerces to what would help with: try expectEqual(@as(usize, 1), parts.len);
try expectEqual(@as(u32, 100), parts[0].id);
try expectEqual(@as(Path.EntityPart, .{ .id = 100 }), parts[0]);
try expectEqual(@as(?Entity(ctx), scope), world.getScope());
try expectEqual(scope, world.getScope().?); // I'm tempted to write this instead.
// And if you need to test against `null` you can do it using `@as`:
try expectEqual(@as(?Entity(ctx), null), world.getScope());
// Or I suppose one coud use `expect`:
try expect(world.getScope() == null);This would be nicer to write: try expectEqual(1, parts.len);
try expectEqual(100, parts[0].id);
try expectEqual(.{ .id = 100 }, parts[0]);
try expectEqual(scope, world.getScope());
try expectEqual(null, world.getScope());And speaking of testing optionals, might it also be helpful to allow for // Very wordy to do the "correct" way?
const name = e.getName();
try expect(name != null);
try expectEqualStrings("child", name.?);
// I'm tempted to write this instead:
try expectEqualStrings("child", e.getName().?);
// When I wish I could do this:
// try expectEqualStrings("child", e.getName());The |
I know this is 2+ years ago, but I had fun learning about generics whilst playing with the idea 🤩 It would be nice not to need to write the type at the start, I'm sure that there is a way somehow... |
Use pub fn expect(actual: anytype) Expect(@TypeOf(actual)) {
return Expect(@TypeOf(actual)){ .actual = actual };
}
test "expect(foo).toBe(foo)" {
const x: ?u8 = 1;
try expect(x).toBe(1);
} |
I tried that, and got: After fixing those errors, I got: |
|
I wouldn't expect pub fn expect(actual: anytype) Expect(@TypeOf(actual)) {
switch (@TypeOf(actual)) {
comptime_int, comptime_float => @compileError("actual is a number literal; did you mean to use it as the expected value?"),
else => {}
}
return Expect(@TypeOf(actual)){ .actual = actual };
} |
|
This could also be non-breaking for pub fn Expect(comptime T: type) type {
if (T == bool) return error{TestUnexpectedResult}!void;
return struct {
actual: T,
pub inline fn equals(expected: @This(), actual: T) !void {
return std.testing.expectEqualInner(T, expected.value, actual);
}
pub inline fn equalsError(expected: @This(), expected_error: anytype) !void {
return std.testing.expectError(expected_error, expected.value);
}
};
}
pub fn expect(actual: anytype) Expect(@TypeOf(actual)) {
if (@TypeOf(actual) == bool) {
if (!actual) return error.TestUnexpectedResult;
return;
}
return .{ .actual = actual };
}
fn foo(a: bool) !u32 {
return if (a) 13 else error.Foo;
}
test expect {
try expect(true);
try expect(1).equals(1);
try expect(foo(true)).equals(13);
try expect(foo(false)).equalsError(error.Foo);
} |
|
love that, and that it keeps the old code working during the transition! |
I find this appealingly clever, but I also feel that that style of test helper is not easy to write and extend in Zig. For consistency, I'd feel compelled to fold the likes of Similarly, it would leave users of Zig with an unsatisfactory choice for their own custom assertion helpers: do I continue to write top-level EDIT: While writing my followup below I realised that type-specific functions would not need to be conditionally excluded from the |
Coming from another background (Java), there's this super-popular testing library JUnit (I'm quite sure you heard about it). It had/has a similar problem: they introduced syntax |
In Zig, Let's say I want to assert that a value is contained within a slice (in practice I probably wouldn't bother to write a custom assertion function for this, but for the sake of argument!) With the status quo, I might write something like this: const std = @import("std");
fn expectContains(haystack: anytype, needle: anytype) !void {
if (std.mem.indexOfScalar(@TypeOf(needle), haystack, needle) == null) {
return error.UnexpectedTestResult;
}
}
test {
const haystack = [_]usize{ 1, 2, 3 };
const needle: usize = 2;
try expectContains(&haystack, needle);
}To phrase this as const std = @import("std");
fn contains(needle: anytype) fn ([]const @TypeOf(needle)) bool {
return struct {
fn match(haystack: []const @TypeOf(needle)) bool {
return std.mem.indexOfScalar(@TypeOf(needle), haystack, needle) != null;
}
}.match;
}
test {
const haystack = [_]usize{ 1, 2, 3 };
const needle: usize = 2;
try std.testing.assertThat(haystack, contains(needle));
}This is already more verbose and subtle than the status quo. As @milanaleksic suggests, it may make up for it through greater composability: e.g. in JUnit, letting you invert the logic with a To phrase this as const std = @import("std");
pub fn ExpectWithCustomPredicates(comptime T: type) type {
if (T == bool) return error{TestUnexpectedResult}!void;
return struct {
value: T,
// The original predicates need to be duplicated in Zig 0.11.0;
// you can't just borrow them with usingnamespace
// because they'll have the wrong type for @This().
pub fn equals(actual: @This(), expected: T) !void {
return std.testing.expectEqual(expected, actual.value);
}
pub fn equalsError(actual: @This(), expected_error: anytype) !void {
return std.testing.expectError(expected_error, actual.value);
}
pub fn contains(actual: @This(), needle: anytype) !void {
if (std.mem.indexOfScalar(@TypeOf(needle), actual.value, needle) == null) {
return error.UnexpectedTestResult;
}
}
};
}
// The original expect function needs to be duplicated too,
// nothing can really be borrowed.
pub fn expect(actual: anytype) ExpectWithCustomPredicates(@TypeOf(actual)) {
if (@TypeOf(actual) == bool) {
if (!actual) return error.TestUnexpectedResult;
return;
}
return .{ .value = value };
}
test {
const haystack = [_]usize{ 1, 2, 3 };
const needle: usize = 2;
try expect(true);
try expect(needle).equals(needle);
try expect(&haystack).contains(needle);
}
This is much more verbose and subtle than the previous two examples, even if we assume some meta magic to avoid duplicating the original predicates; and it would be more complicated to write composable predicates like |
maybe I am alone but I like writing custom helpers anyway, I did not like it at first but more I do this the better the helpers are, and I can do things like errdefer in the while loop and log extra information when it fails, whereas with JUnit-style matchers this would be impossible (or incredibly complicated). tests are supposed to be simple (so you can be confident about them failing/passing). really the only problem for me is flipping the order of arguments (or just swapping the coercion, I don't mind), because that |
Guess it depends if you are from the dusty world's of PHP, JVM or .NET, or the modern world's of Python/Rust/JS etc JS: https://jestjs.io/docs/expect#tobevalue https://github.com/Automattic/expect.js https://www.chaijs.com/ Rust: https://doc.rust-lang.org/book/ch11-01-writing-tests.html assert_eq!(result, 4);Python: https://www.dataquest.io/blog/unit-tests-python/ def test_sum(self):
calculation = Calculations(8, 2)
self.assertEqual(calculation.get_sum(), 10, 'The sum is wrong.')To me this more intuitive and I work with Java/JUnit most days (sadly), but maybe because I've written too many Javascript tests first. |
Having started with C#/.NET and Java back when I was studying, I find the Rust/Python/etc way of doing it to be much more logical. Though the JS ones seem... downright weird to be honest. And as for Zig and the zen, code needs to read well. And to me, being able to say "I expect a + b to equal 42" feels a lot more natural than the other way around. And I quite like rofrol's/alinebee's earlier suggestion on implementing it as a way to not break backwards compatibility |
and others
It may be useful to flip the arguments to
std.testing.expectEqual. Right now, the expected value is the first formal parameter and the actual value is the second formal parameter.When working with optional types, this makes testing via
expectEqualimpossible:But this may also be a very minor edge case.
The text was updated successfully, but these errors were encountered: