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.
