RFR(L): 10: JDK-8177728 - [TESTBUG] Improve CDS test utils

[mikhailo]

Hi Calvin,

   Thank you for review. See my comments inline:


On 04/04/2017 05:00 PM, Calvin Cheung wrote:
> Hi Misha,
>
> Thanks for doing this, the tests look cleaner with this change.
>
> Some comments below.
>
> On 3/28/17, 3:09 PM, mikhailo wrote:
>> Please review this enhancement to CDS tests (RFE)
>>
>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8177728
>>    Webrevs:
>> http://cr.openjdk.java.net/~mseledtsov/8177728.00.hotspot/
>
> ArchiveDoesNotExist.java
>
> Nit:
> the ‘.’ in line #55 should be aligned with the previous line
> Similar for lines #63 and 64. They should be aligned with line #62.
Fixed.
>
> MaxMetaspaceSize.java
>
> Only the copyright header has changed; no code changes.
Oops... Reverted the file.
>
> TransformTestCommon.java
>
> 24 import jdk.test.lib.cds.CDSTestUtils;
> The above is the only code change but CDSTestUtils is not being used.
CDSTestUtils is used inside TransformTestCommon.java. E.g.:
Line 104:         if (CDSTestUtils.isUnableToMap(out))
                         return;

What happened is that originally there was a very small CDSTestUtils 
that lived inside
hotspot/test/runtime/SharedArchiveFile.
The new CDSTestUtils has been extended, and moved to a new location where
common test libs reside.

For this reason I needed to add this import statement to 
TransformTestCommon.java

>
>> http://cr.openjdk.java.net/~mseledtsov/8177728.00.top/
> CDSOptions.java:
>   44     public CDSOptions addPrefix(String... prefx) {
>   45         for (String s : prefx) this.prefix.add(s);
>
>   prefx -> prefix
>
>   50     public CDSOptions addSuffix(String... suffx) {
>   51         for (String s : suffx) this.suffix.add(s);
>
>   suffx -> suffix
>
Fixed
> CDSTestUtils.java:
>
> 84     public static OutputAnalyzer checkDump(OutputAnalyzer output, 
> String... extraMatches)
>
> 216     public static OutputAnalyzer runWithArchiveAndCheck(CDSOptions 
> opts) throws Exception {
>
> 251     public static OutputAnalyzer 
> checkExecExpectError(OutputAnalyzer output,
>
> Why returning OutputAnalyzer since the callers aren’t checking it?
>
This is done to allow the style of "chaining the method calls". E.g.
checkDump(x,y,...).shouldNotContain("abcde")
runWithArchiveAndCheck()
     .shouldNotContain(abc)
     .someExtraChecks

I will address Jiangli's comments, and will post the updated webrev.


Thank you,
Misha
> thanks,
> Calvin
>>
>>
>> Here is a brief summary of the improvements:
>>  - create/use CDS utility methods for common patterns in CDS related 
>> tests:
>>      - creating an testlist
>>      - creating an archive, checking result
>>      - executing JVM with the archive
>>      - checking results and common error patterns
>>
>>  - use -Xshare:on when executing CDS tests, and use test utilities
>>    to filter out failures due to inability to map shared archive.
>>    This is a more deterministic way to execute CDS tests.
>>
>>  - additional improvements came up as part of this work,
>>    such as jdk.test.lib.Utils:getTestName()
>>
>> This work also lays ground for future improvements in this area.
>>
>>
>> Testing:
>>     1. Locally: executed affected tests on Linux-x64
>>        (hotspot/test/runtime/SharedArchiveFile)
>>        PASS
>>
>>     2. Automated multi-platform testing
>>        Running the affected tests on a set of supported platforms.
>>        IN Progress...
>>
>> Thank you,
>> Misha
>>