New Rule: Detect when all{} should be used

[inktomi]

A new rule to suggest usages of all would be helpful.

Perhaps, given code like this, we'd get a warning to use all instead:

assertThat(complexType.skyIsBlue).isTrue()
assertThat(complexType.raining).isFalse()
assertThat(complexType.cake.yummy).isTrue()

I think this would be better written with all:

assertThat(complexType).all {
    prop(ComplexType::skyIsBlue).isTrue()
    prop(ComplexType::raining).isFalse()
    prop(ComplexType::cake).isInstanceOf(Cake::class).prop(Cake::yummy).isTrue()
}

In this case, with all we'd get the evaluations for raining and cake even if the sky isn't blue.

Maybe the rule could check for 3+ assertions on the same variable and then suggest using all instead?

[jzbrooks]

This makes sense to me on a syntactic level.
My only concern is there are semantic implications of suggesting assertion authors make this change.

Your first example without all will fail at the first assertion failure. Since all ensures that all assertions are executed, there could be a surprising behavior difference there if one of the assertion transformations is particularly expensive.

Maybe that's fine. What do you think?

[inktomi]

Am I correct in my understanding that the runtime complexity of the examples (assuming everything runs and is passing) is the same? If so, I think it's totally fine to run everything in an error case and produce a better, more accurate, failure message.

eg, using all does not in and of itself add time to the test, other than that all the assertions within it would run - which is sort of the whole reason for all existing?

[jzbrooks]

Yes, but it wouldn't be correct to compel assertk users to always use .all on two grounds:

1. The assertk API allows for fast fail assertions (sequential assertions) and grouped assertions (via all)
2. The assertions could add extra runtime to the test, which could be avoided by serial assertions (say if one of the property reads is a computed property or if someone uses a complicated transformation on a property via transform.

I like the idea of advertising all, because people may not know about it, but I'm not sure a warning is the right level. A check that logs an informational lint issue instead of a warning would be less severe. It would also mean that we could probably avoid an arbitrary threshold for the check and raise the issue anytime more than one assertThat calls are made sequentially with the same subject or in the case of property reads off an object, the first object in the expression.

Particular projects who care about it more could always increase the severity.

[jzbrooks]

willowtreeapps/assertk#544