Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

Igor Ignatyev
Shura,

Thank you for review, I agree that having separate bugs is more convenient. [1] is new webrev w/ changes only in the files w/ incorrect module dependency declarations.  

[1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html <http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html>

Thanks,
— Igor

> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>
> Igor,
>
> I have reviewed some number tests which change the @modules - they are fine.
>
> You, however, fix more things with this than missing module dependency declaration. There is a redesign of line-number-sensitive tests [1] and other multiple improvements such as in [2]. Would it be more convenient to have that as separate bugs?
>
> Shura
>
> [1] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
> [2] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>
>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <[hidden email]> wrote:
>>
>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>
>> Hi all,
>>
>> Could you please review this changeset which fix @modules dependency declaration in jdk_svc tests?
>> there are a couple issues w/ modules in jdk_svc tests:
>> - some tests do not specify modules which they depend on
>> - modules in TEST.properties is not used in cases there all tests (should) have the same @modules directive
>> - @modules directive isn't placed according to current convention (before the 1st run directive)
>>
>> Since this fix has already touched lots of tests, I have decided to use this opportunity and reordered some of jtreg tags as well, so there won’t be two massive updates in the tests.
>>
>> Some of our tests are line number sensitive, and then I fixed jtreg declaration, they started to fail. It was really hard to find our all line number sensitive tests, so I have unified the way we declare that as a part of this fix. Please let me know if you prefer to have it done separately.
>>
>> There are two one-liners which, I hope, can simplify review:
>> [1] shows only the changes which are not in comments. Besides obvious new added TEST.properties, there are changes in the following line number sensitive tests (which I mentioned before):
>> test/com/sun/jdi/ArgumentValuesTest.java
>> test/com/sun/jdi/BreakpointTest.java  
>> test/com/sun/jdi/FetchLocals.java
>> test/com/sun/jdi/GetLocalVariables.java
>> test/com/sun/jdi/GetSetLocalTest.java
>> test/com/sun/jdi/LambdaBreakpointTest.java
>> test/com/sun/jdi/LineNumberOnBraceTest.java
>> test/com/sun/jdi/PopAndStepTest.java
>>
>> [2] shows changes in jtreg tags, it can help to see that almost all changes in jtreg tags are either moving of tags which does not affect execution order or @modules changes.
>>
>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>> Testing:
>> - jdk_svc on linux, windows, mac
>> - checked that all tests which could be executed with full jdk before still can be executed with full jdk
>>
>> Thanks,
>> — Igor
>>
>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

Alexandre (Shura) Iline
Igor,

I have looked through a bunch of tests where @modules is changed - that looks good.

One minor thing I noticed is that, in test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are explicitly calling out java.management. You do not have to do that because “modules” property is concatenated through the hierarchy.  java.management is already listed in test/java/lang/management/TEST.properties.

Shura

> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <[hidden email]> wrote:
>
> Shura,
>
> Thank you for review, I agree that having separate bugs is more convenient. [1] is new webrev w/ changes only in the files w/ incorrect module dependency declarations.  
>
> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html
>
> Thanks,
> — Igor
>
>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>
>> Igor,
>>
>> I have reviewed some number tests which change the @modules - they are fine.
>>
>> You, however, fix more things with this than missing module dependency declaration. There is a redesign of line-number-sensitive tests [1] and other multiple improvements such as in [2]. Would it be more convenient to have that as separate bugs?
>>
>> Shura
>>
>> [1] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
>> [2] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>>
>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <[hidden email]> wrote:
>>>
>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>>
>>> Hi all,
>>>
>>> Could you please review this changeset which fix @modules dependency declaration in jdk_svc tests?
>>> there are a couple issues w/ modules in jdk_svc tests:
>>> - some tests do not specify modules which they depend on
>>> - modules in TEST.properties is not used in cases there all tests (should) have the same @modules directive
>>> - @modules directive isn't placed according to current convention (before the 1st run directive)
>>>
>>> Since this fix has already touched lots of tests, I have decided to use this opportunity and reordered some of jtreg tags as well, so there won’t be two massive updates in the tests.
>>>
>>> Some of our tests are line number sensitive, and then I fixed jtreg declaration, they started to fail. It was really hard to find our all line number sensitive tests, so I have unified the way we declare that as a part of this fix. Please let me know if you prefer to have it done separately.
>>>
>>> There are two one-liners which, I hope, can simplify review:
>>> [1] shows only the changes which are not in comments. Besides obvious new added TEST.properties, there are changes in the following line number sensitive tests (which I mentioned before):
>>> test/com/sun/jdi/ArgumentValuesTest.java
>>> test/com/sun/jdi/BreakpointTest.java  
>>> test/com/sun/jdi/FetchLocals.java
>>> test/com/sun/jdi/GetLocalVariables.java
>>> test/com/sun/jdi/GetSetLocalTest.java
>>> test/com/sun/jdi/LambdaBreakpointTest.java
>>> test/com/sun/jdi/LineNumberOnBraceTest.java
>>> test/com/sun/jdi/PopAndStepTest.java
>>>
>>> [2] shows changes in jtreg tags, it can help to see that almost all changes in jtreg tags are either moving of tags which does not affect execution order or @modules changes.
>>>
>>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>>> Testing:
>>> - jdk_svc on linux, windows, mac
>>> - checked that all tests which could be executed with full jdk before still can be executed with full jdk
>>>
>>> Thanks,
>>> — Igor
>>>
>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

Igor Ignatyev
Shura,

Thank you for your review. I have update test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked that there are no similar things in other TEST.properties files.

Still looking for a review from a Reviewer.

[1]
> --- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Mon Mar 13 18:54:58 2017 -0700
> +++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Wed Mar 15 11:09:06 2017 -0700
> @@ -1,3 +1,2 @@
> -modules = java.management \
> -          java.logging
> +modules = java.logging


Thanks,
— Igor

> On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline <[hidden email]> wrote:
>
> Igor,
>
> I have looked through a bunch of tests where @modules is changed - that looks good.
>
> One minor thing I noticed is that, in test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are explicitly calling out java.management. You do not have to do that because “modules” property is concatenated through the hierarchy.  java.management is already listed in test/java/lang/management/TEST.properties.
>
> Shura
>
>> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <[hidden email]> wrote:
>>
>> Shura,
>>
>> Thank you for review, I agree that having separate bugs is more convenient. [1] is new webrev w/ changes only in the files w/ incorrect module dependency declarations.  
>>
>> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html
>>
>> Thanks,
>> — Igor
>>
>>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>>
>>> Igor,
>>>
>>> I have reviewed some number tests which change the @modules - they are fine.
>>>
>>> You, however, fix more things with this than missing module dependency declaration. There is a redesign of line-number-sensitive tests [1] and other multiple improvements such as in [2]. Would it be more convenient to have that as separate bugs?
>>>
>>> Shura
>>>
>>> [1] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
>>> [2] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>>>
>>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>
>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>>>
>>>> Hi all,
>>>>
>>>> Could you please review this changeset which fix @modules dependency declaration in jdk_svc tests?
>>>> there are a couple issues w/ modules in jdk_svc tests:
>>>> - some tests do not specify modules which they depend on
>>>> - modules in TEST.properties is not used in cases there all tests (should) have the same @modules directive
>>>> - @modules directive isn't placed according to current convention (before the 1st run directive)
>>>>
>>>> Since this fix has already touched lots of tests, I have decided to use this opportunity and reordered some of jtreg tags as well, so there won’t be two massive updates in the tests.
>>>>
>>>> Some of our tests are line number sensitive, and then I fixed jtreg declaration, they started to fail. It was really hard to find our all line number sensitive tests, so I have unified the way we declare that as a part of this fix. Please let me know if you prefer to have it done separately.
>>>>
>>>> There are two one-liners which, I hope, can simplify review:
>>>> [1] shows only the changes which are not in comments. Besides obvious new added TEST.properties, there are changes in the following line number sensitive tests (which I mentioned before):
>>>> test/com/sun/jdi/ArgumentValuesTest.java
>>>> test/com/sun/jdi/BreakpointTest.java  
>>>> test/com/sun/jdi/FetchLocals.java
>>>> test/com/sun/jdi/GetLocalVariables.java
>>>> test/com/sun/jdi/GetSetLocalTest.java
>>>> test/com/sun/jdi/LambdaBreakpointTest.java
>>>> test/com/sun/jdi/LineNumberOnBraceTest.java
>>>> test/com/sun/jdi/PopAndStepTest.java
>>>>
>>>> [2] shows changes in jtreg tags, it can help to see that almost all changes in jtreg tags are either moving of tags which does not affect execution order or @modules changes.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>>>> Testing:
>>>> - jdk_svc on linux, windows, mac
>>>> - checked that all tests which could be executed with full jdk before still can be executed with full jdk
>>>>
>>>> Thanks,
>>>> — Igor
>>>>
>>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

serguei.spitsyn@oracle.com
Hi Igor,

This looks good to me.
A couple of questions below.


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html

- * @modules jdk.jdi
   *  @library /test/lib
+ * @modules java.management
+ * jdk.jdi    Should the jdk.jdi be removed from this com/sun/jdi test?


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html

   *  @modules java.corba
   *           jdk.jdi

   Should the jdk.jdi be removed from this com/sun/jdi test?


Thanks,
Serguei


On 3/15/17 11:16, Igor Ignatyev wrote:

> Shura,
>
> Thank you for your review. I have update test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked that there are no similar things in other TEST.properties files.
>
> Still looking for a review from a Reviewer.
>
> [1]
>> --- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Mon Mar 13 18:54:58 2017 -0700
>> +++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Wed Mar 15 11:09:06 2017 -0700
>> @@ -1,3 +1,2 @@
>> -modules = java.management \
>> -          java.logging
>> +modules = java.logging
>
> Thanks,
> — Igor
>
>> On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>
>> Igor,
>>
>> I have looked through a bunch of tests where @modules is changed - that looks good.
>>
>> One minor thing I noticed is that, in test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are explicitly calling out java.management. You do not have to do that because “modules” property is concatenated through the hierarchy.  java.management is already listed in test/java/lang/management/TEST.properties.
>>
>> Shura
>>
>>> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <[hidden email]> wrote:
>>>
>>> Shura,
>>>
>>> Thank you for review, I agree that having separate bugs is more convenient. [1] is new webrev w/ changes only in the files w/ incorrect module dependency declarations.
>>>
>>> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html
>>>
>>> Thanks,
>>> — Igor
>>>
>>>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>>>
>>>> Igor,
>>>>
>>>> I have reviewed some number tests which change the @modules - they are fine.
>>>>
>>>> You, however, fix more things with this than missing module dependency declaration. There is a redesign of line-number-sensitive tests [1] and other multiple improvements such as in [2]. Would it be more convenient to have that as separate bugs?
>>>>
>>>> Shura
>>>>
>>>> [1] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
>>>> [2] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>>>>
>>>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>
>>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>>>> Hi all,
>>>>>
>>>>> Could you please review this changeset which fix @modules dependency declaration in jdk_svc tests?
>>>>> there are a couple issues w/ modules in jdk_svc tests:
>>>>> - some tests do not specify modules which they depend on
>>>>> - modules in TEST.properties is not used in cases there all tests (should) have the same @modules directive
>>>>> - @modules directive isn't placed according to current convention (before the 1st run directive)
>>>>>
>>>>> Since this fix has already touched lots of tests, I have decided to use this opportunity and reordered some of jtreg tags as well, so there won’t be two massive updates in the tests.
>>>>>
>>>>> Some of our tests are line number sensitive, and then I fixed jtreg declaration, they started to fail. It was really hard to find our all line number sensitive tests, so I have unified the way we declare that as a part of this fix. Please let me know if you prefer to have it done separately.
>>>>>
>>>>> There are two one-liners which, I hope, can simplify review:
>>>>> [1] shows only the changes which are not in comments. Besides obvious new added TEST.properties, there are changes in the following line number sensitive tests (which I mentioned before):
>>>>> test/com/sun/jdi/ArgumentValuesTest.java
>>>>> test/com/sun/jdi/BreakpointTest.java
>>>>> test/com/sun/jdi/FetchLocals.java
>>>>> test/com/sun/jdi/GetLocalVariables.java
>>>>> test/com/sun/jdi/GetSetLocalTest.java
>>>>> test/com/sun/jdi/LambdaBreakpointTest.java
>>>>> test/com/sun/jdi/LineNumberOnBraceTest.java
>>>>> test/com/sun/jdi/PopAndStepTest.java
>>>>>
>>>>> [2] shows changes in jtreg tags, it can help to see that almost all changes in jtreg tags are either moving of tags which does not affect execution order or @modules changes.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>>>>> Testing:
>>>>> - jdk_svc on linux, windows, mac
>>>>> - checked that all tests which could be executed with full jdk before still can be executed with full jdk
>>>>>
>>>>> Thanks,
>>>>> — Igor
>>>>>
>>>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>>>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

Igor Ignatyev
Hi Serguei,

1s of all, thank you for your review.

since these tests have dependencies on more modules than corresponding TEST.properties file declares, we have to specify @modules tag in them, and because @modules in a test overrides modules from TEST.properties, jdk.jdi module should be mentioned in the tests.

— Igor  
 

> On Mar 15, 2017, at 9:53 PM, [hidden email] wrote:
>
> Hi Igor,
>
> This looks good to me.
> A couple of questions below.
>
>
> http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html
>
> - * @modules jdk.jdi
>  *  @library /test/lib
> + * @modules java.management
> + * jdk.jdi    Should the jdk.jdi be removed from this com/sun/jdi test?
>
>
> http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html
>
>  *  @modules java.corba
>  *           jdk.jdi
>
>  Should the jdk.jdi be removed from this com/sun/jdi test?
>
>
> Thanks,
> Serguei
>
>
> On 3/15/17 11:16, Igor Ignatyev wrote:
>> Shura,
>>
>> Thank you for your review. I have update test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked that there are no similar things in other TEST.properties files.
>>
>> Still looking for a review from a Reviewer.
>>
>> [1]
>>> --- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Mon Mar 13 18:54:58 2017 -0700
>>> +++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Wed Mar 15 11:09:06 2017 -0700
>>> @@ -1,3 +1,2 @@
>>> -modules = java.management \
>>> -          java.logging
>>> +modules = java.logging
>>
>> Thanks,
>> — Igor
>>
>>> On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>>
>>> Igor,
>>>
>>> I have looked through a bunch of tests where @modules is changed - that looks good.
>>>
>>> One minor thing I noticed is that, in test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are explicitly calling out java.management. You do not have to do that because “modules” property is concatenated through the hierarchy.  java.management is already listed in test/java/lang/management/TEST.properties.
>>>
>>> Shura
>>>
>>>> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>
>>>> Shura,
>>>>
>>>> Thank you for review, I agree that having separate bugs is more convenient. [1] is new webrev w/ changes only in the files w/ incorrect module dependency declarations.
>>>>
>>>> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html
>>>>
>>>> Thanks,
>>>> — Igor
>>>>
>>>>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>>>>
>>>>> Igor,
>>>>>
>>>>> I have reviewed some number tests which change the @modules - they are fine.
>>>>>
>>>>> You, however, fix more things with this than missing module dependency declaration. There is a redesign of line-number-sensitive tests [1] and other multiple improvements such as in [2]. Would it be more convenient to have that as separate bugs?
>>>>>
>>>>> Shura
>>>>>
>>>>> [1] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
>>>>> [2] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>>>>>
>>>>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>>>>> Hi all,
>>>>>>
>>>>>> Could you please review this changeset which fix @modules dependency declaration in jdk_svc tests?
>>>>>> there are a couple issues w/ modules in jdk_svc tests:
>>>>>> - some tests do not specify modules which they depend on
>>>>>> - modules in TEST.properties is not used in cases there all tests (should) have the same @modules directive
>>>>>> - @modules directive isn't placed according to current convention (before the 1st run directive)
>>>>>>
>>>>>> Since this fix has already touched lots of tests, I have decided to use this opportunity and reordered some of jtreg tags as well, so there won’t be two massive updates in the tests.
>>>>>>
>>>>>> Some of our tests are line number sensitive, and then I fixed jtreg declaration, they started to fail. It was really hard to find our all line number sensitive tests, so I have unified the way we declare that as a part of this fix. Please let me know if you prefer to have it done separately.
>>>>>>
>>>>>> There are two one-liners which, I hope, can simplify review:
>>>>>> [1] shows only the changes which are not in comments. Besides obvious new added TEST.properties, there are changes in the following line number sensitive tests (which I mentioned before):
>>>>>> test/com/sun/jdi/ArgumentValuesTest.java
>>>>>> test/com/sun/jdi/BreakpointTest.java
>>>>>> test/com/sun/jdi/FetchLocals.java
>>>>>> test/com/sun/jdi/GetLocalVariables.java
>>>>>> test/com/sun/jdi/GetSetLocalTest.java
>>>>>> test/com/sun/jdi/LambdaBreakpointTest.java
>>>>>> test/com/sun/jdi/LineNumberOnBraceTest.java
>>>>>> test/com/sun/jdi/PopAndStepTest.java
>>>>>>
>>>>>> [2] shows changes in jtreg tags, it can help to see that almost all changes in jtreg tags are either moving of tags which does not affect execution order or @modules changes.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>>>>>> Testing:
>>>>>> - jdk_svc on linux, windows, mac
>>>>>> - checked that all tests which could be executed with full jdk before still can be executed with full jdk
>>>>>>
>>>>>> Thanks,
>>>>>> — Igor
>>>>>>
>>>>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>>>>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

serguei.spitsyn@oracle.com
Igor,

It was my guess but it is nice to have it explicitly explained.

Thanks!
Serguei



On 3/15/17 22:07, Igor Ignatyev wrote:

> Hi Serguei,
>
> 1s of all, thank you for your review.
>
> since these tests have dependencies on more modules than corresponding TEST.properties file declares, we have to specify @modules tag in them, and because @modules in a test overrides modules from TEST.properties, jdk.jdi module should be mentioned in the tests.
>
> — Igor
>  
>> On Mar 15, 2017, at 9:53 PM, [hidden email] wrote:
>>
>> Hi Igor,
>>
>> This looks good to me.
>> A couple of questions below.
>>
>>
>> http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html
>>
>> - * @modules jdk.jdi
>>   *  @library /test/lib
>> + * @modules java.management
>> + * jdk.jdi    Should the jdk.jdi be removed from this com/sun/jdi test?
>>
>>
>> http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html
>>
>>   *  @modules java.corba
>>   *           jdk.jdi
>>
>>   Should the jdk.jdi be removed from this com/sun/jdi test?
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 3/15/17 11:16, Igor Ignatyev wrote:
>>> Shura,
>>>
>>> Thank you for your review. I have update test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked that there are no similar things in other TEST.properties files.
>>>
>>> Still looking for a review from a Reviewer.
>>>
>>> [1]
>>>> --- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Mon Mar 13 18:54:58 2017 -0700
>>>> +++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties     Wed Mar 15 11:09:06 2017 -0700
>>>> @@ -1,3 +1,2 @@
>>>> -modules = java.management \
>>>> -          java.logging
>>>> +modules = java.logging
>>> Thanks,
>>> — Igor
>>>
>>>> On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>>>
>>>> Igor,
>>>>
>>>> I have looked through a bunch of tests where @modules is changed - that looks good.
>>>>
>>>> One minor thing I noticed is that, in test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are explicitly calling out java.management. You do not have to do that because “modules” property is concatenated through the hierarchy.  java.management is already listed in test/java/lang/management/TEST.properties.
>>>>
>>>> Shura
>>>>
>>>>> On Mar 13, 2017, at 9:03 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>
>>>>> Shura,
>>>>>
>>>>> Thank you for review, I agree that having separate bugs is more convenient. [1] is new webrev w/ changes only in the files w/ incorrect module dependency declarations.
>>>>>
>>>>> [1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html
>>>>>
>>>>> Thanks,
>>>>> — Igor
>>>>>
>>>>>> On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline <[hidden email]> wrote:
>>>>>>
>>>>>> Igor,
>>>>>>
>>>>>> I have reviewed some number tests which change the @modules - they are fine.
>>>>>>
>>>>>> You, however, fix more things with this than missing module dependency declaration. There is a redesign of line-number-sensitive tests [1] and other multiple improvements such as in [2]. Would it be more convenient to have that as separate bugs?
>>>>>>
>>>>>> Shura
>>>>>>
>>>>>> [1] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
>>>>>> [2] http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html
>>>>>>
>>>>>>> On Mar 7, 2017, at 1:07 PM, Igor Ignatyev <[hidden email]> wrote:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>>>>> 2586 lines changed: 669 ins; 484 del; 1433 mod;
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Could you please review this changeset which fix @modules dependency declaration in jdk_svc tests?
>>>>>>> there are a couple issues w/ modules in jdk_svc tests:
>>>>>>> - some tests do not specify modules which they depend on
>>>>>>> - modules in TEST.properties is not used in cases there all tests (should) have the same @modules directive
>>>>>>> - @modules directive isn't placed according to current convention (before the 1st run directive)
>>>>>>>
>>>>>>> Since this fix has already touched lots of tests, I have decided to use this opportunity and reordered some of jtreg tags as well, so there won’t be two massive updates in the tests.
>>>>>>>
>>>>>>> Some of our tests are line number sensitive, and then I fixed jtreg declaration, they started to fail. It was really hard to find our all line number sensitive tests, so I have unified the way we declare that as a part of this fix. Please let me know if you prefer to have it done separately.
>>>>>>>
>>>>>>> There are two one-liners which, I hope, can simplify review:
>>>>>>> [1] shows only the changes which are not in comments. Besides obvious new added TEST.properties, there are changes in the following line number sensitive tests (which I mentioned before):
>>>>>>> test/com/sun/jdi/ArgumentValuesTest.java
>>>>>>> test/com/sun/jdi/BreakpointTest.java
>>>>>>> test/com/sun/jdi/FetchLocals.java
>>>>>>> test/com/sun/jdi/GetLocalVariables.java
>>>>>>> test/com/sun/jdi/GetSetLocalTest.java
>>>>>>> test/com/sun/jdi/LambdaBreakpointTest.java
>>>>>>> test/com/sun/jdi/LineNumberOnBraceTest.java
>>>>>>> test/com/sun/jdi/PopAndStepTest.java
>>>>>>>
>>>>>>> [2] shows changes in jtreg tags, it can help to see that almost all changes in jtreg tags are either moving of tags which does not affect execution order or @modules changes.
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8176176
>>>>>>> Testing:
>>>>>>> - jdk_svc on linux, windows, mac
>>>>>>> - checked that all tests which could be executed with full jdk before still can be executed with full jdk
>>>>>>>
>>>>>>> Thanks,
>>>>>>> — Igor
>>>>>>>
>>>>>>> [1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
>>>>>>> [2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"

Loading...