[rfc][icedtea-web] signed applets test

[Danesh Dadachanji]

Hi Jiri,

Nice work! I have a few comments below.

On 11/04/12 05:34 AM, Jiri Vanek wrote:
> Hi!
>
> This is implementation of what we have agreed for signed applets - add(and remove) certificate to ~/.icedtea/security/trusted.certs,
> launch applet signed by this cert in explorer and read stdout/in as we are doing for applet tests right now.
>
> There is question how to deal with -Xtrustall which is now useless when there is trusted certificate (all signed reproducers are right
> now signed with same cert, and when more to much reproducers will need to much different certificates this will need re-factoring
> probably too).
>

I don't mind -Xtrustall staying, I think it helps for testing stuff on the fly, outside the test suite. However, I'm not too strong 
about this so if you want to remove it, feel free.

> After this change is in, the PR905 reproducer have sense to go in too.
>

+1

> changelog:
>
> 2012-04-11 Jiri Vanek <jvanek at redhat.com>
>
> Allowed signed appletes in automatic reproducers tests
> * tests/jnlp_tests/signed/AppletTestSigned/resources/AppletTestSigned.html:
> html file for lunching signed applet. Its stile is different from the one for
> calling unsigned applets.

There are a few typos, it should be:

     html file for launching signed applet. It still is different from the one for
     calling unsigned applets.

> * tests/jnlp_tests/signed/AppletTestSigned/resources/bd.css: style of signed
> applets page
> * tests/jnlp_tests/signed/AppletTestSigned/resources/AppletTestSigned.jnlp:
> jnlp fiel for lunched signed applet

s/fiel/file/

> * tests/jnlp_tests/signed/AppletTestSigned/srcs/AppletTestSigned.java
> body of signed applet
> * tests/jnlp_tests/signed/AppletTestSigned/testcases/AppletTestSignedTests.java:
> (AppletTestSignedTest) testing method to launch signed applet in javaws
> (AppletTestSignedFirefoxTest) testing method to launch signed applet in
> browser
> * Makefile.am: KEYSTORE_PASS, EXPORTED_CERT, TEST_CERT_ALIAS, PUBLIC_KEYSTORE
> PUBLIC_PASS: new globnal variables holding keystores credentials

s/globnal/global/ && s/keystores/keystore/ (or store's, both work)

> (clean-local) clean-bootstrap-directory moved to be last one, as keytool
> is necessary for removing certificate
> (EXPORTED_CERT): new target exporting certificate from testing keystore
> (stamps/netx-dist-tests-import-cert-to-public) new target to
> certificate to PUBLIC_KEYSTORE
> (netx-dist-tests-remove-cert-from-public) new target removing testing
> certificate from PUBLIC_KEYSTORE
> (clean-netx-dist-tests) now depends on netx-dist-tests-remove-cert-from-public
> and is removing EXPORTED_CERT file

You missed a few ":" chars after the braces.

 > diff -r cde6d59a2901 Makefile.am
 > --- a/Makefile.am	Thu Apr 05 10:57:16 2012 -0400
 > +++ b/Makefile.am	Wed Apr 11 11:32:36 2012 +0200
 > @@ -23,6 +23,12 @@
 >   JNLP_TESTS_SERVER_DEPLOYDIR=$(TESTS_DIR)/jnlp_test_server
 >   JNLP_TESTS_DIR=$(TESTS_DIR)/jnlp_tests
 >   KEYSTORE_NAME=teststore.ks
 > +KEYSTORE_PASS=123456789

I'm curious, why was this password chosen over the regular "changeit"? I know that you're getting it from below but I'm wondering if 
anyone knows why the original dev chose this over the normal one. I would rather it be "changeit" for consistency's sake though. What 
do you think?

 > +EXPORTED_CERT=icedteatests.crt
 > +TEST_CERT_ALIAS=icedteaweb
 > +PUBLIC_KEYSTORE=~/.icedtea/security/trusted.certs
 > +PUBLIC_PASS=changeit
 > +

[snip]

 > diff -r cde6d59a2901 tests/jnlp_tests/signed/AppletTestSigned/resources/AppletTestSigned.html
 > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
 > +++ b/tests/jnlp_tests/signed/AppletTestSigned/resources/AppletTestSigned.html	Wed Apr 11 11:32:36 2012 +0200

Heh, nice failure message. ;) It's because the applet was eventually killed, right?

