assertk icon indicating copy to clipboard operation
assertk copied to clipboard

Relax bounds of built-in value-asserting functions.

Open JakeWharton opened this issue 1 year ago • 5 comments
trafficstars

I'm migrating a couple thousand test cases from Truth, and another thing that is coming up similar to https://github.com/willowtreeapps/assertk/issues/393#issuecomment-2428206350 is that calls to isTrue() need to become isNotNull().isTrue() on nullable subjects. In general this makes sense! However, the fact that I can call .isEqualTo(true) without the isNotNull() check just feels bad. It's shorter! Why wouldn't I always then do that?

I fully understand and am justifying calls to isNotNull() before things like startsWith() which operate on the actual value and therefore need to null-check it first. But isTrue() and isFalse() seem like they should be equivalent to isEqualTo(true) and isEqualTo(false), just maybe with a better message. That makes me think they should operate on Boolean? and not just Boolean.

JakeWharton avatar Oct 22 '24 16:10 JakeWharton

The reason isNotNull() exists is because otherwise each assertion would have to make a decision on how to handle nulls. This ends up complicating every single implementation so it's convenient to have the logic in one place instead. isEqualTo() is a special case in that it's more flexible on what it can take (nulls & different types) because it's the most heavily used and what developers are familiar with. It also mirrors override fun equals(other: Any?): Boolean in java/kotlin.

I'm not against having another way of thinking about this that includes more assertion methods but it should still be something reasonably easy to understand so you aren't left guessing when to call isNotNull() and when not too.

evant avatar Oct 22 '24 20:10 evant

The reason isNotNull() exists is because otherwise each assertion would have to make a decision on how to handle nulls.

Assertions that operate on actual values are defined on the non-null variant of that type. It makes perfect sense that I cannot call startsWith() on an Assert<String?>. Similarly, I cannot call message() on a Assert<Throwable?>. I fully support and endorse this behavior. You will hear no complains about needing to migrate message().startsWith() to message().isNotNull().startsWith(). It is more verbose, yes, but the former conflates the presence check with an operation on the underlying value. The latter splits those two into distinct operations.

However, isTrue and isFalse do not operate on the actual underlying value in the same way that a startsWith does. Instead, they are shorthands for the comparison isEqualTo(true) and isEqualTo(false). They reduce verbosity by elevating common type values (the only two values, in this case) into the function name rather than the argument list.

If they are merely shorthands, then they only work half of the time. The other half they're longhands. isNotNull().isTrue() is longer than isEqualTo(true). But here the explicit presence check is redundant with the comparison operation. It doesn't tell me anything new.

The language mirrors this distinction:

  • Given val a: String?, I simply cannot call a.startsWith without some form of explicit presence detection (null check, safe call, etc.).
  • Given val a: Boolean?, I'm allowed to do == true. (In fact, the IntelliJ even directs me towards this rather than performing explicit null checks)

JakeWharton avatar Oct 23 '24 02:10 JakeWharton

So based on this and your other comment you are suggesting isTrue(), isFalse(), and isInstanceOf handle nulls? Any others?

evant avatar Oct 23 '24 17:10 evant

Sounds like you want the heuristic to be "assertions that use is or == because those operators support null themselves", does that sound correct? I could get behind that.

evant avatar Oct 23 '24 17:10 evant

Yes. That is a fantastically succinct framing of it.

JakeWharton avatar Oct 23 '24 18:10 JakeWharton