truth icon indicating copy to clipboard operation
truth copied to clipboard

Add a subject for CharSequence

Open vRallev opened this issue 5 years ago • 9 comments

Android mostly deals with CharSequence instead of String. Many of the StringSubject checks rely on the API of CharSequence and not String. Is there a reason that no subject for CharSequence exists? It feels like both subjects could share a lot of their implementation.

At the moment, it is necessary to call toString() on the sequence, what isn't that pretty.

    val sequence: CharSequence = ""
    assertThat(sequence.toString()).isEmpty()

vRallev avatar Apr 05 '19 20:04 vRallev

As a workaround, you could add:

public final class CharSequenceTruth {
  public static StringSubject assertThat(CharSequence actual) {
    return Truth.assertThat(actual.toString());
  }
}

That solves the cosmetic issue, albeit probably not exactly what you want.

ronshapiro avatar Apr 05 '19 20:04 ronshapiro

Yes, correct. This raises questions when different implementations are equal and could result in false positives. Right now it's forcing the caller to make this decision, what might be intended. In AssertJ-Android there are two different methods hasText(..) and hasTextString(..).

vRallev avatar Apr 05 '19 21:04 vRallev

Thanks, I didn't realize CharSequence was so common on Android. And in fact I do see a huge number of assertions on the result of toString() in Android code.

We had declined to add this once years ago, but we didn't have the Android context at the time. (Another concern seems to have been that StringSubject "should" extend both CharSequenceSubject and ComparableSubject, but we've been thinking lately about whether inheritance like this really buys us much, anyway, so we probably don't need inheritance at all.)

One complication is that we usually let isEqualTo call plain equals, which wouldn't do the right thing for CharSequence, anyway. Possibly we should go against that here.

This deserves more thought.

cpovirk avatar Apr 08 '19 21:04 cpovirk

An intermediate option is to give isEqualTo magic for converting a CharSequence to a String. (We don't usually want that magic on other methods, since calls like isSameInstanceAs and isInstanceOf really should operate on the original object.) But that's leaky, at least in its current form, which doesn't apply to isAnyOf, etc.

And that doesn't help with isEmpty(), contains(), and so forth.

Arguably it would be nice to make a decision here for 1.0, since we don't want to remove APIs after that, and this would let us remove StringSubject. But we could live with the wart of identical StringSubject and CharSequenceSubject classes if it came to that.

cpovirk avatar Apr 09 '19 20:04 cpovirk

We're still not sure that we'll do this, but we should think about it for 1.0.

cpovirk avatar May 14 '19 14:05 cpovirk

Various comments:

As always, one fear that we should motivate us is negative assertions: assertThat(charSequence).isNotEqualTo(otherCharSequence) can pass, even if the 2 objects have the same content. I'm going to run some tests internally to see if this is biting anyone.

I will also look to test how many assertions we see on charSequence.toString() -- looking both at all assertions on such values and only at isEqualTo+isNotEqualTo assertions.

I might even see if I can dig up data for how often users used to have assertThat(charSequence).isEqualTo(something) and had to switch to assertThat(charSequence.toString()).isEqualTo(something.toString()). (I think I did some similar research for the isSameAs -> isSameInstanceAs rename.)

Removing assertThat(String) in favor of assertThat(CharSequence) (if we wanted to go that far) is difficult to do compatibly because calls will continue to resolve to the former as long as it's present. I think I have found a trick, though, which I will keep in my back pocket: public static <S extends String & Impossible> StringSubject assertThat(S actual)

cpovirk avatar May 31 '19 22:05 cpovirk

My assertThat(charSequence).isNotEqualTo(otherCharSequence) change didn't turn up anything too scary.

It's pretty clear that we're not going to get to this for 1.0 at this point. That doesn't prevent us from adding such a subject in the future, even if it would make StringSubject largely redundant. (And maybe that's even OK, since users are logically going to expect a "StringSubject" to exist. (Along those lines, I sometimes wonder if users expect to find a "CollectionSubject" or "ListSubject," too.))

cpovirk avatar Jun 20 '19 19:06 cpovirk

One way that we could come at this is to introduce CharSequenceSubject but don't expose it through the normal Truth.assertThat and StandardSubjectBuilder.that methods. Then users could deliberately select it if they feel that it's the right behavior for them. (We could even make Subject.isEqualTo suggest that if given 2 CharSequence objects that have equal content but aren't considered the same by equals.)

cpovirk avatar Jun 21 '19 15:06 cpovirk

Possibly we should also talk to the AndroidX people: Maybe they'd want to put all their assertThat methods in a single AndroidXTruth class instead of (well, nowadays probably "in addition to") putting each in its own Subject class. Then, we could see if they think that comparing CharSequence objects for equality based on content is a reasonable default behavior in the world of Android. If so, they could include assertThat(CharSequence) in their class, from which many Android developers would get it automatically.

cpovirk avatar Jun 21 '19 15:06 cpovirk