Hi Alex,

  Several test-related comments:
  - CanGenerateAllClassHook.java
     " 44  * So the test runs 2 java process": s/process/process*es*/

  - CanGenerateAllClassHook.java
A general comment - please consider using "TestCommon" class and "CDSTestUtils" where possible. There are useful methods for forming ProcessBuilder with parameters specific to AppCDS, and also for checking results. In some cases the test case is so custom that use of these utilities is not applicable. Please take a look at ClassFileLoadHookTest.java as an example. It uses TestCommon.executeAndLog(), TestCommon.checkExec() and more. Other good examples are SharedArchiveFile.java, HelloExtTest.java or other tests under
     appcds directory.

Important to note a condition of "mapping failure". When using CDS/AppCDS with Xshare:on, the mapping of a CDS archive to memory may fail at random (due to ASLR or other conditions). TestCommon.checkExec() handles such conditions propertly, otherwise you will have random/intermittent test failures.

  - CanGenerateAllClassHook.java

    122     private static boolean isWin() {

I recommend using jdk.test.lib.Platform, the isWindows() method. The SQE guideline is to use test utilities where possible.
     FYI, the test utilities reside in: open/test/lib/jdk/test/lib

The rest of the tests look good,

Thank you,

On 2/1/18, 4:11 PM, Alex Menkov wrote:
Hi Serguei,

Thanks for the feedback.

Updated fix: http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev.02/


On 02/01/2018 11:00, serguei.spit...@oracle.com wrote:
One more attempt to send it with the correct to-list...
Sorry for the noise.

Hi Alex,

It looks good in general.
A couple of comments on the test.

We had a convention to avoid having test sub-folders with bug numbers.
Could you, please, rename it to some symbolic name?

Also we need the test to fail in the native agent when an error is reported with the reportError().

It'd be great if some CDS/AppCDS experts from the Runtime team look at the fix and test
so I've included Jiangli and Misha into the to-list.


On 1/25/18 16:11, Alex Menkov wrote:
Hi all,

Please take a look at a fix for
JDK-8161605 : The '!UseSharedSpaces' check is not need in JvmtiManageCapabilities::recompute_always_capabilities

The fix moves can_generate_all_class_hook_events capability from "onload_capabilities" to "always_capabilities" and cleans up recompute_always_capabilities method.

jira: https://bugs.openjdk.java.net/browse/JDK-8161605
webrev: http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev/


Reply via email to