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 😊 > 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. > 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. > 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? 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. > >
