RFR: JDK-8251384: [TESTBUG] jvmti tests should not be executed with minimal VM

[Alex Menkov]

Hi David,

On 08/26/2020 21:49, David Holmes wrote:
> Hi Alex,
> 
> On 21/08/2020 6:54 am, Alex Menkov wrote:
>> Hi Igor,
>>
>> On 08/20/2020 09:23, Igor Ignatyev wrote:
>>> HI Alex,
>>>
>>> one minor nit: according to usual java coding conventions, 
>>> isJVMTIIncluded should be spelled as isJvmtiIncluded. otherwise the 
>>> fix looks good to me.
>>
>> I tried to be consistent with other methods like
>> isCDSIncludedInVmBuild, isJFRIncludedInVmBuild, isGCSupported, 
>> isGCSelected, etc.
> 
> Yes - when a name includes an acronym the use of camel-case is a 
> secondary consideration.
> 
>> Maybe this should be isJVMTIIncludedInVmBuild..
> 
> Yes that seems better and I would also prefer to see it implemented in 
> the same style as:

Sorry, the fix was pushed several days ago.
Do you want to change the name to isJVMTIIncludedInVmBuild by follow-up?

> 
> WB_ENTRY(jboolean, WB_IsJFRIncludedInVmBuild(JNIEnv* env))
> #if INCLUDE_JFR
>    return true;
> #else
>    return false;
> #endif // INCLUDE_JFR
> WB_END
> 
> to avoid implicit booleans and avoid the runtime condition check.

I don't think true/false are correct here. This is jboolean, not bool.

INCLUDE_JVMTI ? JNI_TRUE : JNI_FALSE
  #if INCLUDE_JVMTI
     return JNI_TRUE;
  #else
     return JNI_FALSE;
  #endif
looks better imo.

--alex

> 
> Thanks,
> David
> -----
> 
>>>
>>>> Other tests will be updated in the follow-ups.
>>> have you already identified all the tests which need this @requires? 
>>> filed bugs/RFEs for them?
>>
>> Not yet.
>> I had problem with running all hotspot tests with minimal build (for 
>> some reason jtreg was not able to complete it), so I decided start 
>> from the tests mentioned in the jira issue and then test area-by-area, 
>> file and fix the tests in batches.
>>
>> --alex
>>
>>>
>>> Cheers,
>>> -- Igor
>>>
>>>
>>>> On Aug 19, 2020, at 6:02 PM, Alex Menkov <alexey.menkov at oracle.com> 
>>>> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> please review the fix for
>>>> https://bugs.openjdk.java.net/browse/JDK-8251384
>>>> webrev:
>>>> http://cr.openjdk.java.net/~amenkov/jdk16/minimal_jvmti/webrev/
>>>>
>>>> The fix introduces new @requires option "vm.jvmti":
>>>> test/lib/sun/hotspot/WhiteBox.java
>>>> test/jtreg-ext/requires/VMProps.java
>>>> src/hotspot/share/prims/whitebox.cpp
>>>> test/hotspot/jtreg/TEST.ROOT
>>>>
>>>> and updates tests in test/hotspot/jtreg/serviceability/jvmti (the 
>>>> only change in all tests is added "@requires vm.jvmti")
>>>> Other tests will be updated in the follow-ups.
>>>>
>>>> The
>>>