RFR(L): 8229186: Improve error messages for TestStringIntrinsics failures

[Igor Ignatyev]

Hi Evgeny,

in general, I like the idea, yet I feel it would be more useful if ArrayDiff is also used by Assert class, do you plan to do that by a separate RFE? I also have a number of comments regarding the patch:

test/hotspot/jtreg/compiler/intrinsics/string/TestStringIntrinsics.java:
- I'd prefer invokeAndCompareArrays and invokeAndCheck to be as close as possible: have both of them to accept either boolean or Object as 2nd arg; print/throw the same error message
- in invokeAndCompareArrays, it seems to be useful to have at least one array printed out when expectedResult != result and expectedResult is true

test/lib-test/jdk/test/lib/format/ArrayDiffTest.java:
- it's more common in our code to have an open curved bracket for class definition one the same line as a class name
- you don't need to @compile ArrayDiffTest.java as jtreg should compile it automatically for you by @run.
- copyright year shouldn't have 2019 listed there, unless you wrote this code in the prev. year and just didn't have a chance to integrate it

test/lib/jdk/test/lib/format/ArrayDiff.java:
- similar comment about copyright year
- I don't like FAILURE_MARKS, you are making an assumption about maximal length for an object string representation, you can easily create a mark by `new String("^").repeat(n)`, yes it might be less memory efficient, but that's a test aux library after all
- per code style guidelines, all even one-line loops and if-s should have { }, e.g. L#185, L#192, L#218
- there shouldn't be a space b/w function name and opening parenthesis, e.g. L#183
- 'else if' should be at the same line as closing parenthesis, e.g. L#91
- I don't see why ELLIPSIS is to be in defined in Format when it's used in ArrayDiff, I'd rather see it as a private static final field in ArrayDiff, and if someone else needs '...' string, they can create one
- the same for PADDINGS; + PADDINGS seems to be highly coupled w/ how ArrayDiff and has the same problem as FAILURE_MARKS -- you can easily get IOOOBE
- ArrayCodec::findMismatchIndex assumes that there are no null in source, it's better to use java.util.Objects.equals
- maybe I'm missing smth, but I don't understand why ArrayCodec supports only char and byte arrays; and hence I don't understand why you need ArrayCodec::of methods, as you can simply do new ArrayCoded(Arrays.stream(a).collect(Collectors.toList()) where a is an array of any type  
- it'd be appreciated if all public methods had javadoc which describes all parameters using @param
- it seems that ArrayCodec should be an inner static class of ArrayDiff

test/lib/jdk/test/lib/format/Diff.java
- similar comment about copyright year
- could you please add javadoc to both methods?

test/lib/jdk/test/lib/format/Format.java:
- typo s/maximul/maximal
- shouldn't asLiteral call asLiteral(String.valueOf(o)) at L#58?
- typo s/it's/its at L#45
- it'd be appreciated if all public methods had javadoc which describse all parameters using @param


there are a few other code-style/editorial nonconformities (e.g. space before ')' or javadoc comment doesn't have leading empty line) I've noticed, but I haven't written down the place where I saw them, so I'll make another pass after you address/answer the code-related comments. 

Thanks,
-- Igor


> On May 28, 2020, at 12:33 PM, Evgeny Nikitin <evgeny.nikitin at oracle.com> wrote:
> 
> Forwarding to the compiler mailing list as this changes a test in the Compiler area.
> 
> On 2020-05-18 16:46, Evgeny Nikitin wrote:
>> Hi,
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229186
>> Webrev: http://cr.openjdk.java.net/~enikitin/8229186/webrev.00/
>> Error reporting was improved by writing a C-style escaped string representations for the variables passed to the methods being tested. For array comparisons, a dedicated diff-formatter was implemented.
>> Sample output for comparing byte arrays (with artificial failure):
>> ----------System.err:(21/1553)----------
>> Result: (false) of 'arrayEqualsB' is not equal to expected (true)
>> Arrays differ starting from [index: 7]:
>> ... 5, 6,   7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, ...
>> ... 5, 6, 125, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, ...
>>          ^^^^
>> java.lang.RuntimeException: Result: (false) of 'arrayEqualsB' is not equal to expected (true)
>>         at compiler.intrinsics.string.TestStringIntrinsics.invokeAndCheckArrays(TestStringIntrinsics.java:273)          at ... stack trace continues - E.N.
>> Sample output for comparing char arrays:
>> ----------System.err:(21/1579)*----------
>> Result: (false) of 'arrayEqualsC' is not equal to expected (true)
>> Arrays differ starting from [index: 7]:
>> ... \\u0005, \\u0006, \\u0007, \\u0008, \\u0009, \\n, \\u000B, \\u000C, \\r, \\u000E, ...
>> ... \\u0005, \\u0006,      }, \\u0008, \\u0009, \\n, \\u000B, \\u000C, \\r, \\u000E, ...
>>                    ^^^^^^^
>> java.lang.RuntimeException: Result: (false) of 'arrayEqualsC' is not equal to expected (true)
>>         at compiler.intrinsics.string.TestStringIntrinsics.invokeAndCheckArrays(TestStringIntrinsics.java:280)          at
>> ... and so on - E.N.
>> Please review.
>> Thanks in advance,
>> /Evgeny Nikitin.