Grouping assertions dynamically

[bnorm]

The Expect TestVerb is great for short JUnit tests but I write non-JUnit system tests that run for hours. I would like to use Truth for all the assertion checks I make throughout my tests but I want a way to be able to group some checks together so I get as much information at the point of failure while still stopping the test at the point of failure.

Sort of the design I'm thinking is this

TestVerb testVerb = ...;
try (GroupVerb groupVerb = GroupVerb.from(testVerb)) {
    FooSubject subject = groupVerb.about(foos()).that(foo);
    subject.hasProperty1();
    subject.hasProperty2();
    subject.hasProperty3();
}

Checks made with a GroupVerb are gathered like with Expect and when GroupVerb is "closed," the group failures are packaged like Expect and sent to the failure strategy of the wrapped test verb. I could use this to check multiple properties on the same subject or even iterate a list of subjects, checking them all.

I have a working prototype for GroupVerb but it requires being in the com.google.common.truth package because getFailureStrategy() on AbstractVerb is protected. I thought this might be a good feature for the library though.

[kluever]

This is an interesting idea... /cc @cpovirk @PeteGillinGoogle

[jbduncan]

This sounds rather similar to JUnit 5's assertAll assertion to me.

[PeteGillin]

Yeah, this is something that I've wanted before. I think it's useful even for JUnit tests, because it's more targeted than Expect (you can say "run all of this logical group of assertions, but don't run this second group which would be meaningless if the first group fails"... the failure messages of Expect are already kind of awful a lot of the time, and adding in noise from all the later tests you don't care about can make them much worse — this has often been enough to steer me away from using Expect, and just doing some debugging to figure out which of the other tests pass when the first fails).

After reading your proposal, I was going to float an alternative, adding a method like Subject.satisfiesAllOf(Assertion<? super S>... assertions) where Assertion is a functional interface a bit like a throwing Consumer. Example usage:

assertThat(5).satisfiesAllOf(
    i -> i.isGreaterThan(4),
    i -> i.isLessThan(6));

This supports other test verbs and custom subjects in the natural way, so the equivalent of your example would be like this:

testVerb.about(foos()).that(foo).satisfiesAllOf(
    subject -> subject.hasProperty1(),
    subject -> subject.hasProperty2(),
    subject -> subject.hasProperty3());

(The implementation of satisfiesAllOf would have to do an unchecked cast of 'this' to S, because Generics, but we already have precedent for that.)

However, I suspect that the presence of assertAll in JUnit5 (thanks for the pointer, @jbduncan) makes this a lot less appealing. With Truth and JUnit5 as they currently stand, you could write my example as:

assertAll(
    () -> assertThat(5).isGreaterThan(4),
    () -> assertThat(5).isLessThan(6));

and your example as:

assertAll(
    () -> testVerb.about(foos()).that(foo).hasProperty1(),
    () -> testVerb.about(foos()).that(foo).hasProperty2(),
    () -> testVerb.about(foos()).that(foo).hasProperty3());

Comparing your proposal, my proposal, and what's available for free once JUnit5 lands, what are the arguments in favour of doing something in Truth?

• We can get a marginally neater API (less repetition).
• It's available to people who aren't using JUnit (which is, I suspect, a tiny fraction of Truth users, sorry!).
• It opens the door to functionality like satisfiesAnyOf in the future, if we want.
• In my proposal (although not in yours) we would get to catch all Exceptions. JUnit decided to have assertAll only catch AssertionErrors, any other kind of exception kills it straight away. I'm not sure whether we'd agree with them or choose to do the pokemon thing.

I'm not sure how compelling those arguments are. I'd love to know what @kluever and @cpovirk think — and you, @bnorm.

(JUnit5 API for reference: http://junit.org/junit5/docs/current/api/org/junit/jupiter/api/Assertions.html)

[PeteGillin]

Incidentally, @bnorm, can you say anything more about the difficulties you're having with your own implementation of GroupVerb? I don't see right now why the visibility of AbstractVerb.getFailureStrategy() is an issue for you. I think this works:

  public static class GroupVerb extends AbstractVerb implements AutoCloseable {
    
    private final AbstractVerb delegate;

    public GroupVerb(AbstractVerb delegate) {
      super(new GroupFailureStrategy(delegate.getFailureStrategy()));
      this.delegate = delegate;
    }

    @Override
    public AbstractVerb withFailureMessage(String failureMessage) {
      return new GroupVerb(delegate.withFailureMessage(failureMessage));
    }

    @Override
    public AbstractVerb withFailureMessage(String format, Object... args) {
      return new GroupVerb(delegate.withFailureMessage(format, args));
    }

    @Override
    public void close() throws Exception {
      ((GroupFailureStrategy) getFailureStrategy()).executeDeferredFailures();
    }
  }

and then you just need to implement GroupFailureStrategy which overrides fail(String, Throwable) to capture the failure and implements executeDeferredFailures to delegate to the FailureStrategy passed into the constructor (somehow rolling up all of the captured failures).

[bnorm]

That is exactly the working prototype I have. But if GroupVerb is in any package other than com.google.common.truth, the call to getFailureStrategy() in the constructor fails to compile because it cannot access the method via AbstractVerb. Including such a class in my project doesn't sit well with me; seems like a bad code smell.

I do like the flow of satisfiesAllOf, since one doesn't need to worry about closing anything. However, it means I can only check a single subject. One of the things I liked about the GroupVerb design is I can check multiple subjects, and even different types.

[PeteGillin]

Oops, I had totally failed to spot that my prototype only worked because I'd tried it in the same package :-). Sorry about that.

It's kind of annoying. If it weren't for the fact that you need to implement the (String, Throwable) overload of FailureStrategy.fail, you could do it by delegating to the AbstractVerb's fail method, which you do have access to. It seems like a bit of an oversight that AbstractVerb has methods which forward to only 2 of the 3 overloads on FailureStrategy. However, the very existence of that overload is a bit weird — as far as I can tell, it's never used within Truth itself with a non-null Throwable. There are a handful of callers elsewhere in the Google codebase (from within Subject subclasses, where you do have access to the FailureStrategy) but it's against the Truth idiom.

So, in practical terms, I think you could implement GroupFailureStrategy by having fail(String) delegate to the original AbstractVerb (which you do have access to) and having fail(String, Throwable) either throw an UnsupportedOperationException; or you could do it by having fail(String, Throwable) ignore the Throwable and delegate to the AbstractVerb... Unless you're using a custom Subject which calls the fail(String, Throwable) overload, you're not going to get any problems.

(I agree this isn't ideal.)

[PeteGillin]

But we're digressing somewhat...

You make a reasonable point about my proposal being restricted to assertions about one subject, whereas your proposal is more general at the expense of being more verbose. I think there'd be a useful discussion to be had — but only if we've convinced ourselves that either of them is worth doing given the imminent arrival of assertAll.

[cpovirk]

I don't have an intuitive feel for how often this comes up, so I should probably give it some more thought. At the moment, we're pondering some changes to the failure APIs, TestVerb, and other "internal" parts of Truth. They may end up resolving your package-private problem, though I'm not sure. In any case, it will be a while before I personally can give this attention, but thanks for raising it, and hopefully we will still get to it eventually.

[dimo414]

I like this idea too, though I agree with Chris that I don't think it's likely to come up in practice all that often. I worry that it could encourage people to write overly-complex test methods, whereas Expect has a nice side effect of prompting users in the direction of smaller test methods.

One question; can we think of a better name than "GroupVerb"? The name should ideally (a) read naturally like assertThat(), (b) clearly describe what the verb is doing and possibly (c) discourage misuse.