Expand and externalize our docs on writing your own Subject

[cpovirk]

We have some docs internally. Other things for us to cover:

• Don't throw AssertionError directly: Call this.fail():
  • fail() generates the appropriate type of failure (e.g., AssumptionViolatedException when someone uses assume().
  • fail() uses the user's failure message and subject name.
  • fail() has message to disambiguate objects with the same toString() but different classes.
  • fail() comes in overloads that help you construct the message.
• Similarly, don't delegate sub-object assertions to assertThat(...): Delegate to check().that(...). (But note bug #199.) This preserves the failureStrategy.
• Once you call fail(), don't do anything else. If someone is using expect() (or some custom FailureStrategy that logs failures -- I think Google has at least one), fail() won't throw, so execution will continue.
  • This could result in multiple separate failure reports ("size is different," "missing element a," "missing element b"). (Sometimes that could be a good thing, but if it's a good thing, then we should do it for assert(), too, by putting everything in one fail() call!)
  • Worse, it could result in exceptions: If we fail() if the sizes of two collections differ but then continue to iterate over them in parallel, we're likely to throw NoSuchElementException. This defeats the purpose of expect() and might(?) obscure the real error.
  • We could maybe "solve" the former problem by wrapping the FailureStrategy in some kind of failOnlyTheFirstTimeStrategy, but this doesn't help with the second problem.
  • Related question: What happens with expect() and chained assertions like containsExactly().inOrder()? I guess that we need to return the "inOrder() always succeeds" object, which we should maybe stop calling IN_ORDER and documenting as though it means that. It's possible that the failOnlyTheFirstTimeStrategy approach could be useful here, but I'm wary of it.
• Distinguish between "check failed" and "bug in your test: check is bogus" (issue #207).
• Use failComparing() when substring/subsequence matches are helpful to highlight.
• [If we ever require you to override named() (issue #202), we should document that.]

[cpovirk]

• How to expose your SubjectFactory: FooSubject.factory()? FooSubject.foo()? FooSubject.foos()? And/or do you expose your own assertThat method, which can be static imported alongside other overloads so long as they don't overlap (like if you wrote your own StringSubject) or conflict (like if you created a SerializableSubject that could apply to, say, ImmutableList as well as `IterableSubject could). (We've had a little discussion on this internally but haven't reached a conclusion.)

[cpovirk]

• Anything necessary for your subject to work with thatEach? Or, as we've discussed from time to time, we could remove thatEach or substantially change how it works.

[cgruber]

I'm taking the assignment, and will start to do the harmonization/sync infrastructure work, and when I'm done, I'll flip it to @kluever to massage the content.

[cpovirk]

• At some point, we should decide whether to recommend that all assertion methods be named for a singular subject (hasValues) or whether some can be plural (haveValues). I'm still a fan of singular, since things like isEqualTo will always be around, but someone (Kevin?) felt pretty strongly in the other direction.

[cpovirk]

• If you provide a chaining API for asserting about subobjects, how should you name the chaining methods? We have MultimapSubject.valuesForKey... so probably like that?

[cpovirk]

• Discourage people from saying (for example) "AtomicLongMap is basically a Map, so let's either (a) have assertThat(atomicLongMap) return a MapSubject or (b) make AtomicLongMapSubject extend MapSubject."

Both produce weird behavior for most of the methods inherited from Subject:

• isAnyOf
• isEqualTo
• isInstanceOf
• isNoneOf
• isNotIn
• isNotInstanceOf
• isNotEqualTo
• isNotNull
• isNotSameAs
• isNull
• isSameAs

We could conceivably override those. We'd also need to put in special logic for null, something like:

return atomicMap == null 
    ? new AtomicLongMapSubject(null, null) 
    ? new AtomicLongMapSubject(atomicMap.asMap(), atomicMap); 

But at that point, we might as well just inherit only from Subject and delegate our implementations to an internal MapSubject where helpful.

[cpovirk]

• Document the unchecked cast necessary if you want to extend IterableSubject and implement a corresponding SubjectFactory. Or make the cast unnecessary by zigging or zagging with named(). Or make it all code generated, as Christian has discusses.

[cpovirk]

RE: SubjectFactory naming (https://github.com/google/truth/issues/208#issuecomment-153868330):

I might prefer singular names:

• assertAbout(optionals()) sounds a little weird to me. (This might be unique to Optional, which I guess I still don't fully see as a sensible noun. But likely I'll get there someday :))
• "optional()" seems easier to work into other possible APIs:

Compare:

assertThatEvery(optional()).in(list).isPresent();
assertThatSome(optional()).in(list).isPresent();

vs.

assertThatAll(optionals()).in(list).isPresent(); // "all... is"
assertThatAtLeastOneOfThe(optionals()).in(list).isPresent(); // yuck

On the other hand, I sort of prefer the sound of "assertAbout(strings())" to "assertAbout(string())." But I do think that the latter is probably clearer: You're asserting only about one string.

[cpovirk]

In favor of plural names: "string()" sounds kind of weird by itself -- like it's a specific string? -- whereas "strings()" sounds more like it's about the type of strings in general.

[cpovirk]

Slightly more reasonable but still kind of messy:

assertThatEachOfThe(strings())... assertThatOneOfThe(strings())...

[cpovirk]

@netdpb suggests that we could use a different order to make plurals work with iteration:

assertAbout(strings()).thatEachOf(...).... assertAbout(strings()).thatAnyOf(...)....

[cpovirk]

I suppose that we could try to go even further:

assertAboutStringsThat

That requires users to know about only one method (rather than both assertAbout and strings()). But it starts to sound like a mouthful for a single method. Hmm, and I guess it cuts off the possibility of assertAbout(optional()).withMessage(...).that(...) (though users can still do assertAbout(optional()).that(...).withMessage(...)). Maybe we still want assertAbout... I guess for roughly the same reason that we need assert_ (though I forget what that is)?

[cgruber]

Related, we should recommend against subclassing common Subjects like IterableSubject. Generally, if you want your type to expose iterables-like propositions, you can always get an Iterable view of your getSubject() contents and call check().that(iterable).blahblahblah(). We didn't especially carefully design these subjects for extension.

[cgruber]

We're now syncing docs internally and externally, so I'll work something up here.

[cpovirk]

RE: https://github.com/google/truth/issues/208#issuecomment-223094859

We just had an internal discussion in which we were about ready to say, "Sure, assertThat(Stream) can return an IterableSubject." Then @kluever pointed out that tests like this would fail:

assertThat(stream).isInstanceOf(Stream.class);
assertThat(stream).isEqualTo(stream); // (not that we encourage people to test equals() in this way)

[cpovirk]

Maybe the formal way of stating that is that actual() must return the object you passed to assertThat. Exceptions:

• Chaining cases: assertThat(multimap).valuesForKey() returns a subject with those values as the actual().
• (maybe?) Names other than "assertThat": assertThatStringResultOf(future) could return a StringSubject because the method name makes this clear. So, for example, no one expects assertThatStringResultOf(future).isInstanceOf(Future.class) to pass.

[cpovirk]

some philosophy on chaining: https://github.com/google/truth/issues/254#issuecomment-313763906

Internally, we have a doc entitled "Truth Chaining Thoughts"

[nick-someone]

https://truth.dev/extension#writing-your-own-custom-subject is what we have now.