[RFC][icedtea-web] Unit tests for the new updates to JarSIgner (incomplete)

[Danesh Dadachanji]

On 08/04/12 07:18 PM, Omair Majid wrote:
> On 04/06/2012 05:57 PM, Danesh Dadachanji wrote:

[snip]

>> I've attached a new patch that loosens up the code a bit, I've padded
>> newlines above (and below) the chunk of asserts. There are a few more
>> modifications as follows.
>>
>> 1) The new patch also brings in the changeset that renames JarSigner to
>> JarCertVerifier[1] so hopefully it won't throw reviewers off too much.
>
> Great :)
>
>> 2) In the tests that run 1 entry than 3 entries, I've reset
>> JarCertVerifier to a new instance when the three-entry vector is used. I
>> think this may cause confusion later on because there would have been
>> cases where the JarCertVerifier would have totalSignableEntries = 4
>> instead of 3.
>
> Hm... If you are doing this, I would recommend you go all the way and
> split the 1-entry/3-entry tests into different methods to isolate the
> tests completely.
>

Done! It looks a little bit more heavy and jumps up to 22 unit tests. All in all though, I think it looks good.

>> 3) Lastly, I've added a new set of asserts using the following two methods:
>>
>>      /**
>>       * Checks if the provided cert path has signed any of the jars
>> associated
>>       * with the cert verifier.
>>       * @param certPath The cert path of the potential signer.
>>       * @return Whether or not the cert path represents one of the
>> signers of
>>       * a jar associated with the verifier.
>>       */
>>      boolean hasJarSigner(CertPath certPath)
>
> The name is not very descriptive. How about something like
> certHasSignedAnyJar? Also, why a certPath instead of a cert?
>

Yeah, not too sure what I was thinking when I came up with the name. Please see the comment below.

>>      /**
>>       * Calculate the number of signers all the jars have in total.
>>       *
>>       * Note this is not the number of common signers across all jars. It is
>>       * the number of common signers each jar has with respect to itself,
>>       * totalled together across the other jars.
>>       * @return The number of signers that have signed at least one jar
>> entirely.
>>       */
>>      int numSigners()
>>
>
> Likewise, this name doesn't convey much. I am still trying to figure out
> what this method does.
>
> Are these methods public? Any reason why we are adding these (or are
> they just for tests)gf?

Yes, these methods are just for tests)gf only. ;)

I have rethought these 2 methods and decided not to use them. What they did was essentially grab the key set of the hash map that 
managed all the signers, then check if the list contains the CertPath or return the size of the list. Instead of creating extra methods 
for duplicate info, I'm just going to use JarCertVerifier.getCerts() in the tests.

Here's a little background info on the key set. If the CertPath is a key of the hash map, that means that this CertPath has signed at 
least one jar entirely (namely, every signable entry has been signed). So if the tests run as expected, the end result is one of the 
signers being set as the key (if expected). The size is also checked to make sure nothing else slipped in by mistake. I've changed the 
asserts to use this instead. I hope this is more clear!

Main differences in this patch:
   1) Separate single-entry and many-entry tests into their own methods
   2) Rework calls to numSigners and hasJarSigner to use jcv.getCerts()'s .contains(signer) and .size()

Thanks for the review!

Cheers,
Danesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jar-signer-tests-03.patch
Type: text/x-patch
Size: 33813 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120416/6273e333/jar-signer-tests-03.patch