RFR(S/M): 8205419: [testbug] TestJmapCore failing without SA: introduce @requires vm.hasSA

[Chris Plummer]

Hi Goetz,

On 6/20/18 1:22 PM, Lindenmaier, Goetz wrote:
> Hi Chris,
>
> Thanks for looking at this change.
>
>> Can't VMProps.vmHasSA() just return Platform.shouldSAAttach()? It seems
>> you are unnecessarily replicating Platform.shouldSAAttach() logic.
> I think there are two things to distinguish:
>   - platforms that don't implement SA
>   - systems/setups where SA does not work.
> If both can be recognized when VMProps is evaluated, shouldSAAttach()
> is the one that is superfluous in the long run?!  If not, shouldSAAttach
> should only return those where it is necessary to check in the test.
> But this is probably too much brains in this case ��
I'm not really sure what you are pointing our here. My point it that 
VMProps.vmHasSA() can just return Platform.shouldSAAttach() and not 
include all the extra logic you have. I wasn't saying one or the other 
is not needed.
>
>> If you are adding "@requires vm.hasSAandCanAttach" to a test, shouldn't
>> you remove the test's Platform.shouldSAAttach() check?
> Well I asked in my mail whether I should do that. So I take this as a yes.
> But it depends on whether it must be evaluated in the test.
I think yes for consistency.
>
>> Is there a reason not to take the more direct and established approach
>> of just adding the Platform.shouldSAAttach() check to TestJmapCore? It
>> would be a lot less disruptive.  I know the vm.hasSAandCanAttach
>> approach has advantages in not even attempting to compile and run the
>> test, but I'm not so sure those advantages outweigh the simplicity of
>> just adding a Platform.shouldSAAttach() check to one test. I can go
>> either way here. Just wondering if there are additional reasons I might
>> not be seeing.
> I like the @requires much more. It tells right where the test is described
> that it does not run with SA. The other is quite hidden in the test, e.g., in
> helper class ClhsdbLauncher.java.
> Also, it's more easy to remember (The implementor of
> TestJmapCore forgot it...).  And, last but not least, it seems best practice
> nowadays. There are lots of excludes for mac, they are also done by @requires
> and not as a check using Platform at runtime.
Sometimes however you just can't avoid the platform or functionality 
check in the code because only part of the test is reliant on it.
>> Sorry, I don't have an answer to your VMProps evaluation question. You
>> might need to ask a wider audience than just svc.
> Whom should I ask? Hotspot-dev?
Well, that would cover the widest possible audience for this question. :)

It's up to you. I was just suggesting that you'll be more likely to get 
an answer if you expand beyond svc. You could got with runtime-dev, 
hotspot-dev, or jdk-dev.

cheers,

Chris
>
> Best regards,
>    Goetz.
>
>
>> thanks,
>>
>> Chris
>>
>> On 6/20/18 6:49 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> TestJmapCore is failing on aix because there jhsdb is not implemented.
>>> So far, such tests check at runtime Platform.shouldSAAttach() which is
>> missing
>>> in TestJmapCore.
>>>
>>> Instead, I implement @requires vm.hasSAandCanAttach.
>>> This makes jtreg skip the tests without compiling and starting them.
>>>
>>> I added the new property to all tests that check shouldSAAttach in jdk and
>> hostpot test
>>> directories.
>>> This is the implementation of the new property, the rest is just repetitive
>> checking it:
>>> http://cr.openjdk.java.net/~goetz/wr18/8205419-
>> requiresHasSA/01/test/jtreg-ext/requires/VMProps.java.udiff.html
>>> webrev:
>>> http://cr.openjdk.java.net/~goetz/wr18/8205419-requiresHasSA/01/
>>>
>>> Is it correct to assume the VMProps is evaluated on the same machine and
>> with the same
>>> user as used for executing the test?  canPtraceAttachLinux directly
>> accesses the system.
>>> (Should I remove the checks for shouldSAAttach() from the changed tests?)
>>>
>>> Best regards,
>>>     Goetz.
>>