I'm worried that such a harsh failure message will throw testers off, they may not realize it is meant to fail to run. Thoughts? It 
might be useful to add a "(This applet was killed, it should not be running)" message too?

[snip]

diff -r cde6d59a2901 tests/jnlp_tests/signed/AppletTestSigned/resources/AppletTestSigned.html
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tests/jnlp_tests/signed/AppletTestSigned/resources/AppletTestSigned.html	Wed Apr 11 11:32:36 2012 +0200
[...]
 > +<vendor>NetX</vendor>
 > +<homepage href="http://jnlp.sourceforge.net/netx/"/>

Please change these to use IcedTea and the test page[1], thanks.

[snip]

 > diff -r cde6d59a2901 tests/jnlp_tests/signed/AppletTestSigned/resources/bd.css
 > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
 > +++ b/tests/jnlp_tests/signed/AppletTestSigned/resources/bd.css	Wed Apr 11 11:32:36 2012 +0200

Shouldn't this have a copyright header too? Otherwise if it's easier, you can include it directly in the HTML file instead of a new file.

[snip]

 > diff -r cde6d59a2901 tests/jnlp_tests/signed/AppletTestSigned/srcs/AppletTestSigned.java
 > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
 > +++ b/tests/jnlp_tests/signed/AppletTestSigned/srcs/AppletTestSigned.java	Wed Apr 11 11:32:36 2012 +0200
 > @@ -0,0 +1,82 @@
 > +
 > +import java.applet.Applet;
 > +
 > +/* AppletTestSigned.java
 > +Copyright (C) 2011 Red Hat, Inc.

s/2011/2012/


[snip]

 > diff -r cde6d59a2901 tests/jnlp_tests/signed/AppletTestSigned/testcases/AppletTestSignedTests.java
 > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
 > +++ b/tests/jnlp_tests/signed/AppletTestSigned/testcases/AppletTestSignedTests.java	Wed Apr 11 11:32:36 2012 +0200
 > @@ -0,0 +1,100 @@
 > +/* AppletTestTests.java
 > +Copyright (C) 2011 Red Hat, Inc.

s/2011/2012/

[snip]

 > +        Assert.assertTrue("AppletTestSigned stdout should contains " + s3 + " bud didn't", pr.stdout.contains(s3));
 > +        String s0 = "AppletTestSigned was started";
 > +        Assert.assertTrue("AppletTestSigned stdout should contains " + s0 + " bud didn't", pr.stdout.contains(s0));
 > +        String s1 = "value1";
 > +        Assert.assertTrue("AppletTestSigned stdout should contains " + s1 + " bud didn't", pr.stdout.contains(s1));
 > +        String s2 = "value2";
 > +        Assert.assertTrue("AppletTestSigned stdout should contains " + s2 + " bud didn't", pr.stdout.contains(s2));
 > +        String s4 = "AppletTestSigned was stopped";
 > +        Assert.assertFalse("AppletTestSigned stdout shouldn't contains " + s4 + " bud did", pr.stdout.contains(s4));
 > +        String s5 = "AppletTestSigned will be destroyed";
 > +        Assert.assertFalse("AppletTestSigned stdout shouldn't contains " + s5 + " bud did", pr.stdout.contains(s5));
 > +        String ss = "xception";
 > +        Assert.assertFalse("AppletTestSigned stderr should not contains " + ss + " but did", pr.stderr.contains(ss));

For "AppletTestSigned stdout should contains ", s/ contains / contain / (without the s). Also s/bud/but/ =)

Everything else looks good! Thanks for getting this done.

Cheers,
Danesh

[1] http://icedtea.classpath.org/wiki/IcedTea-Web#Testing_IcedTea-Web