Re: RFR: 8140336: Add @modules for exported dependencies to jdk_core tests

[Alan Bateman]

On 27/10/2015 13:36, Alexandre (Shura) Iline wrote:

On Oct 27, 2015, at 12:17 AM, Mandy Chung  wrote:



On Oct 26, 2015, at 10:13 AM, Alexandre (Shura) Iline 
 wrote:

Hi.

Could you please take a look on the suggested change in tests belonging to 
jdk_core test group.

Pls notice that the dependencies which would later be removed by fixing 
JDK-8139430 we not added in this change.

http://cr.openjdk.java.net/~shurailine/8140336/webrev.01/

Thanks for the patch.

This change is against jake/jdk repo.  I assume you intend to integrate this 
change to jdk9/dev/jdk.  You can separate into two changesets, one for 
jdk9/dev/jdk and the other for test/jdk/jigsaw tests that I can push your 
change to test/jdk/jigsaw tests to jake.

I have created two separate reviews:
http://cr.openjdk.java.net/~shurailine/8140336/webrev.jdk9.02/
http://cr.openjdk.java.net/~shurailine/8140336/webrev.jake.02/
The two patches mostly look okay to me. I notice in 
JmodNegativeTest.java that it adds a second @modules tag, I assume it 
would be clearer to have one tag.


-Alan

Re: RFR: 8140336: Add @modules for exported dependencies to jdk_core tests

[Mandy Chung]

> On Oct 27, 2015, at 6:36 AM, Alexandre (Shura) Iline 
>  wrote:
> 
> I have created two separate reviews:
> http://cr.openjdk.java.net/~shurailine/8140336/webrev.jdk9.02/
> http://cr.openjdk.java.net/~shurailine/8140336/webrev.jake.02/

Thanks.

I have pushed webrev.jdk9.02 to jdk9/dev and webrev.jake.03 to jake.

> 
>> 
>> test/java/lang/ProcessHandle/TEST.properties
>> - which test references types in jdk.management?   If there is only a couple 
>> of tests depending on jdk.management, better to use @modules in those 
>> individual tests instead.
> 
> The dependency is coming through java/lang/ProcessHandle/JavaChild.java. 
> Three of five tests in the dir use it: InfoTest.java, OnExitTest.java, 
> TreeTest.java.
> 
> Yes, I could update just the three tests, but since JTReg will compile all 
> the classes in the dir, the compilation for the rest of the tests will fail 
> if no jdk.management present. Instead I would rather have the fix as it is 
> right now and have a separate bug to fix this later. Which is what I was 
> going to do.
> 

That’s okay.  Just need to make sure the java.management dependency is removed 
when  JDK-8139430 is resolved.

Mandy

Re: RFR: 8140336: Add @modules for exported dependencies to jdk_core tests

[Alexandre (Shura) Iline]

> On Oct 27, 2015, at 12:17 AM, Mandy Chung  wrote:
> 
> 
>> On Oct 26, 2015, at 10:13 AM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> Hi.
>> 
>> Could you please take a look on the suggested change in tests belonging to 
>> jdk_core test group.
>> 
>> Pls notice that the dependencies which would later be removed by fixing 
>> JDK-8139430 we not added in this change.
>> 
>> http://cr.openjdk.java.net/~shurailine/8140336/webrev.01/
> 
> Thanks for the patch.
> 
> This change is against jake/jdk repo.  I assume you intend to integrate this 
> change to jdk9/dev/jdk.  You can separate into two changesets, one for 
> jdk9/dev/jdk and the other for test/jdk/jigsaw tests that I can push your 
> change to test/jdk/jigsaw tests to jake.

I have created two separate reviews:
http://cr.openjdk.java.net/~shurailine/8140336/webrev.jdk9.02/
http://cr.openjdk.java.net/~shurailine/8140336/webrev.jake.02/

> 
> test/java/lang/ProcessHandle/TEST.properties
> - which test references types in jdk.management?   If there is only a couple 
> of tests depending on jdk.management, better to use @modules in those 
> individual tests instead.

The dependency is coming through java/lang/ProcessHandle/JavaChild.java. Three 
of five tests in the dir use it: InfoTest.java, OnExitTest.java, TreeTest.java.

Yes, I could update just the three tests, but since JTReg will compile all the 
classes in the dir, the compilation for the rest of the tests will fail if no 
jdk.management present. Instead I would rather have the fix as it is right now 
and have a separate bug to fix this later. Which is what I was going to do.

This is, effectively, the opposite of what I was suggesting with regards to 
JDK-8139430. The difference is that in the case of java/lang/ProcessHandle 
tests, I will only need to update two tests twice and in the case of 
JDK-8139430 it’s a _lot_ of tests.

> 
> Is java.management module pulled in due to the test library in the following 
> tests?
> test/java/lang/instrument/RedefineBigClass.sh
> test/java/lang/instrument/RedefineMethodInBacktrace.sh
> test/java/lang/instrument/RetransformBigClass.sh
> test/java/lang/instrument/NativeMethodPrefixAgent.java

The dependency is coming through test/java/lang/instrument/NMTHelper.java, 
test/java/lang/instrument/RedefineMethodInBacktraceApp.java, 
test/java/lang/instrument/NativeMethodPrefixApp.java

> 
> It’s okay to remove java.management dependency later by JDK-8139430.   At the 
> same time, would it be easier not to update the tests that require 
> java.management due to the test library and let JDK-8139430 fixes it?

See above.

I would rather push this and get on fixing JDK-8139430.

Shura


> 
> Mandy
> 
> 

Re: RFR: 8140336: Add @modules for exported dependencies to jdk_core tests

[Mandy Chung]

> On Oct 26, 2015, at 10:13 AM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi.
> 
> Could you please take a look on the suggested change in tests belonging to 
> jdk_core test group.
> 
> Pls notice that the dependencies which would later be removed by fixing 
> JDK-8139430 we not added in this change.
> 
> http://cr.openjdk.java.net/~shurailine/8140336/webrev.01/

Thanks for the patch.

This change is against jake/jdk repo.  I assume you intend to integrate this 
change to jdk9/dev/jdk.  You can separate into two changesets, one for 
jdk9/dev/jdk and the other for test/jdk/jigsaw tests that I can push your 
change to test/jdk/jigsaw tests to jake.

test/java/lang/ProcessHandle/TEST.properties
- which test references types in jdk.management?   If there is only a couple of 
tests depending on jdk.management, better to use @modules in those individual 
tests instead.

Is java.management module pulled in due to the test library in the following 
tests?
test/java/lang/instrument/RedefineBigClass.sh
test/java/lang/instrument/RedefineMethodInBacktrace.sh
test/java/lang/instrument/RetransformBigClass.sh
test/java/lang/instrument/NativeMethodPrefixAgent.java

It’s okay to remove java.management dependency later by JDK-8139430.   At the 
same time, would it be easier not to update the tests that require 
java.management due to the test library and let JDK-8139430 fixes it?
 
Mandy