Add eventually utility

[bdmendes]

Hi! As discussed in #1161, here's a proposal for eventually. Some things to think about:

• I have experimented with an effect agnostic version that did munitValueTransform(body) and returned a Future. However, that wouldn't work for sync assertions before the end of a test example, and wouldn't return composable effect types for the interested downstream effect adapter libraries.
• I don't think we have a way to sleep on JS for the synchronous case. Am I missing something?
• The type implementations catch all exceptions and retry, not only assertion failures. This makes it very simple to e.g. skip transient failures on a Future and only map on the happy value. I understand this is opinionated, let me know what you think (vs. only catching munit assertion failures).
• The stack trace on failures is very long (recursive retries). Should we clean it up and filter the retry calls?

I am open to all kinds of suggestions.

[tgodzik]

Looks like a sensible addition to me, but maybe we should ask for more opinions? I can raise it on Scala contributors and discord to get more eyes on it. What do you think?

[tgodzik]

The stack trace on failures is very long (recursive retries). Should we clean it up and filter the retry calls?

One way hand yes, but I find tinkering with stack trace might cause more issues and is a bit magical.

[bdmendes]

I can raise it on Scala contributors and discord to get more eyes on it

Sure, let's try to find some common ground. Can you point me to the Scala Contributors thread/Discord where the discussion happens? I am happy to keep an eye on those and in this PR's thread.

[tgodzik]

I've posted on the general Scala Discord and on Scalameta one. I think ideally people would comment here.

I just wanted some additional opinions, since I mostly maintain munit in a minimal capacity 😓 (didn't work on the library to extensively)

[kitbellew]

• I have experimented with an effect agnostic version that did munitValueTransform(body) and returned a Future. However, that wouldn't work for sync assertions before the end of a test example, and wouldn't return composable effect types for the interested downstream effect adapter libraries.

i didn't understand this part.

I'm pretty sure everything we do in this library is a Future (or you wouldn't have had to add an await). which means, this approach is inconsistent and needs to be revised.

the user needs to assert inside the body, or worry about await themselves. you just need to avoid retrying failures due to failed assertions (this looks like skipping a particular exception type).

[bdmendes]

I'm pretty sure everything we do in this library is a Future

I am not getting your point here. munit assertions are by design "synchronous" and run wherever the user wants them to run. It is perfectly wise to do

test("do something"):
  assertEquals(1,1)
  assert(1 == 1)

And none of these return a Future. Behind the scenes, munit will try to transform the evaluated assert(1 == 1) through the value transforms, and here does nothing as it's Unit. It will do work when the return value is a Future, or something the user has provided how to convert to it.

The whole transformer mechanism is of course evident when

test("do something"):
  for
    v <- fut()
  yield assertEquals(v, 1)

But then again, the first example is also perfectly valid.

The goal of this proposal is for eventually to be an "effect transformer" that retries failed futures, as for instance https://github.com/cb372/cats-retry. This PR does await on Unit, as it takes Unit as an effect. What I was getting with

have experimented with an effect agnostic version that did munitValueTransform(body) and returned a Future

Is that if eventually is

def eventually[A](body: Any) = withRetries(() =>munitValueTransform(body))

Then we lose the capacity to easily retry synchronous code as the test examples in this PR show. Also, what framework integrations consider their preferred way to convert to the Future world may necessarily be their preferred way of retrying failures. This design allows them to do it inside of their effect world before the final Future conversion runs.

you just need to avoid retrying failures due to failed assertions

The goal of this is to retry failed assertions, to wait until something that we expect to happen eventually happens. Eg, something is listening in a Docker container port, etc. This is an alternative take of specs2 eventually. Of course, we can catch only assertion errors, instead of most exceptions, that's a decision choice.

Please, explain your point further.

[kitbellew]

Please, explain your point further.

i saw your await while reviewing on the phone and somehow interpreted as equivalent to Await.result.

[kitbellew]

@bdmendes @tgodzik i revised the change. the thing that bothered me the most had to do with retrying any exception, so i fixed that. now by default it only retries failed assertions, but this can be overridden. it can also be overridden per assertion or per block.

[bdmendes]

What's the point of this default? Doesn't this render eventually a noop?

[albertikm]

correct. there's no "reasonable" default, in my opinion.

[bdmendes]

In that case, why try to provide one? This makes eventually similar to identity, right? I think it's reasonable to require the user to provide retry settings, either via implicit or explicitly.

[kitbellew]

sounds good, let's do it this way. @tgodzik please chime in (since i revised it, i guess it's my implicit "ok".)

[bdmendes]

Thanks. This looks okay to me.

[tgodzik]

LGTM from me!