RFR for JDK-8028711: TEST_BUG: Tests should pass through VM options: corelibs tests
Hi, Please review the following code changes for https://bugs.openjdk.java.net/browse/JDK-8028711 The bug and fix are for enforcing the following rules: launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS} should be passed in. launch javac from shell script, ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS} should be passed in. So almost all the scripts who failed to follow above rules are touched by this fix. There are very few exceptions since test cases explicitly state that they are intended to launch java without any vm options. webrev http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/ Thanks, Michael Cui
On 26/01/2014 08:10, michael cui wrote:
Hi,
Please review the following code changes for
https://bugs.openjdk.java.net/browse/JDK-8028711
The bug and fix are for enforcing the following rules:
launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS} should be passed in. launch javac from shell script, ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS} should be passed in.
So almost all the scripts who failed to follow above rules are touched by this fix.
There are very few exceptions since test cases explicitly state that they are intended to launch java without any vm options.
webrev http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/ You might be aware of this area but we've had two previous efforts ([1] [2]) to update these tests to pass through options. I have to admit that I wasn't aware of TESTJAVAOPTS or TESTJAVACOPTS. Looking at the" jtreg -help all" page then these seem to correspond to -javaoption and -javacoption. I see -vmoption used all the time to specify options, I don't think I've seen -javaoption being used but I see your point. One thing that might be helpful here is some guidance or examples as it confusing to have so many options that initially appear to do the same thing.
Mike Duigou might want to have a quick look to see if jdk/Makefile needs to be updated to support -javacoption. As these tests aren't really testing javac (except incidentally) then it might not be important but it's yet another place where you can loose a finger. I haven't looked at all the files but just notice that you've updated the genXXX scripts. They are used to generate tests, they aren't tests themselves and so aren't run by jtreg. So are you planning to also update the java tests that create VMs to pass through the options? This point came up during previous efforts (and some test infrastructure has been added) but it isn't clear to me where we are on that. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae5d04dbacd6 [2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7da291690aa0
On 01/26/2014 05:14 PM, Alan Bateman wrote:
On 26/01/2014 08:10, michael cui wrote:
Hi,
Please review the following code changes for
https://bugs.openjdk.java.net/browse/JDK-8028711
The bug and fix are for enforcing the following rules:
launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS} should be passed in. launch javac from shell script, ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS} should be passed in.
So almost all the scripts who failed to follow above rules are touched by this fix.
There are very few exceptions since test cases explicitly state that they are intended to launch java without any vm options.
webrev http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/ You might be aware of this area but we've had two previous efforts ([1] [2]) to update these tests to pass through options. I am aware of efforts [1] JDK-8003890. Initially JDK-8028711 is opened for double check the result of JDK-8003890 and fix them if there are uncovered scripts.
Initial list scripts need to touch are : java/lang/Thread/UncaughtExceptions.sh sun/tools/common/ApplicationSetup.sh sun/tools/jrunscript/jrunscript-argsTest.sh sun/tools/jrunscript/jrunscript-cpTest.sh sun/tools/jrunscript/jrunscript-DTest.sh sun/tools/jrunscript/jrunscript-eTest.sh sun/tools/jrunscript/jrunscript-fTest.sh sun/tools/jrunscript/jrunscriptTest.sh Later on, by reading http://openjdk.java.net/jtreg/vmoptions.html and current last comments of JDK-8019502 (which is the parent of JDK-8028711), also by exploring current existing usage (I mean large part of scripts passing TESTJAVAOPTS or TESTJAVACOPTS.), I feel that we might want to make the consistent usage of these two env variables as well. I am not aware of efforts [2]. Thanks for the link.
I have to admit that I wasn't aware of TESTJAVAOPTS or TESTJAVACOPTS. Looking at the" jtreg -help all" page then these seem to correspond to -javaoption and -javacoption. I see -vmoption used all the time to specify options, I don't think I've seen -javaoption being used but I see your point. One thing that might be helpful here is some guidance or examples as it confusing to have so many options that initially appear to do the same thing. I just attached a very simple example to show the behaviours described at http://openjdk.java.net/jtreg/vmoptions.html.
Sample can download it from https://bugs.openjdk.java.net/secure/attachment/18467/sample.zip My understanding of options is : When we start jtreg with "-vmoptions" , jtreg will set its value to TESTVMOPTS and TESTTOOLVMOPTS before running shell script test (shell scripts identified by jtreg tags @test and @run shell ). So if we call java with TESTVMOPTS and call javac with TESTTOOLVMOPTS in shell script, then it impact to both. If we want to set some options only for launching java without impact other javac or other java tools, then we need to use "-javaoptions" with jtreg. (Precondition : TESTJAVAOPTS was passed) Similarly, "-javacoptions" will only impact launching javac from all shell scripts. (Precondition : TESTJAVACOPTS was passed) It would be less confusing if we could keep use them in a consistent way.
Mike Duigou might want to have a quick look to see if jdk/Makefile needs to be updated to support -javacoption. As these tests aren't really testing javac (except incidentally) then it might not be important but it's yet another place where you can loose a finger.
I haven't looked at all the files but just notice that you've updated the genXXX scripts. They are used to generate tests, they aren't tests themselves and so aren't run by jtreg.
Then I will rollback those changes. I found three scripts belongs to these category. test/java/nio/Buffer/genBasic.sh test/java/nio/Buffer/genCopyDirectMemory.sh test/java/util/Formatter/genBasic.sh
So are you planning to also update the java tests that create VMs to pass through the options? This point came up during previous efforts (and some test infrastructure has been added) but it isn't clear to me where we are on that.
I don't plan to cover that in this bug fix. I though it will be covered by JDK-8019502. I could work on that bug if you want me to cover that as well.
-Alan.
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae5d04dbacd6 [2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7da291690aa0
-Michael Cui
Hi Alan, Based on current discussions we have had and existing usages in our code base, I would like to propose that the fix should ONLY make sure that ${TESTVMOPTS} will be passed in when launching java from shell script. For other options such as ${TESTJAVAOPTS} ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS}, Since we are not very clear about needs to make them presents all the time when launching java or other java tools from shell, it is better to not make changes to current usages. Please let me know if you are ok with this approach. Thanks, Michael Cui On 01/27/2014 09:22 AM, michael cui wrote:
On 01/26/2014 05:14 PM, Alan Bateman wrote:
On 26/01/2014 08:10, michael cui wrote:
Hi,
Please review the following code changes for
https://bugs.openjdk.java.net/browse/JDK-8028711
The bug and fix are for enforcing the following rules:
launch java from shell script, ${TESTVMOPTS} and ${TESTJAVAOPTS} should be passed in. launch javac from shell script, ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS} should be passed in.
So almost all the scripts who failed to follow above rules are touched by this fix.
There are very few exceptions since test cases explicitly state that they are intended to launch java without any vm options.
webrev http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.01/ You might be aware of this area but we've had two previous efforts ([1] [2]) to update these tests to pass through options. I am aware of efforts [1] JDK-8003890. Initially JDK-8028711 is opened for double check the result of JDK-8003890 and fix them if there are uncovered scripts.
Initial list scripts need to touch are : java/lang/Thread/UncaughtExceptions.sh sun/tools/common/ApplicationSetup.sh sun/tools/jrunscript/jrunscript-argsTest.sh sun/tools/jrunscript/jrunscript-cpTest.sh sun/tools/jrunscript/jrunscript-DTest.sh sun/tools/jrunscript/jrunscript-eTest.sh sun/tools/jrunscript/jrunscript-fTest.sh sun/tools/jrunscript/jrunscriptTest.sh
Later on, by reading http://openjdk.java.net/jtreg/vmoptions.html and current last comments of JDK-8019502 (which is the parent of JDK-8028711), also by exploring current existing usage (I mean large part of scripts passing TESTJAVAOPTS or TESTJAVACOPTS.), I feel that we might want to make the consistent usage of these two env variables as well.
I am not aware of efforts [2]. Thanks for the link.
I have to admit that I wasn't aware of TESTJAVAOPTS or TESTJAVACOPTS. Looking at the" jtreg -help all" page then these seem to correspond to -javaoption and -javacoption. I see -vmoption used all the time to specify options, I don't think I've seen -javaoption being used but I see your point. One thing that might be helpful here is some guidance or examples as it confusing to have so many options that initially appear to do the same thing. I just attached a very simple example to show the behaviours described at http://openjdk.java.net/jtreg/vmoptions.html.
Sample can download it from https://bugs.openjdk.java.net/secure/attachment/18467/sample.zip
My understanding of options is :
When we start jtreg with "-vmoptions" , jtreg will set its value to TESTVMOPTS and TESTTOOLVMOPTS before running shell script test (shell scripts identified by jtreg tags @test and @run shell ). So if we call java with TESTVMOPTS and call javac with TESTTOOLVMOPTS in shell script, then it impact to both.
If we want to set some options only for launching java without impact other javac or other java tools, then we need to use "-javaoptions" with jtreg. (Precondition : TESTJAVAOPTS was passed)
Similarly, "-javacoptions" will only impact launching javac from all shell scripts. (Precondition : TESTJAVACOPTS was passed)
It would be less confusing if we could keep use them in a consistent way.
Mike Duigou might want to have a quick look to see if jdk/Makefile needs to be updated to support -javacoption. As these tests aren't really testing javac (except incidentally) then it might not be important but it's yet another place where you can loose a finger.
I haven't looked at all the files but just notice that you've updated the genXXX scripts. They are used to generate tests, they aren't tests themselves and so aren't run by jtreg.
Then I will rollback those changes. I found three scripts belongs to these category.
test/java/nio/Buffer/genBasic.sh test/java/nio/Buffer/genCopyDirectMemory.sh test/java/util/Formatter/genBasic.sh
So are you planning to also update the java tests that create VMs to pass through the options? This point came up during previous efforts (and some test infrastructure has been added) but it isn't clear to me where we are on that.
I don't plan to cover that in this bug fix. I though it will be covered by JDK-8019502. I could work on that bug if you want me to cover that as well.
-Alan.
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae5d04dbacd6 [2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7da291690aa0
-Michael Cui
On 07/02/2014 03:41, michael cui wrote:
Hi Alan,
Based on current discussions we have had and existing usages in our code base, I would like to propose that the fix should ONLY make sure that ${TESTVMOPTS} will be passed in when launching java from shell script.
For other options such as ${TESTJAVAOPTS} ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS}, Since we are not very clear about needs to make them presents all the time when launching java or other java tools from shell, it is better to not make changes to current usages.
Please let me know if you are ok with this approach. If I understand correctly then you are proposing to address the remaining issues where the vmoptions aren't passed through from shell tests. That would be good. As I mentioned, there have been efforts on this already but we either missed some tests or we didn't continue it into newer tests. The other part is java tests that launch additional VMs as we do have a few that don't pass the options through. That might be a separate bug but would be good to address.
As regards TESTJAVAOPTS then I think it might be worth starting a discussion on the jtreg list to see if there are opportunities to consolidate the number of variables that tests have to deal with. I don't have any objection to the original proposal, it's just that it makes it obvious that there are too many options and too easy to get it wrong. -Alan.
On 02/07/2014 07:03 PM, Alan Bateman wrote:
On 07/02/2014 03:41, michael cui wrote:
Hi Alan,
Based on current discussions we have had and existing usages in our code base, I would like to propose that the fix should ONLY make sure that ${TESTVMOPTS} will be passed in when launching java from shell script.
For other options such as ${TESTJAVAOPTS} ${TESTTOOLVMOPTS} and ${TESTJAVACOPTS}, Since we are not very clear about needs to make them presents all the time when launching java or other java tools from shell, it is better to not make changes to current usages.
Please let me know if you are ok with this approach. If I understand correctly then you are proposing to address the remaining issues where the vmoptions aren't passed through from shell tests. That would be good. As I mentioned, there have been efforts on this already but we either missed some tests or we didn't continue it into newer tests. The other part is java tests that launch additional VMs as we do have a few that don't pass the options through. That might be a separate bug but would be good to address. Yes, that's my suggestion. I made code changes based on this proposal and tested it locally.
Please review the newest version at : http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.03/
As regards TESTJAVAOPTS then I think it might be worth starting a discussion on the jtreg list to see if there are opportunities to consolidate the number of variables that tests have to deal with. I don't have any objection to the original proposal, it's just that it makes it obvious that there are too many options and too easy to get it wrong.
-Alan.
Thanks, -Michael Cui
On 10/02/2014 07:00, michael cui wrote:
:
Please review the newest version at : http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.03/ This looks good to me.
There are a couple of places where javac wasn't updated. A few that I noticed are: test/com/sun/corba/5036554/TestCorbaBug.sh test/sun/tools/jconsole/ResourceCheckTest.sh test/sun/tools/native2ascii/resources/ImmutableResourceTest.sh A minor comment on test/java/net/URLPermission/nstest/lookup.sh is that the resulting line length is 182 characters and this will likely be annoying for future side-by-side views. So I think I'd split this while you are there. -Alan.
On 02/10/2014 05:36 PM, Alan Bateman wrote:
On 10/02/2014 07:00, michael cui wrote:
:
Please review the newest version at : http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.03/ This looks good to me.
There are a couple of places where javac wasn't updated. A few that I noticed are:
test/com/sun/corba/5036554/TestCorbaBug.sh test/sun/tools/jconsole/ResourceCheckTest.sh test/sun/tools/native2ascii/resources/ImmutableResourceTest.sh
The original code change did adding ${TESTTOOLVMOPTS} to javac, but I dropped it from new proposal. My understanding is that vmoption for javac would not change the compiler's behaviour. I could be wrong. Do you think we really need to pass such vm options to javac? And if it is needed, than how about other java tools such as jar, rmiregistry, ... etc.
A minor comment on test/java/net/URLPermission/nstest/lookup.sh is that the resulting line length is 182 characters and this will likely be annoying for future side-by-side views. So I think I'd split this while you are there. I will split it after finalize the changes that we want to make.
-Alan.
-Michael Cui
On 02/10/2014 05:36 PM, Alan Bateman wrote:
A minor comment on test/java/net/URLPermission/nstest/lookup.sh is that the resulting line length is 182 characters and this will likely be annoying for future side-by-side views. So I think I'd split this while you are there. Please review the updated version at http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.04/ <http://cr.openjdk.java.net/%7Etyan/michael/JDK-8028711/webrev.04/>
Changes includes : 1. split line if it longer than 80 characters. 2. merge the fix of JDK-8033897 <https://bugs.openjdk.java.net/browse/JDK-8033897> 3. add few missed scripts. If no further changes need to be made, I would like to find sponsor to push this fix. Michael Cui
On 12/02/2014 12:59, michael cui wrote:
On 02/10/2014 05:36 PM, Alan Bateman wrote:
A minor comment on test/java/net/URLPermission/nstest/lookup.sh is that the resulting line length is 182 characters and this will likely be annoying for future side-by-side views. So I think I'd split this while you are there. Please review the updated version at http://cr.openjdk.java.net/~tyan/michael/JDK-8028711/webrev.04/ <http://cr.openjdk.java.net/%7Etyan/michael/JDK-8028711/webrev.04/>
Changes includes :
1. split line if it longer than 80 characters. 2. merge the fix of JDK-8033897 <https://bugs.openjdk.java.net/browse/JDK-8033897> 3. add few missed scripts.
If no further changes need to be made, I would like to find sponsor to push this fix. The changes look okay to me. I see you decided to ignore the javac usages but that is okay and can be done another time.
Also thanks for fixing lookup.sh. I don't personally mind lines > 80 but that one was >180 which make it difficult to look at side-by-side changes. -Alan.
On 02/13/2014 02:35 AM, Alan Bateman wrote:
The changes look okay to me. I see you decided to ignore the javac usages but that is okay and can be done another time.
Also thanks for fixing lookup.sh. I don't personally mind lines > 80 but that one was >180 which make it difficult to look at side-by-side changes. Hi Alan,
Thanks so much for reviewing this! Would you mind to sponsor this for me? Regrads, Michael Cui
participants (2)
-
Alan Bateman
-
michael cui