Ok. That makes sense. I was under the impression other tests were failing.
So only the one update is needed for this and it is arguably just an omission.

jpackage has a couple of types of requirement
1) run only on platforms that support jpackage - the @modiules will do this.
2) run only on a subset of (1) - ie one or more specific platforms that support jpackage

If the tests being updated are all of type (2) then the @requires platform may still be merited but probably that is not the case - it sounds like (1), so I think Andy should add what you suggest but I expect the @requires is harmless to leave in too ...

-phil.

On 12/7/19 1:47 PM, Igor Ignatev wrote:
As far as I can see only junit.java test is executed on Solaris and is the only 
one failing. jtreg uses @modules during test selection/filtering phase, so 
tests which have @modules A won’t be run on jdk which doesn’t have module A, 
hence it should be sufficient. If it’s not, we have a bug in jtreg.

— Igor

On Dec 7, 2019, at 1:43 PM, Phil Race <philip.r...@oracle.com> wrote:

All these tests specify this already so it doesn’t seem sufficient.

-Phil.

On Dec 7, 2019, at 12:07 PM, Igor Ignatyev <igor.ignat...@oracle.com> wrote:

can we just add '@modules jdk.incubator.jpackage' to 
test/jdk/tools/jpackage/junit/junit.java to solve that? or some of the tests 
run by test/jdk/tools/jpackage/junit/junit.java don't need 
jdk.incubator.jpackage module?

-- Igor

On Dec 7, 2019, at 11:57 AM, Philip Race <philip.r...@oracle.com> wrote:
Yes, since only a (relatively) small number of tests needed to be updated
this is fine with me at least for now. So +1

-phil.

On 12/7/19, 5:48 AM, Andy Herrick wrote:
Phil - are you approving this change ? - I think you are the only registered 
Reviewer.

/Andy

On 12/6/2019 8:11 PM, Phil Race wrote:
Well we could deprecate and remove the solaris port :-)
But until that is done this is the only way I know of.
we could require all jpackage tests to include some helper code which decides 
if it is applicable but that will be more work upfront and many jpackage tests 
are already platform specific so @requires is not going away.


-Phil.

On Dec 6, 2019, at 2:33 PM, Alexander Matveev <alexander.matv...@oracle.com> 
wrote:

Looks good, but is there better way to exclude tests on Solaris? I do not like 
idea adding @requires for all tests.

Thanks,
Alexander

On 12/6/2019 10:35 AM, Alexey Semenyuk wrote:
Looks good.

- Alexey

On 12/6/2019 1:33 PM, Andy Herrick wrote:
Please review this jpackager test fix for bug [1] at [2].

the fix adds "@requires (os.family == "linux") | (os.family == "mac") | (os.family == "windows")" 
to all jpackage tests that were not already filtered with "@requires (os.family == "XXX")"

[1] https://bugs.openjdk.java.net/browse/JDK-8235453

[2] http://cr.openjdk.java.net/~herrick/8235453/

/Andy


Reply via email to