I think in these cases, I would either ignore the error (username, _ := ...) or write a simple assert(t, err == nil) that makes the if check just one line. Then, fuzz NewUsername separately to handle the cases where the requested username could be invalid and those errors matter.
But hey, decent application of generics and I wouldn't necessarily change this if I saw it in the wild.
I similarly don't think I'd object to yours in the wild, but I do find it less expressive.
The advantage of MustCreate is that it communicates to the reader that this is logic that's unrelated to the system under test.
If you do assert(t, err == nil), then it looks like any other assertion in the test and makes it seem like the test is meant to exercise Username, when it's actually incidental.
If I see (username, _ := ...), it always makes me stop and ask, "Did they mean to swallow this error? Why are they swallowing this error?" The other issue is that if you tighten the rules of NewUsername such that the username in your store.ReadUser test is no longer valid, then the ReadUser test will have confusing behavior.
I think in these cases, I would either ignore the error (username, _ := ...)
Agreed. I usually test the error case and the non-error case separately, so that neither has to call t.Fatal. Each type can go through its whole set of cases without worrying about the other return value.
Nitpick: if you don't check the error, then you might trigger a panic down the line ;-)
Yup, that's true. As you say in reply to my other comment, it depends on how sure you are that something won't fail. I was taking the OP at his word: "When I write tests for ReadUser, I call NewUsername with a value I know is valid, but I still have to include a lot of verbose error-checking code to convince the compiler that everything is okay." In reply to that, my point was that if you genuinely know that the values under test are valid, and you are testing other things, then ignore the error. But you're right: that leaves open a potential problem.
Go's FAQ says assertions aren't encouraged in production or testing code.
I think you're taking this too literally. As I understand it, the FAQ discourages assertion libraries that use a DSL and obscure the Go code in your tests.
Go wants users to handle errors properly at all times.
Again, I think you're being overly strict here. The original article says this:
When I write tests for ReadUser, I call NewUsername with a value I know is valid, but I still have to include a lot of verbose error-checking code to convince the compiler that everything is okay.
nickmonad is right to point out that you simply don't have to write a lot of code to convince the compiler. _ is completely idiomatic Go for this situation.
Sure, and I disagree! Assertions are useful in production code to downgrade correctness bugs to liveness bugs. There are also many, many cases where error handling that doesn't crash the system makes complete sense. Totally fine if Go doesn't want to support it in the language, it's trivial to write.
For the issue of testing in this specific context, doing an assert(t, err == nil), or doing the if err != nil dance has the same result. t.Fatal crashes the test in effectively the same way.
My path is similar. Maybe the difference is that I try to religiously adhere to the idiomatic way of the language and when I stop believing in the values of the language and the community I switch. “Backporting” Zig wisdom to Go seems inappropriate to me.
What is a bounds check if not an assertion? It definitely seems like the Go authors are fine with assertions, considering they panic all over the place (e.g., the reflect library, nil pointer access, etc).
Simpler, but with panic ("with a value I know is valid")
Isn't panic almost always the wrong thing to do in tests? It causes the entire run to bail out messily. t.Fatal is better for cases where a test case can't continue because it doesn't blow up the entire test suite.
Maybe I'm side-stepping the point of the article but I would instead reach for a Username constructor that the compiler knows can't fail, maybe even whatever Go's QuickCheck is in the extreme case. Presumably NewUsername is tested elsewhere; I don't want its failures bleeding over into other tests.
Can you elaborate? I'm interested in improving this pattern, but I'm not sure what you mean.
Imagine the codebase has to enforce a rule that usernames can't contain uppercase letters, so it looks like this:
func NewUsername(raw string) (Username, error) {
if containsUppercaseCharacters(raw) {
return Username{}, errors.New("username must be lowercase")
}
return Username{ value: raw }
}
How would you turn that into a constructor that can't fail? You could add a "test-only" constructor that skips lowercase letter validation, but then you have to worry about clients accidentally calling it in production. And then you'd further have to create extra test-only constructors for each type.
Hmm. My Go-less-ness may be on full show here, but yeah, test-only constructors, or simply exposing the Username struct, would be an easy one if you have "only compile for test" or inline testing or like "friend" visibility. Otherwise maybe a global the_known_working_test_username would reduce both textual and semantic repetition. Or get the test to skip instead of giving a redundant failure. Or, what I was thinking from QuickCheck would be similar to if every type that could be tested also came with a list of known good examples built-in, I suppose. (Sorry I can't really give specifics, I don't really know what Go actually lets you do).
There's also the thing where there's multiple "levels": there's the plain-old-data constructors and their tests that are disjoint, then ReadUser is part of a different disjoint set that depends on those. I bet there's something insightful you could do with that, like test suites predicated on other suites or a GiveMeTheTestExamples() or something. Basically where each level doesn't have to repeat the same assertions all the time, which is the only real problem I see that's still going on with MustCreate.
And then you'd further have to create extra test-only constructors for each type.
Aha, but then you'd easily see which types don't have tests!
I may be misunderstanding your suggestion, but it seems like a rather poor idea to have separate test-only constructors or allowing the type to be constructed directly. The primary point of having a separate Username type in this case is to provide some guarantee on what usernames can be, and both options would allow to break such invariants. (Perhaps you're suggesting that the separate constructor should perform the necessary checks at compile time when the input is statically known, a la C++ constexpr, but I think this gets rather gnarly in practice.)
Aye, it is, but I'm trying to spitball compromises that aren't full property-based testing (where "something" constructs a Username from principles and hands it to the tests). Also, in this case specifically, at least ReadUser needs unchecked construction in case an invalid username gets into the database some other way.
Personally I prefer the simpler variant (mentioned in a sibling comment here already) that reads as testutil.Must(NewUsername("joe123")). The implementation in the OP separates the function and the argument, which I don't personally like.
I really hate to be like this, because this is a moderately useful pattern. But this is exactly why leaving out generics from Go for so long was such a joke. This is the type of code that you could (and did) write in any serious language 20 years ago.
nickmonad | 22 hours ago
I think in these cases, I would either ignore the error (
username, _ := ...) or write a simpleassert(t, err == nil)that makes theifcheck just one line. Then, fuzzNewUsernameseparately to handle the cases where the requested username could be invalid and those errors matter.But hey, decent application of generics and I wouldn't necessarily change this if I saw it in the wild.
[OP] mtlynch | 20 hours ago
Thanks for reading!
I similarly don't think I'd object to yours in the wild, but I do find it less expressive.
The advantage of
MustCreateis that it communicates to the reader that this is logic that's unrelated to the system under test.If you do
assert(t, err == nil), then it looks like any other assertion in the test and makes it seem like the test is meant to exerciseUsername, when it's actually incidental.If I see
(username, _ := ...), it always makes me stop and ask, "Did they mean to swallow this error? Why are they swallowing this error?" The other issue is that if you tighten the rules ofNewUsernamesuch that the username in yourstore.ReadUsertest is no longer valid, then theReadUsertest will have confusing behavior.telemachus | 22 hours ago
Agreed. I usually test the error case and the non-error case separately, so that neither has to call t.Fatal. Each type can go through its whole set of cases without worrying about the other return value.
oliverpool | 19 hours ago
Nitpick: if you don't check the error, then you might trigger a panic down the line ;-)
telemachus | 16 hours ago
Yup, that's true. As you say in reply to my other comment, it depends on how sure you are that something won't fail. I was taking the OP at his word: "When I write tests for ReadUser, I call NewUsername with a value I know is valid, but I still have to include a lot of verbose error-checking code to convince the compiler that everything is okay." In reply to that, my point was that if you genuinely know that the values under test are valid, and you are testing other things, then ignore the error. But you're right: that leaves open a potential problem.
bilinguliar | 22 hours ago
Go's FAQ says assertions aren't encouraged in production or testing code. Go wants users to handle errors properly at all times.
telemachus | 22 hours ago
I think you're taking this too literally. As I understand it, the FAQ discourages assertion libraries that use a DSL and obscure the Go code in your tests.
Again, I think you're being overly strict here. The original article says this:
nickmonad is right to point out that you simply don't have to write a lot of code to convince the compiler.
_is completely idiomatic Go for this situation.nickmonad | 22 hours ago
Sure, and I disagree! Assertions are useful in production code to downgrade correctness bugs to liveness bugs. There are also many, many cases where error handling that doesn't crash the system makes complete sense. Totally fine if Go doesn't want to support it in the language, it's trivial to write.
For the issue of testing in this specific context, doing an
assert(t, err == nil), or doing theif err != nildance has the same result.t.Fatalcrashes the test in effectively the same way.bilinguliar | 19 hours ago
I agree, so isn’t it just an indication that Zig would work better for you?
nickmonad | 12 hours ago
Zig is my preferred language! I also have a long history with Go and still use it periodically, mainly at work.
bilinguliar | 9 hours ago
My path is similar. Maybe the difference is that I try to religiously adhere to the idiomatic way of the language and when I stop believing in the values of the language and the community I switch. “Backporting” Zig wisdom to Go seems inappropriate to me.
weberc2 | 18 hours ago
What is a bounds check if not an assertion? It definitely seems like the Go authors are fine with assertions, considering they panic all over the place (e.g., the reflect library, nil pointer access, etc).
oliverpool | 22 hours ago
Simpler, but with
panic("with a value I know is valid"):Then it is a normal function call:
user := Must(NewUsername("john"))https://go.dev/play/p/XEx083CU9EJ
If you are really allergic to panic:
Called as
user := Must(NewUsername("john"))(t)https://go.dev/play/p/ngHSA4lTBhq
Unfortunately multiple return values cannot be combined with other values, so
Must(t, NewUsername("john"))is not possible.telemachus | 21 hours ago
Isn't panic almost always the wrong thing to do in tests? It causes the entire run to bail out messily.
t.Fatalis better for cases where a test case can't continue because it doesn't blow up the entire test suite.oliverpool | 19 hours ago
It depends how sure you are this will not fail.
Yes,
t.Fatalis cleaner, but also a bit more cumbersome to use (the))(t)suffix is subtle).tentacloids | 21 hours ago
Maybe I'm side-stepping the point of the article but I would instead reach for a Username constructor that the compiler knows can't fail, maybe even whatever Go's QuickCheck is in the extreme case. Presumably NewUsername is tested elsewhere; I don't want its failures bleeding over into other tests.
[OP] mtlynch | 20 hours ago
Can you elaborate? I'm interested in improving this pattern, but I'm not sure what you mean.
Imagine the codebase has to enforce a rule that usernames can't contain uppercase letters, so it looks like this:
How would you turn that into a constructor that can't fail? You could add a "test-only" constructor that skips lowercase letter validation, but then you have to worry about clients accidentally calling it in production. And then you'd further have to create extra test-only constructors for each type.
tentacloids | 19 hours ago
Hmm. My Go-less-ness may be on full show here, but yeah, test-only constructors, or simply exposing the Username struct, would be an easy one if you have "only compile for test" or inline testing or like "friend" visibility. Otherwise maybe a global
the_known_working_test_usernamewould reduce both textual and semantic repetition. Or get the test to skip instead of giving a redundant failure. Or, what I was thinking from QuickCheck would be similar to if every type that could be tested also came with a list of known good examples built-in, I suppose. (Sorry I can't really give specifics, I don't really know what Go actually lets you do).There's also the thing where there's multiple "levels": there's the plain-old-data constructors and their tests that are disjoint, then ReadUser is part of a different disjoint set that depends on those. I bet there's something insightful you could do with that, like test suites predicated on other suites or a
GiveMeTheTestExamples()or something. Basically where each level doesn't have to repeat the same assertions all the time, which is the only real problem I see that's still going on with MustCreate.Aha, but then you'd easily see which types don't have tests!
jo3_l | 19 hours ago
I may be misunderstanding your suggestion, but it seems like a rather poor idea to have separate test-only constructors or allowing the type to be constructed directly. The primary point of having a separate
Usernametype in this case is to provide some guarantee on what usernames can be, and both options would allow to break such invariants. (Perhaps you're suggesting that the separate constructor should perform the necessary checks at compile time when the input is statically known, a la C++ constexpr, but I think this gets rather gnarly in practice.)tentacloids | 18 hours ago
Aye, it is, but I'm trying to spitball compromises that aren't full property-based testing (where "something" constructs a Username from principles and hands it to the tests). Also, in this case specifically, at least ReadUser needs unchecked construction in case an invalid username gets into the database some other way.
jo3_l | 21 hours ago
Related (declined) Go proposal: https://github.com/golang/go/issues/54297, with some variations and discussion of this pattern.
Personally I prefer the simpler variant (mentioned in a sibling comment here already) that reads as
testutil.Must(NewUsername("joe123")). The implementation in the OP separates the function and the argument, which I don't personally like.bilinguliar | 22 hours ago
This is a place where generics actually look nice.
amw-zero | 21 hours ago
I really hate to be like this, because this is a moderately useful pattern. But this is exactly why leaving out generics from Go for so long was such a joke. This is the type of code that you could (and did) write in any serious language 20 years ago.