RFC [iceedtea-web] added first set of reproducres

[Jiri Vanek]

On 06/20/2011 03:56 PM, Omair Majid wrote:
> Hi Jiri,
>
> On 06/20/2011 05:48 AM, Jiri Vanek wrote:
>> All those reproducers are based on older issues found, reproduced and
>> fixed by Omair (and maybe others I'm not aware about) - so are passing.
>>
>> Regards J.
>>
>
> Overall, the patch looks fine to me. Please see the comments in-line below.
>
>> diff -r e413e4676929 tests/jnlp_tests/simple/AccessClassInPackage/srcs/AccessClassInPackage.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/jnlp_tests/simple/AccessClassInPackage/srcs/AccessClassInPackage.java Mon Jun 20 11:38:50 2011 +0200
>> @@ -0,0 +1,6 @@
>> +public class AccessClassInPackage {
>> +
>> + public static void main(String[] args) throws Exception{
>> + Class.forName("sun.security.internal.spec.TlsKeyMaterialSpec");
>> + }
>> +}
>
> This test is missing a copyright notice. Is there a reason you are adding copyrights to files under tescases but not to those under resources/srcs?

Ok. Added. I had (probably bad)  remembrance  that  reproducers should not have the copyright. It sounds myslef quit foolsi now:-/
>
>> diff -r e413e4676929 tests/jnlp_tests/simple/AccessClassInPackage/testcases/AccessClassInPackageTest.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/jnlp_tests/simple/AccessClassInPackage/testcases/AccessClassInPackageTest.java Mon Jun 20 11:38:50 2011 +0200
>> @@ -0,0 +1,67 @@
>> +/* AccessClassInPackageTest.java
>> +Copyright (C) 2011 Red Hat, Inc.
>> +
>> +This file is part of IcedTea.
>> +
>> +IcedTea is free software; you can redistribute it and/or
>> +modify it under the terms of the GNU General Public License as published by
>> +the Free Software Foundation, version 2.
>> +
>> +IcedTea is distributed in the hope that it will be useful,
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> +General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with IcedTea; see the file COPYING. If not, write to
>> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> +02110-1301 USA.
>> +
>> +Linking this library statically or dynamically with other modules is
>> +making a combined work based on this library. Thus, the terms and
>> +conditions of the GNU General Public License cover the whole
>> +combination.
>> +
>> +As a special exception, the copyright holders of this library give you
>> +permission to link this library with independent modules to produce an
>> +executable, regardless of the license terms of these independent
>> +modules, and to copy and distribute the resulting executable under
>> +terms of your choice, provided that you also meet, for each linked
>> +independent module, the terms and conditions of the license of that
>> +module. An independent module is a module which is not derived from
>> +or based on this library. If you modify this library, you may extend
>> +this exception to your version of the library, but you are not
>> +obligated to do so. If you do not wish to do so, delete this
>> +exception statement from your version.
>> + */
>> +
>> +
>> +import net.sourceforge.jnlp.ServerAccess;
>> +import org.junit.Assert;
>> +
>> +import org.junit.Test;
>> +
>> +public class AccessClassInPackageTest {
>> +
>> + private static ServerAccess server = new ServerAccess();
>> +
>> +
>> +
>> + @Test
>> + public void AccessClassInPackageTestLunch1() throws Exception {
>> + System.out.println("connecting AccessClassInPackageTest request");
>> + ServerAccess.ProcessResult pr=server.executeJavawsHeadless(null,"/AccessClassInPackage.jnlp");
>> + System.out.println(pr.stdout);
>> + System.err.println(pr.stderr);
>> + Assert.assertTrue(pr.stderr.contains("java.security.AccessControlException: access denied (java.lang.RuntimePermission accessClassInPackage.sun.security.internal.spec)"));
>> + Assert.assertFalse(pr.stderr.contains("ClassNotFoundException"));
>> + Assert.assertFalse(pr.stdout.length()>2);
>> + Assert.assertFalse(pr.wasTerminated);
>> + Assert.assertEquals((Integer)0, pr.returnValue);
>> + }
>> +
>> +
>> +
>> +
>> +
>> + }
>
> 5 blank lines seems a little too much. Normally we stick with 1 or two blanks lines. This is also present in most of the new tests in the patch. Please fix this.

Sorry for this:-/ done.
>
>> diff -r e413e4676929 tests/jnlp_tests/simple/ReadProperties/srcs/ReadProperties.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/jnlp_tests/simple/ReadProperties/srcs/ReadProperties.java Mon Jun 20 11:38:50 2011 +0200
>> @@ -0,0 +1,10 @@
>> +public class ReadProperties {
>> +
>> +
>> + public static void main(String[] args) {
>> +
>> + // System.getProperty("user.name");
>> + // System.getProperty("user.home");
>> + System.getProperty(args[0]);
>> + }
>> +}
>
> I am a little curious as to why the commented out lines are present. Is there a problem with System.getProperty("user.name")? Or is it just leftover code from when you were testing?

I had left them as "documentation" what is expected to be passed through arguments. They are left in new patch. Do you really want them removed?
>
> Cheers,
> Omair

Thank you for review!

J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_reproducers2.patch
Type: text/x-patch
Size: 52505 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110620/77109999/add_reproducers2.patch