jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Jaroslav Bachorik
Please, review the following simple change

Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00

The patch removes the special path taken when jmxrmi.jar is present on
the bootclasspath. There are two reasons for this cleanup:
1. Bootclasspath will not be available (or meaningful) in Jigsaw
2. We haven't been testing with an external jmxrmi.jar for ages - since
JMX was included into JDK in JDK 5.

The modified test is still passing.

-JB-
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Alan Bateman


On 04/01/2016 10:20, Jaroslav Bachorik wrote:

> Please, review the following simple change
>
> Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
> Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00
>
> The patch removes the special path taken when jmxrmi.jar is present on
> the bootclasspath. There are two reasons for this cleanup:
> 1. Bootclasspath will not be available (or meaningful) in Jigsaw
> 2. We haven't been testing with an external jmxrmi.jar for ages -
> since JMX was included into JDK in JDK 5.
>
> The modified test is still passing.
Does the @summary line need to be updated to drop the mention of "when
JMX is bundled"? Otherwise looks okay to me.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Jaroslav Bachorik
On 4.1.2016 12:09, Alan Bateman wrote:

>
>
> On 04/01/2016 10:20, Jaroslav Bachorik wrote:
>> Please, review the following simple change
>>
>> Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
>> Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00
>>
>> The patch removes the special path taken when jmxrmi.jar is present on
>> the bootclasspath. There are two reasons for this cleanup:
>> 1. Bootclasspath will not be available (or meaningful) in Jigsaw
>> 2. We haven't been testing with an external jmxrmi.jar for ages -
>> since JMX was included into JDK in JDK 5.
>>
>> The modified test is still passing.
> Does the @summary line need to be updated to drop the mention of "when
> JMX is bundled"? Otherwise looks okay to me.

Not sure. With this change in place the test will not even try to detect
the situation when JMX is not bundled with JDK.

There is another test
(jdk/test/javax/management/remote/mandatory/version/ImplVersionTest.java) with
the same wording in the summary but not even trying to detest the
standalone jmxrmi.jar.

If you insist on changing the wording I would prefer changing both of
these tests in one go, perhaps as a separate CR?

-JB-

>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Alan Bateman


On 04/01/2016 15:05, Jaroslav Bachorik wrote:

> :
>
> Not sure. With this change in place the test will not even try to
> detect the situation when JMX is not bundled with JDK.
>
> There is another test
> (jdk/test/javax/management/remote/mandatory/version/ImplVersionTest.java)
> with the same wording in the summary but not even trying to detest the
> standalone jmxrmi.jar.
>
> If you insist on changing the wording I would prefer changing both of
> these tests in one go, perhaps as a separate CR?
I just wanted to make the point that with the change then the @summary
on the test doesn't make sense and it's the opportunity to align it with
what the test does.

Doing the other test as as separate issue is no problem.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Eamonn McManus-2
In reply to this post by Jaroslav Bachorik
I think this test should either be deleted or reduced to a simple check that the MBeanServerDelegate's ImplementationVersion attribute is equal to System.getProperty("java.runtime.version"). The whole business of starting up a separate process and checking things with security managers and so on is completely obsolete.

Éamonn

2016-01-04 2:20 GMT-08:00 Jaroslav Bachorik <[hidden email]>:
Please, review the following simple change

Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00

The patch removes the special path taken when jmxrmi.jar is present on the bootclasspath. There are two reasons for this cleanup:
1. Bootclasspath will not be available (or meaningful) in Jigsaw
2. We haven't been testing with an external jmxrmi.jar for ages - since JMX was included into JDK in JDK 5.

The modified test is still passing.

-JB-

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Jaroslav Bachorik
On 4.1.2016 21:26, Eamonn McManus wrote:
> I think this test should either be deleted or reduced to a simple check
> that the MBeanServerDelegate's ImplementationVersion attribute is equal
> to System.getProperty("java.runtime.version"). The whole business of
> starting up a separate process and checking things with security
> managers and so on is completely obsolete.

I would leave this for SQE to decide. Removing the security manager part
would change the semantics of the test.

I would suggest dealing with this in a separate issue not to block the
effort of removing references to 'sun.boot.class.path' for jigsaw.

-JB-

>
> Éamonn
>
> 2016-01-04 2:20 GMT-08:00 Jaroslav Bachorik
> <[hidden email] <mailto:[hidden email]>>:
>
>     Please, review the following simple change
>
>     Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
>     Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00
>
>     The patch removes the special path taken when jmxrmi.jar is present
>     on the bootclasspath. There are two reasons for this cleanup:
>     1. Bootclasspath will not be available (or meaningful) in Jigsaw
>     2. We haven't been testing with an external jmxrmi.jar for ages -
>     since JMX was included into JDK in JDK 5.
>
>     The modified test is still passing.
>
>     -JB-
>
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Alan Bateman


On 05/01/2016 13:52, Jaroslav Bachorik wrote:

> On 4.1.2016 21:26, Eamonn McManus wrote:
>> I think this test should either be deleted or reduced to a simple check
>> that the MBeanServerDelegate's ImplementationVersion attribute is equal
>> to System.getProperty("java.runtime.version"). The whole business of
>> starting up a separate process and checking things with security
>> managers and so on is completely obsolete.
>
> I would leave this for SQE to decide. Removing the security manager
> part would change the semantics of the test.
>
> I would suggest dealing with this in a separate issue not to block the
> effort of removing references to 'sun.boot.class.path' for jigsaw.
Just to say that sun.boot.class.path is already removed in the jake
forest. I created JDK-8143047 to re-examine this test as it looked like
it was obsolete and could be deleted. So no urgently fixing this, it's
not blocking anything.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Jaroslav Bachorik
On 5.1.2016 15:00, Alan Bateman wrote:

