Descendants of AbstractArraySubject have poor error reporting

[Alexander--]

The current code of PrimitiveByteArraySubject (as well as other primitive array subjects) has several major flaws:

1. The Throwable message is generated by concatenating string representation of arrays. If arrays are sufficiently long, those messages become unreadable and sometimes even cause OOM errors because of excessive length of resulting string;
2. There is no automatic check for array length, so one have to perform that check every time by calling hasLength. I believe, that isEqualTo on AbstractArraySubject should automatically perform that check;
3. The message shows array bodies, but does not tell which elements differ.

These are the changes I'd like to see:

1. Always put array contents after the rest of error message: even if IDE or something else truncates the message, the most informative part should still be visible;
2. If array lengths differ, the exception message should start from saying so. I am unable to fathom a situation when someone would like to know if arrays are equal but wouldn't care about their lengths;
3. If total length of arrays is sufficiently big (say, bigger than 200 elements), do not add their full string representation to the Throwable message. Instead override the printStackTrace so that information is printed to stream (you can even keep the same format while doing so). This improvement might also be relevant for ListSubject
4. Add the same difference-finding logic currently present in ListSubject to the descendants of AbstractArraySubject.

The resulting error message woukd like like this:

Arrays byte[99] and byte[100] differ at position 4 (5 more differences) *details about differences* [optional array contents]

[dimo414]

Overriding printStackTrace is an interesting idea, but I worry that would lead to a very confusing user-experience for any code that doesn't happen to enter that path. It'd also be good to implement such a change Truth-wide, rather than in the array classes alone.

In general I'm not sure how compelling the OOM case really is - it's always possible for an object's .toString() to end up constructing a massive string, there's nothing special about arrays in this regard.

[Alexander--]

@dimo414 well, by the same logic, the possibility of OOM is always around the corner, so there is no need to care about memory usage, is there?

The real problem is the extent of issue: by concatenating array members into a two-byte Java string, the storage space for storing each element of byte[] is multiplied x4 to x12 times. So an array, that took 512Mb of heap memory ends up producing a Throwable message of ~3Gb. That's nothing to sneer at, especially on Android.

OOM is not even the biggest concern. It is really about readability: by placing a huge blob in front of error message you are obfuscating the actually useful information. Because Truth currently puts the array contents before the diff, I wasn't even aware, that change №4 has already been implemented since 0.32 release, — all my arrays are so long, that small difference report at the end of message was completely lost (IntelliJ truncated messages; console printout was too long to manually inspect all of it).

[cpovirk]

I do expect us to do (1) and (4), hopefully this quarter. Possibly we'll do (2), as well; I'm a little worried that it might be noisy for IterableSubject, but of course you're only asking for it for the array subjects (though doing it only for arrays runs somewhat counter to (4), which would make IterableSubject and the array subjects more similar -- I've never been sure how similar it makes sense to make them). It's also possible that we'll do something in between (2) and (4) that includes a count of the missing and unexpected elements, which implicitly mentions the difference in length. But a direct mention would be more useful in some cases....

RE: (3), I'm hoping that we'll eventually abbreviate messages better than we do now. That may help. Then again, in the course of doing all the bookkeeping needed, we might well use up a bunch of extra space.... But hopefully we can do at least a little better:

• short-circuit that hypothetical work if the message is long
• maybe delay the computation until someone calls getMessage (and override printStackTrace to avoid calling getMessage)
• maybe avoid showing both hex and decimal values

[Alexander--]

@cpovirk

I'm a little worried that it might be noisy for IterableSubject, but of course you're only asking for it for the array subjects

Why do you think, that message like the one I provided would be noisy? In my opinion, it only makes developer's life easier by highlighting most important information first. If the array lengths do not differ, it might make sense to skip byte[99] and byte[99] part and make it

Arrays differ at position 4 (5 more differences) *details about differences* [optional array contents]

hopefully we can do at least a little better:...

I don't think, that adding full array contents to Throwable message should be done at all for huge arrays. The main reason why I sometimes use huge arrays in tests is to test robustness of memory management or to enable discovery of issues, that can not be highlighted by existing test cases, when full-blown fuzzing solution is an overkill. As such, those arrays are either filled with zeroes or contain some randomly generated/meaningless data. It is rather inconvenient to visually compare two randomly generated byte arrays with >200 elements each.

When the contents of huge array are omitted, the resulting message can be made to look like this:

Arrays byte[5] and byte[1024] differ at position 4 *details about differences*; array contents:  <1, -68, 110, 6, 0>, <array is too long, full contents are added to exception stack trace>

If omitting array contents may be undesirable (for compatibility reasons, or because a developer might not always have access to stack trace), that behavior can be made configurable via system property or environment variable.