Kotlin: Support `assertThat(foo) { isNotNull() isNotEqualTo(bar) ... }`

[cpovirk]

It's already possible to write nearly that by using apply:

assertThat(foo).apply {
  isNotNull()
  isNotEqualTo(bar)
}

Might it be worth a shortcut? Anecdotes welcome.

[cpovirk]

I'm reliably informed that the implementation would be something like:

operator fun <T: Subject> T.invoke(assertions: T.() -> Unit): Unit = this.assertions()

[JakeWharton]

Yep, that'll work!

I also flip the parameter to a receiver, but that requires one extension per subject type instead of the invoke trick.

[vRallev]

@JakeWharton Is this what you mean?

fun assertThat(string: String, assertions: StringSubject.() -> Unit) {
  assertThat(string).run { assertions() }
}

This would allow

assertThat("hello") {
  isNotEmpty()
  isEqualTo("hello")
}

[JakeWharton]

No. The parameter becomes a receiver of the extension.

fun String.assertThat(assertions: StringSubject.() -> Unit) = assertThat(this).assertions()

[vRallev]

Got it. Then it reads differently, though. But that's a matter of taste I guess.

[netdpb]

@JakeWharton Would it then read as follows? Does that look better or worse than the explicit assertThat("hello") call?

"hello".assertThat {
  isNotEmpty()
  isEqualTo("hello")
}

[JakeWharton]

Well the input is rarely so trivial.

getApi()
  .makeSomeCall()
  .performSomeAction()
  .assertThat {
    isNotEmpty()
    isEqualTo("hello")
  }

[cpovirk]

For ease of searching: I'm told that, in addition to apply, it's also possible to use let or with.

https://kotlinlang.org/docs/reference/scope-functions.html#apply https://kotlinlang.org/docs/reference/scope-functions.html#let https://kotlinlang.org/docs/reference/scope-functions.html#with

Of those existing options, it seems like apply is most sensible here, though there may be subtleties that I'm unaware of.

For purposes of this issue, though, the real choice is the one being discussed above between string.assertThat and assertThat(string).

[netdpb]

@JakeWharton sure. So I guess the question is which of these seems better, and why?

getApi().makeSomeCall().performSomeAction().assertThat {
  isNotEmpty()
  isEqualTo("hello")
}

or

assertThat(getApi().makeSomeCall().performSomeAction()) {
  isNotEmpty()
  isEqualTo("hello")
}

The latter establishes the context of assertion right at the start, but the former maybe allows you to attach the assertion more directly to the clause that names the value ("some action").

Which is more discoverable, harder to get wrong? Which seems more Kotlin-idiomatic?

[netdpb]

@cpovirk If you're talking about using a scope function on the result of calling assertThat(foo), then I think it has to be run, with, or apply. let and also would force you to refer to the subject as it, which I think is worse:

assertThat(foo).let {
  it.isNotEmpty()
  it.isEqualTo("foo")
}

apply would return the Subject itself, which may enable things we don't want to see, like

assertThat(foo).apply {
  isNotEmpty()
  isEqualTo("foo")
}.contains("f")

run and with return whatever the last expression statement in the block returns, which for most assertions is Unit/void, which seems more like what we want (the assertion is the whole statement).

Finally, the difference between run and with is the same as the difference we're talking about between assertThat(foo) { assertions } and foo.assertThat { assertions } (with is the former style and run the latter).

@JakeWharton please check me as I'm learning this stuff: did I get that right?

[cpovirk]

Interesting, thanks.

It sounds like run (and with) could enable the same kind of weirdness as apply, arguably only worse:

assertThat(foo).run {
  doesNotContain("foo")
  containsAtLeast("bar", "baz")
}.inOrder()

But maybe that's so weird that no one would do it :)

Probably the takeaway is that Unit is the best return type for writing our own API. And sure enough, that's the return type we've been using in our proposals :)

(The other takeaway might be that, although users have options already, it's not necessarily obvious which is best, especially given the (remote) possibility of weird usages. That might make this feature slightly more useful than I'd initially realized.)