>
>
> On 05/01/2016 13:52, Jaroslav Bachorik wrote:
>> On 4.1.2016 21:26, Eamonn McManus wrote:
>>> I think this test should either be deleted or reduced to a simple check
>>> that the MBeanServerDelegate's ImplementationVersion attribute is equal
>>> to System.getProperty("java.runtime.version"). The whole business of
>>> starting up a separate process and checking things with security
>>> managers and so on is completely obsolete.
>>
>> I would leave this for SQE to decide. Removing the security manager
>> part would change the semantics of the test.
>>
>> I would suggest dealing with this in a separate issue not to block the
>> effort of removing references to 'sun.boot.class.path' for jigsaw.
> Just to say that sun.boot.class.path is already removed in the jake
> forest. I created JDK-8143047 to re-examine this test as it looked like
> it was obsolete and could be deleted. So no urgently fixing this, it's
> not blocking anything.

Ok. Thanks for the clarification. In this case I downgraded the task
priority to P3.

-JB-

>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Eamonn McManus-2
In reply to this post by Jaroslav Bachorik
OK. In that case I would suggest removing the checkVersion variable since it is now always true, along with the logic from ImplVersionCommand for when it is false.

Éamonn

2016-01-05 5:52 GMT-08:00 Jaroslav Bachorik <[hidden email]>:
On 4.1.2016 21:26, Eamonn McManus wrote:
I think this test should either be deleted or reduced to a simple check
that the MBeanServerDelegate's ImplementationVersion attribute is equal
to System.getProperty("java.runtime.version"). The whole business of
starting up a separate process and checking things with security
managers and so on is completely obsolete.

I would leave this for SQE to decide. Removing the security manager part would change the semantics of the test.

I would suggest dealing with this in a separate issue not to block the effort of removing references to 'sun.boot.class.path' for jigsaw.

-JB-


Éamonn

2016-01-04 2:20 GMT-08:00 Jaroslav Bachorik
<[hidden email] <mailto:[hidden email]>>:

    Please, review the following simple change

    Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
    Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00

    The patch removes the special path taken when jmxrmi.jar is present
    on the bootclasspath. There are two reasons for this cleanup:
    1. Bootclasspath will not be available (or meaningful) in Jigsaw
    2. We haven't been testing with an external jmxrmi.jar for ages -
    since JMX was included into JDK in JDK 5.

    The modified test is still passing.

    -JB-




Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Jaroslav Bachorik
On 5.1.2016 16:47, Eamonn McManus wrote:
> OK. In that case I would suggest removing the checkVersion variable
> since it is now always true, along with the logic from
> ImplVersionCommand for when it is false.

Done. Also updated the test summary wording as suggested by Alan:
http://cr.openjdk.java.net/~jbachorik/8143047/webrev.01

-JB-

>
> Éamonn
>
> 2016-01-05 5:52 GMT-08:00 Jaroslav Bachorik
> <[hidden email] <mailto:[hidden email]>>:
>
>     On 4.1.2016 21:26, Eamonn McManus wrote:
>
>         I think this test should either be deleted or reduced to a
>         simple check
>         that the MBeanServerDelegate's ImplementationVersion attribute
>         is equal
>         to System.getProperty("java.runtime.version"). The whole business of
>         starting up a separate process and checking things with security
>         managers and so on is completely obsolete.
>
>
>     I would leave this for SQE to decide. Removing the security manager
>     part would change the semantics of the test.
>
>     I would suggest dealing with this in a separate issue not to block
>     the effort of removing references to 'sun.boot.class.path' for jigsaw.
>
>     -JB-
>
>
>         Éamonn
>
>         2016-01-04 2:20 GMT-08:00 Jaroslav Bachorik
>         <[hidden email]
>         <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>>:
>
>              Please, review the following simple change
>
>              Issue : https://bugs.openjdk.java.net/browse/JDK-8143047
>              Webrev: http://cr.openjdk.java.net/~jbachorik/8143047/webrev.00
>
>              The patch removes the special path taken when jmxrmi.jar is
>         present
>              on the bootclasspath. There are two reasons for this cleanup:
>              1. Bootclasspath will not be available (or meaningful) in
>         Jigsaw
>              2. We haven't been testing with an external jmxrmi.jar for
>         ages -
>              since JMX was included into JDK in JDK 5.
>
>              The modified test is still passing.
>
>              -JB-
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Alan Bateman


On 06/01/2016 11:00, Jaroslav Bachorik wrote:
> On 5.1.2016 16:47, Eamonn McManus wrote:
>> OK. In that case I would suggest removing the checkVersion variable
>> since it is now always true, along with the logic from
>> ImplVersionCommand for when it is false.
>
> Done. Also updated the test summary wording as suggested by Alan:
> http://cr.openjdk.java.net/~jbachorik/8143047/webrev.01
@summary looks okay now - thanks!

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8143047: Re-examine javax/management/ImplementationVersion/ImplVersionTest.java

Eamonn McManus-2
Looks good to me (emcmanus) too.

Éamonn

2016-01-06 7:04 GMT-08:00 Alan Bateman <[hidden email]>:


On 06/01/2016 11:00, Jaroslav Bachorik wrote:
On 5.1.2016 16:47, Eamonn McManus wrote:
OK. In that case I would suggest removing the checkVersion variable
since it is now always true, along with the logic from
ImplVersionCommand for when it is false.

Done. Also updated the test summary wording as suggested by Alan:
http://cr.openjdk.java.net/~jbachorik/8143047/webrev.01
@summary looks okay now - thanks!

-Alan