[HaveCountMatcher] Implement Shorter Failure Message

[Ben-G]

As discussed in #308 I took a shot at implementing a new failure message for the haveCount matcher.

I've added a new prettyCollectionType with two overloads to print nicer type information for Objective-C types. This way the failure message will mention NSArray instead of __NSArrayI.

Not sure if there's a better approach for this; or if you consider it useful at all. Would love to hear your feedback!

[briancroom]

@Ben-G Thanks a lot for taking the time to open this discussion and put the PR together! I think there can be a lot of value in these kinds of micro-optimizations, and the screenshots you posted in #308 sure do illustrate the issue well.

I do have some concern about simply dropping the mored detailed information altogether. When I'm looking at the console output of a failed CI run, I almost always wish for more data about the failure rather than less! 😀 I think I'd rather see Nimble move in the direction of having a shorter failure of the summary as the first line, and including extra details afterwards. A little experimentation seems to indicate that the inline failure highlights in the Xcode source editor only include the first line of the message, with the rest available either in the console log, or test build log.

What if we were to introduce the concept of "extra details" to the FailureMessage type which would be appended on a separate line when recording the failure. The haveCount message could then look something like:

expected to have Array<Int> with count 1, got 3
Actual value: [1, 2, 3]

[Ben-G]

@briancroom You're bringing up a good point!

I touched on this third option in #308 as well. We could conserve the details but simply move them to the end of the message so that we get the quick lookup in the source editor + additional detail in CI and other scenarios.

What if we were to introduce the concept of "extra details" to the FailureMessage type which would be appended on a separate line when recording the failure. The haveCount message could then look something like:
expected to have Array with count 1, got 3
Actual value: [1, 2, 3]

Intuitively this sounds great! But would love to hear other's opinions on this.

[briancroom]

Calling all @Quick/contributors 😀

[ashfurrow]

The compromise of moving details to the end sounds good to me, 👍

[Ben-G]

@briancroom @ashfurrow I just took a look at this again. There's code in place that trims whitespace characters and newlines (https://github.com/Quick/Nimble/blob/master/Sources/Nimble/FailureMessage.swift#L36-L41) which makes it difficult to implement this is as a small change.

I can set the postfixActual value on FailureMessage but because of the lack of newlines / spacing the formatting is off.

Seems like more changes would be required to support this. Probably not worth it? What do you think?

[ashfurrow]

@Ben-G what does the message look like, in terms of poor formatting?

[Ben-G]

@ashfurrow Ah, once again lacking a screenshot. Here we go:

[ashfurrow]

That's a tricky one, a bit outside my wheelhouse I'm afraid.

[Ben-G]

@ashfurrow The blame information on https://github.com/Quick/Nimble/blob/master/Sources/Nimble/FailureMessage.swift#L36-L41suggests that @briancroom might now why whitespaces / newlines are currently removed and if it is possible/desirable to change that.

Thanks a lot for trying to help! 👍

[briancroom]

I don't have a lot of context on the motivation for stripping newlines, but I'd wager that it is to help prevent failure messages from bloating too much when they contain descriptions of expected and actual values which are formatted to take a lot of vertical space. I've found it's easy to lose track of the failure message itself when the value descriptions are too overwhelming.

My thought here is that we should introduce a new optional String property on FailureMessage (extendedMessage or similar, perhaps?), which would get appended to the final string after a newline. How does that sound?

[Ben-G]

@briancroom sounds good to me!

[Ben-G]

@briancroom I rewrote the the commit based on your latest suggestion. Let me know what you think of the result!

[briancroom]

I feel pretty good about this! Thanks for sticking with it @Ben-G

The Linux CI failure is because NSHashTable isn't available in Corelibs-Foundation. Could you wrap that entry in the switch with a #if _runtime(_ObjC) conditional?

[Ben-G]

@briancroom updated and linux build is passing now!

Unfortunately I need to move the runtime check out of the switch statement to avoid a compile error.

[briancroom]

Excellent! Having seen no dissent to this approach in the last days, I'm really happy to merge this! Thanks again for this well thought-through contribution @Ben-G