[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR(L): 8229186: Improve error messages for TestStringIntrinsics failures
From:       Igor Ignatyev <igor.ignatyev () oracle ! com>
Date:       2020-05-30 0:12:13
Message-ID: A3EA1F84-589B-4B44-9948-7CFF32B24D1D () oracle ! com
[Download RAW message or body]

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@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.


[prev in list] [next in list] [prev in thread] [next in thread]