Allow custom assertions on the value of an Optional

[alice-ks]

When doing assertions on an Optional containing a simple type like a string, the current OptionalSubject#hasValue method is sufficient. However, when the Optional contains a custom type which might even have nested inner types, doing a simple equality check isn't always the desired way. Users will typically end up with code like this:

assertThat(person).isPresent();
Address address = person.getAddress();
assertThat(address.getStreet()).startsWith("D");

I suggest to add an additional assertThat() method to OptionalSubject which takes a function as second argument. This function maps the value of the Optional to an appropriate Subject. To make it properly work, OptionalSubject needs to be generified (which shouldn't break existing code). As result, the previous assertion could be written like:

assertThat(person, PersonSubject::assertThat).value().address().street().startsWith("D");

Or with a bit more verbose naming:

assertThat(person, PersonSubject::assertThat).hasValueThat().hasAddressThat().hasStreetThat().startsWith("D");

I intend to provide a pull request for this soon.

Does anybody have an opinion about the name of the method which allows to access the value (e.g. "value" or "hasValueThat"?

[cpovirk]

We have so far encouraged users to write...

assertThat(person.get().getAddress().getStreet()).startsWith("D");

...which is almost as good, except the failure message is worse, and it always throws, even if you're using something like Expect.

It's possible that we might someday want to support...

assertThat(person).value(/*SubjectFactory*/ people()).address().street().startsWith("d");

...but we're still working out some best practices around subject chaining.

[alice-ks]

I realized that I forget the get() method call in my example. It should have been:

assertThat(person).isPresent();
Address address = person.get().getAddress();
assertThat(address.getStreet()).startsWith("D");

If I abbreviated it to what you suggested, it would be:

assertThat(person.get().getAddress().getStreet()).startsWith("D");

... which will throw an ugly exception if the person Optional doesn't contain a value. In addition, static code analyzers might complain about it.

I used Hamcrest on a previous project and liked the way you can combine matchers. When I started working on my current project, which uses Truth, I looked for a way to do something similar. I remember to have read a note in your documentation about subject chaining some months ago. That's why I started our own set of custom, chained subjects, which works pretty well and allows to write concise tests with good failure messages. I also started our own variant of OptionalSubject which supports subject chaining. My hope was that I could get rid of this special class by adding that feature to the original OptionalSubject of Truth.

Does it make sense that I provide a pull request for it?

Are there any discussions I could join regarding subject chaining?

[cpovirk]

@dimo414, who's looked the most into chained subjects, has taken the position that chained subjects are useful mainly when they let us provide richer failure messages. Here, we might be able to slightly improve the failure someday, but currently I think it's mostly going to look like "expected value to be present but was not," whether it's the IllegalStateException from get() or the AssertionError from value().

We may still have more to work out here, though.

[dimo414]

I confess, your first example (with two separate assertions) reads much more nicely to me. I appreciate that there's something elegant and appealing about chaining a sequence of assertions together, but in many of the examples I've seen the result is more complex to work with - either for users, Truth internals, or both.

As Chris said, for the specific case of Optional values I've been advocating for simply calling .get() directly and skipping the .isPresent() check, as .get() on an absent optional will fail just as well as OptionalSubject.isPresent() (and you can always add back the .isPresent() if you change your mind), so I would suggest writing your example as simply*:

assertThat(person.get().getAddress().getStreet()).startsWith("D");

To me this reads more cleanly and is more intuitive than the proposed chaining syntax (and avoids adding a decent amount of complexity to Truth's user-facing surface). I'd like to see evidence that a chaining assertion improves the resulting error messages or avoids some error-prone usage before considering adding that sort of complexity.

* I think you could make a case for chaining off of an AddressSubject type, so that you retain the context of the full address when asserting about the street, but that's a separate question.