New entrypoint: `assertFailure`

[JakeWharton]

I'm finally getting around to migrating a bunch of assertions and I find this to be a frequent enough pattern to warrant its own entrypoint.

First, consider the very common pattern assertThat(foo.bar()).isEqualTo("bar"). This line is implicitly asserting that foo.bar() is successful and that the return value is "bar".

Now another way to write this is:

assertThat { foo.bar() }
  .isSuccess()
  .isEqualTo("bar")

But that's so verbose! I always expect the function call to succeed. I just want to assert on the value.

Now invert the problem: I want to test a function which I expect to always fail.

assertThat { foo.bar() }
  .isFailure()
  .hasMessage("You're not 21!")

Even worse, in the case of functions that fail they usually fail in with particular exception types.

assertThat { foo.bar() }
  .isFailure()
  .isInstanceOf(UnderageException::classs)
  .hasMessage("You're not 21!")

The kotlin.test APIs have assertFails and assertFailsWith functions for these two cases. These are entrypoints where you are invoking a function whose result is failure and whose result is failure of a particular type.

assertFails { foo.bar() } // Assert<Throwable>
  .hasMessage("You're not 21!")

assertFailsWith<UnderageException> { foo.bar() } // Assert<UnderageException>
  .hasMessage("You're not 21!")

I believe kotlin.test also supports supplying a string message to match on, but in my opinion that's going too far and that's where the normal assertion APIs thrive (not to mention validating causes, other properties, etc.).

What do you think?

[evant]

Hm, what about instead of an additional entrypoint we add convenience assertions on results?

assertThat { foo.bar() }
   .isFailureWith(UnderageException::class)

assertThat { foo.bar() }
   .isFailureWithMessage("You're not 21!")

[JakeWharton]

For me, the primary issue is having to trampoline through Assert<Result<T>> in order to get to Assert<Throwable>. I'm perfectly happy using isInstanceOf and hasMessage to validate properties of the exception.

The success case benefits from being the default execution mechanism of the dot operator (if you can even conceptualize such a thing). If there were a special operator which let me invoke a function in a way that returned a Throwable and failed if it returned normally I would use that with assertThat(..). Inverted dot operator?

assertThat(foo˙bar()).hasMessage("You're not 21!")

Of course, such a thing doesn't exist.

Would you go for just?

fun <T : Throwable> assertFailure(body: () -> Any?): Assert<Throwable>

I think assertThat(body: () -> Result<T>) is a nice, general-purpose function for APIs which return Result, but if I'm not working with them otherwise I would prefer to never see it in the same way I don't see it with successful value assertion.

[evant]

Yeah that does make some sense. My specific concern with assertFailsWith<UnderageException> is that it moves the assertion itself to the entrypoint which is different from every other kind of assertion and backwards from the 'normal' api. This still somewhat happens with assertFails too, unlike assertThat it actually does something by itself. Maybe that's fine, though a common problem I've seen is people doing assertThat without an assertion on it and I wonder if this'll add more to that confusion.

[JakeWharton]

I'm definitely sympathetic to the case of the fact that it performs an assertion as part of its setup.

That's why I tried to frame my argument in contrast to assertThat(foo.bar()) which also contains an assertion of success. Of course that argument is somewhat weak since the default way an exception-based language works is under the premise that everything is literally an implicit assertion of success.

The best argument may simply be that it's a pragmatic middle-ground. You especially see the verbosity when comparing successful tests against failure-expecting tests. In my case it was:

assertThat(FqName.bestGuess("Map.Entry"))
  .isEqualTo(FqName(listOf("", "Map", "Entry")))

assertThat { FqName.bestGuess("Map.entry") }
  .isFailure()
  .isInstanceOf(IllegalArgumentException::class)
  .hasMessage("Couldn't guess: Map.entry")

There were about 10 positive cases and then 10 failure cases right next to each which really illustrated the different in verbosity.

With this it would be

assertThat(FqName.bestGuess("Map.Entry"))
  .isEqualTo(FqName(listOf("", "Map", "Entry")))

assertFailure { FqName.bestGuess("Map.entry") }
  .isInstanceOf(IllegalArgumentException::class)
  .hasMessage("Couldn't guess: Map.entry")

and with #454 it'd be:

assertThat(FqName.bestGuess("Map.Entry"))
  .isEqualTo(FqName(listOf("", "Map", "Entry")))

assertFailure { FqName.bestGuess("Map.entry") }
  .isInstanceOf<IllegalArgumentException>()
  .hasMessage("Couldn't guess: Map.entry")

which looks much better visually, to me.

I'm perfectly happy dropping assertFailsWith altogether from the suggestion.

[evant]

Ok yeah I think that's reasonable