[cpovirk]

I am interested in thoughts from Jake (and other Kotlin users) on what is most idiomatic, but I also want to highlight his point from earlier:

The string.assertThat approach "requires one extension per subject." That's not a problem for core Truth and even ProtoTruth, etc., since we can just add all those extension methods. It's also arguably not a problem for pure-Kotlin Truth Subject classes, since we can recommend that their authors define an extension method, at least once we write Kotlin instructions for Subject authors. But it feels a little unfortunate that most pure-Java custom Subject classes won't come with such extension methods. Of course users can always define their own, but it's a minor obstacle for users to confront -- especially because foo.assertThat will initially appear to "work"... but by returning a plain Subject instead of the expected FooSubject. (I guess this is really just the same problem that users always see when they need to import a new Subject.assertThat method, though?)

I don't think this rules out the foo.assertThat approach -- though I admit that I'm already slightly biased against it because users will already have to understand assertThat(foo), and I'm not sure foo.assertThat is "better enough" to justify 2 styles and the resulting debates over which is better for a given usage. But I don't feel that strongly, especially if foo.assertThat would be more idiomatic.

[netdpb]

It sounds like run (and with) could enable the same kind of weirdness as apply, arguably only worse:

assertThat(foo).run {
  doesNotContain("foo")
  containsAtLeast("bar", "baz")
}.inOrder()

But maybe that's so weird that no one would do it :)

Oh yes, in the case of chainable assertions.

[netdpb]

The string.assertThat approach "requires one extension per subject." That's not a problem for core Truth and even ProtoTruth, etc., since we can just add all those extension methods. It's also arguably not a problem for pure-Kotlin Truth Subject classes, since we can recommend that their authors define an extension method, at least once we write Kotlin instructions for Subject authors. But it feels a little unfortunate that most pure-Java custom Subject classes won't come with such extension methods. Of course users can always define their own, but it's a minor obstacle for users to confront -- especially because foo.assertThat will initially appear to "work"... but by returning a plain Subject instead of the expected FooSubject. (I guess this is really just the same problem that users always see when they need to import a new Subject.assertThat method, though?)

I think the problem of assertThat being available for custom subjects is the same whether it's assertThat(foo) or foo.assertThat.

[cpovirk]

I guess someone could try...

assertThat(foos).run {
  containsAllIn(x)
  containsAllIn(y)
}.inOrder()

...and expect inOrder to apply to both. But that still seems unlikely, and it remains mostly academic unless someone is proposing that our new method return something other than Unit.

[cpovirk]

I think the problem of assertThat being available for custom subjects is the same whether it's assertThat(foo) or foo.assertThat.

Yeah, I think I was overrating the difference -- probably because of that existing bias that I mentioned :)

There's perhaps still something to be said for the idea that assertThat(Foo) is at least nearly certain to exist (though you still have to import it), while Foo.assertThat might not always. But that's separate from the import question.

Hmm, and I wonder if foo.assertThat is actually easier to type in IntelliJ and other tools? It would depend on how helpful autocompletion is for the two cases. Sure enough: At least in the Kotlin Playground, I get autocompletion for calling fun Foo.assertThat(foo : Foo) but not for companion object { fun assertThat(foo : Foo) ... }. I'm sure that IntelliJ will autocomplete the former, too, so the only question is whether it can also help with the latter (well, at least in automatically adding the import after I type it).

Also: No one would be crazy enough to define Foo.assertThat inside Foo (rather than as an extension function), right? Inside Google, at least, I would expect our testonly enforcement to prevent it, but maybe someone would do it in the wild. The worst result of that is probably just extra dependencies, though (which people can always do by depending on Truth, anyway).

[cpovirk]

Oh: If we make assertThat a top-level function (instead of a method in a companion object), it might appear more widely in autocompletion.

[netdpb]

Thinking a little more about foo.assertThat. I think, if we were to do it, it should not take a subject-receiver lambda, but instead a subject-parameter lambda so that it reads better as an English phrase:

