Bool assert idea

[lpil]

What do you think?

rendered

[lpil]

@hayleigh-dot-dev and @giacomocavalieri have raised concerns with people using assert in libraries to check preconditions in functions.

I feel that introducing and documenting the feature purely in the context of testing would likely get us where we want to be, looking at Erlang and Elixir and Rust as examples of this succeeding elsewhere. I suspect most folks don't know you can even use their assertions outside of tests.

As additional encouragement we can also add another layer of nagging to gleam publish, and show when a package panics on the package index or some other documentation.

---

It was suggested we could ban panicking in published libraries, but there are lots of valid reasons to panic in libraries, even if typically it is discouraged and a bad idea. For example, OTP based code.

---

It was suggested that let assert True = could be upgraded to have all these features, but it's unwieldy to type and to read, and it fails the strangeness budget test for simple assertions, so it doesn't meet the goal of improving Gleam's test DX.

[giacomocavalieri]

It was suggested we could ban panicking in published libraries, but there are lots of valid reasons to panic in libraries, even if typically it is discouraged and a bad idea. For example, OTP based code.

Do you have any examples? Looking at gleam_otp the only place where it's used is doing some let assert Ok(thing) = ... and couldn't really be replaced with anything else.

In the tests and examples there's a couple of places doing let assert True = ...:

- let assert True = process.is_alive(p1)
+ assert process.is_alive(p1)

And there's also a couple of other assertions that could be rewritten but I'm not sure I like it:

- let assert Error(Nil) = process.receive(subject, 10)
+ assert result.is_error(process.receive(subject, 10))

- let assert Error(Timeout) = task.try_await(t3, 35)
+ assert Error(Timeout) == task.try_await(t3, 35)

What would become the preferred style in those cases? If we're not making let assert carry around the same kind of info assert does I can see people gravitating towards the latter just because it would produce more detailed error messages.
This is just my opinion but I think it's a net negative to favour bool returning functions instead of pattern matching and let assert.

[lpil]

I don't have examples to hand, but it would be any case there a precondition is checked.

The error checking example you have there should be rewritten like this:

assert process.receive(subject, 10) == Error(Timeout)

Now == is always used for equality rather than you choosing between different approaches depending on what error message you want and how much boilerplate you are willing to tolerate.

[giacomocavalieri]

But then if I want to change it in the future and assert that it's just an error do you think it would be easier to go this way:

- assert process.receive(subject, 10) == Error(Timeout) 
+ let assert Error(_) = process.receive(subject, 10)

Or this way:

- assert process.receive(subject, 10) == Error(Timeout) 
+ assert process.receive(subject, 10) |> result.is_error

To me the second one feels encouraged.

And it's a bit unexpected that writing let assert Error(Nil) = wibble will produce a different and worse error message than assert Error(Nil) == wibble.
What should happen to all let asserts that check a pattern that could be rewritten as an assert?

[lpil]

To me the second one feels encouraged.

Why do you think that? To use equality rather than a helper function is most common way to write that test both in Gleam and the other languages I'm familiar with.

And it's a bit unexpected that writing let assert Error(Nil) = wibble will produce a different and worse error message than assert Error(Nil) == wibble.

Perhaps unintuitive, but easy to learn, and it's the norm and doesn't cause problems for any of the other languages referenced. Equality is the simpler and more common feature so I don't think people would default to skipping over it in favour of the more complex partial pattern matching.

[jhillyerd]

I'm in favor of this. I think the existing gleeunit/should module is clunky to use, and find it frustrating that it does not accept custom error/context messages. This proposal allows for concise, readable, and flexible test assertions. (edit: and I like that the assertions are just gleam, rather than a DSL)

I don't think it needs to be blocked from use in non-test code, but I do expect it will lead to folks requesting a "disable assertions in production builds" feature.

[bentomas]

To me the second one feels encouraged.

I think you can discourage the second one by having whatever updated test runner that replaces the current gleeunit use the left hand side as the expected value in messaging about failures.

[sbergen]

Would it be possible to pull in the argument names, instead of 1, 2, 3 in this case?

error: Lock required for user
test/myapp/user_test.gleam:45

    assert lock_user(username, 10, role)

1. "lucystarfish"
2. 10
3. Admin

[lpil]

Argument names are not part of the public API of a function.

[sbergen]

Argument names are not part of the public API of a function.

How about labelled arguments?

[lpil]

What would you propose the exception data structure be in the presence of labels?

[sbergen]

I don't really know enough about the internals of Gleam to gauge the feasibility of this at any level, but in addition to value and kind could there be an optional label in the expression map?

[GearsDatapacks]

I think this is a very good idea, and the design is pretty near what we will eventually implement. I've left a couple of small notes to potentially consider inline.

[GearsDatapacks]

We have now added a custom panic message for let assert, so it might be worth rewording this paragraph with that in mind

[GearsDatapacks]

Does it make sense to include the original source code of the expression in here? So we can display stuff like age + 1: 27
I guess it may not be necessary if the entire asserted expression is displayed above, but something to potentially consider.

[lpil]

I don't think the code would be ideal as it can be arbitrarily large, which could result in code size bloating.

It could have byte indexes perhaps? Currently it contains the source line. That with a parser would be enough to get the source code, but what's best I am unsure. Perhaps my suggestion below of including the actual code isn't ideal.

Reading this again I don't understand why there's kind: expression | literal. What does the programmer do with this information?

[GearsDatapacks]

These tables seem to be written from the perspective of the representation on the Erlang target (e.g. Atom doesn't exist in JS). Is it worth clarifying how these will be represented on JS, or is it just assumed that we'll use the nearest JS equivalent of the Erlang-specific stuff?

[lpil]

Yup on JS it's the same but atom -> string