// awkward. "foo: assert that is empty"?
foo.assertThat {
  isEmpty()
  contains(1, 2)
}

// better. "foo: assert that it is empty"
foo.assertThat {
  it.isEmpty()
  it.contains(1, 2)
}

// also good (imho best): "assert that foo is empty"
assertThat(foo) {
  isEmpty()
  contains(1, 2)
}

[JakeWharton]

In general, extension methods are considered the most idiomatic in Kotlin due to their discoverability. You never need to know where a function lives, because the IDE will suggest all applicable ones on the target type.

This is also a boon when you already have one function imported. If you import the core assertThat there will be available overloads for a lot of common types. But if you're looking for specific assertions using an extension will still prompt both versions whereas with top-level functions you'll need to manually add an import to get the more specific version.

---

Historically I believe I've named my extensions using just assert instead of assertThat.

user.name.assert {
  isNotEmpty()
  isEqualTo("Jake")
}

I don't find the English argument as a super strong influencer because it basically mandates you have a named local. Once you replace it with a method call (especially one with arguments) the sentence immediately breaks apart.

val box = Box(2, 2, 2, Red)
val scene = buildScene {
  putShape(Point(10, 10, 10), box)
}
assertThat(scene.viewport(Point(0, 0, 0), Point(1, 1, 1))) {
  containsShape(box)
}

Now you have to start inserting words to try and contort it back to English: "Assert that scene [with a] viewport [from] point 0,0,0 [facing] point 1,1,1 contains shape box". This is because we don't try to maintain English readability in the regular API that we're interacting with. And while it may make sense to pull out a local for a single assertion, if I want to check multiple viewports in this test function I would need to start getting creating with names.

Is that measurably more readable than

scene.viewport(Point(0, 0, 0), Point(1, 1, 1)).assert {
  containsShape(box)
}

due to trying to maintain English API order? I actually have no opinion on the English-ness of the API, but the import argument above I think is my primary one.

[lowasser]

I have some concerns about making this method that discoverable. If it's an extension method, and your IDE always suggests it, then assert will come up in the autocomplete of all non-test code, where it really shouldn't ever come up. Worse, you might get several different overloads of assert, all coming up at once -- and since a is at the beginning of the alphabet, a lot of users will start seeing them first while writing non test code. That's a lot of autocomplete pollution.

That may depend somewhat on IDE configuration, but there are a lot of setups out there.

[JakeWharton]

That would mean Truth would have to be on your non-test classpath which seems like a larger problem. In a test context, though, you can invoke assertions on any object at any time and that seems like it's working as intended.

[cpovirk]

(Thanks, this is all very helpful!)

At least in my testing with our normal Google build setup, IntelliJ doesn't care that a Truth subject is on the test classpath only: As far as IntelliJ is concerned, there seems to be only one classpath for the prod and test code together.

Now, if I actually tried to run the test through the build system, then I would get an error for the missing dependency, since the prod target doesn't depend on the Truth subject. But as far as autocomplete goes, I still see assert as a suggestion.

That said:

• The assert suggestion wasn't the first suggestion for the class I tested with. (I already deleted the project, but I think it was ~5th?) I don't know if this is representative.

• IntelliJ might well be capable of making a distinction between test and non-test code for autocompletion purposes, but our internal setup might not be taking advantage of it.

• It looks like at least some Kotlin-specific assertion frameworks might be taking the foo.assert approach, for better or for worse. Here's a post that shows it for Kluent and Expekt.

[cpovirk]

One thing that I don't recall discussing yet here: We might be able to run all the assertions in the block, even if some of them fail, by accumulating their results Expect-style.

That said, users may be surprised by such behavior. And it can go sour if one of the assertion throws a straight-up exception (rather than calling into Truth's failure reporting) -- though we could catch it.

It could be especially weird if generalized for FailureStrategy implementations other than assert_.

Still, we should give this at least a passing thought.

[kevinb9n]

I'm comfortable with recommending apply for this, and with the original proposal, so this comment is only about the extension method.

I'd humbly suggest that Truth's target users should be mixed-language (J + K) projects. I know extension methods are kotliny, but I think the bar should be a bit higher for justifying a split Truth idiom across the two languages. Even if this means that pure-Kotlin projects have a reason to prefer some other library.

Truth being English-grammatical was one of its founding value propositions and I still believe in it. A family member just asked me "what's that new garage code again?" and stood ready to validate my answer. I could have responded,

"I claim that equals the 1234 the garage code." (JUnit) "I claim that the garage code is equal to 1234." (Truth) "The garage code I claim [that] is equal to 1234." (ext. method proposal)

IDEA can offer autocomplete suggestions that rewrite what you already typed (like list.for<complete> rewrites to a foreach loop), and assertions are common enough that would it actually be worth getting it to do that.

[Note: any of these garage assertions would of course have failed. I'm not dumb enough to use 1234 as my garage code! Who do you take me for? It's 5873.]

[netdpb]

I'm on the side against making the assertion an extension function on the thing being asserted about. I like the distinction between the assertion context and a normal "method" on an object.

But I'm coming around to the feeling that we may want to consider a slightly different approach to the "sentence" construction for multiple assertions on a single value. The apply form can lead to a disconnect between the object and the assertion:

assertThat(foo) {
  isNotNull()
  isNotEqualTo(bar)
  isSomeOtherThing()
}

"Assert that foo is condition" is great for one thing, but for multiple conditions I wonder if using "it" would be better:

assertAbout(foo).that {
  it.isNotNull()
  it.isNotEqualTo(bar)
  it.isSomeOtherThing()
}

Nice:

• It allows multiple fluent assertions about a single expression.
• It still reads as passable English: "Assert about foo that 1. it is not null; 2. it is not equal to bar".

Awkward:

• It's an overload of assertAbout, which already exists to select the domain without the object. Perhaps about(foo).assertThat avoids that, but it makes the sentence more awkward. Or maybe considering(foo).assertThat or something?

[cpovirk]

I don't think we'd want assertAbout(foo): It requires an additional entry point for every Subject subclass. (This is a concern I also have about foo.assertThat.)) Subject authors already have to define assertThat, and they already should define a way to get a Subject.Factory. So I wouldn't want to require them to define a third entry point. (To be fair, assertAbout is at least something that could be defined in pure Java, unlike the foo.assertThat approach, which needs to be defined in Kotlin.)

Given that, I suspect that any new methods are going to have to go on Subject. (I'm not 100% confident that that's the only way, but that's where I'd start.)

Overall, I'm still comfortable with the original proposal. But if we can do better, that's great. It just may be tricky to make the phrasing work, especially when we'd ideally want the result to be "better" than the standard let or apply.

To be fair, we just heard anecdotally from a former team member that new developers, perhaps especially those fresh out of school, typically have some trouble distinguishing let and apply -- as do I, so far :) Now that's still an obstacle that they'll have to clear eventually, but that's one way in which we can be "better" -- or a way in which we can be worse if we create yet another thing for them to learn :)

[netdpb]

Thinking further, I now think the right idiom is:

long.expression().let {
  assertThat(it).isNotNull()
  assertThat(it).isNotEqualTo(bar)
  assertThat(it).isSomeOtherThing()
}

That is very close to what I suggested above about using "it", and uses only standard Kotlin, requiring no Truth API changes at all.

There is a small risk of someone dangling an assertion modifier off the end of the let (e.g., foo.let { ... }.inOrder()), but that could be mitigated with a check that flags any method call on a Subject whose receiver is a scope function call.

[kevinb9n]

If Truth has no special feature for this, then I think most kotlin-familiar users will naturally do Chris's original recipe.

[netdpb]

Should we simply document those two strategies (apply and let)? Should we prefer one over the other?

I don't see a strong argument for a new API.

[cpovirk]

Still no plans here, but I wanted to mention something that I was reminded of in a couple other conversations: Whether we do something here or not, we will eventually want to think about the implications for static analysis (CheckReturnValue, TruthIncompatibleType, and others (probably mostly those checks that are specific to Truth)).